Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #123 - Union types without sub-classes should be sealed #143

Closed
wants to merge 9 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Jan 27, 2015

This implements the F# Language User Voice feature request to change the F# compiled representations of union types by making union types sealed when their compiled representations have no sub-types.

This is also covered by suggestion #123.

Since this changes compiled forms in FSharp.Core.dll it is a candidate to consider for F# 4.0.

@latkin
Copy link
Contributor

latkin commented Jan 27, 2015

👍

type X2 = A | B of string
do check "vwllfewlkefw4" (typedefof<X2>.IsSealed) false
type X3 = A | B | C
do check "vwllfewlkefw5" (typedefof<X3>.IsSealed) false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns true -- is it expected value or behavior that is incorrect?

@latkin
Copy link
Contributor

latkin commented Jan 27, 2015

Can you please update the codegen tests which are now failing? There are a bunch that use DUs which are now out of date.

@dsyme
Copy link
Contributor Author

dsyme commented Jan 28, 2015

@latkin - Ok, will do

I found it a bit tricky to update the one micro codegen test I added, because the *.il generated files get deleted by run RunTests.cmd process somewhere. Is there an option to keep these around? That would make it much simpler.

@latkin
Copy link
Contributor

latkin commented Jan 28, 2015

@dsyme yes this is a pain point. Let me see if there is something simple we could add to make that easier...

@dsyme
Copy link
Contributor Author

dsyme commented Jan 29, 2015

baselines now updated

@latkin
Copy link
Contributor

latkin commented Jan 30, 2015

These are still failing for me:

  • CodeGen\EmittedIL\SerializableAttribute (ToplevelNamespace.fs - Desktop)
  • CodeGen\EmittedIL\SerializableAttribute (ToplevelNamespace.fs - Portable)
  • Optimizations\GenericComparison (Compare05.fs - NetFx40)
  • Optimizations\GenericComparison (Compare07.fs - NetFx40)
  • Optimizations\GenericComparison (Compare10.fs - NetFx40)
  • Optimizations\GenericComparison (Equals04.fs - NetFx40)
  • Optimizations\GenericComparison (Equals06.fs - NetFx40)
  • Optimizations\GenericComparison (Equals09.fs - NetFx40)
  • Optimizations\GenericComparison (Hash05.fs - NetFx40)
  • Optimizations\GenericComparison (Hash06.fs - NetFx40)
  • Optimizations\GenericComparison (Hash09.fs - NetFx40)
  • Optimizations\GenericComparison (Hash12.fs - NetFx40)

Detective work indicates the TopLevelNamespace guys were patched up by hand based on similar fix to TopLevelModule. However namespace guys turn out to need fix in more locations.

And I assume it was not realized that there are codegen tests under 'Optimizations'.

@dsyme
Copy link
Contributor Author

dsyme commented Jan 30, 2015

Updated, thanks. BTW if you get a chance could you look at the Perl problem in AppVeyor? https://ci.appveyor.com/project/KevinRansom/visualfsharp-radou/build/0.0.1.18

@dsyme
Copy link
Contributor Author

dsyme commented Jan 31, 2015

Those "Optimization" codegen tests are still failing, will look at it in the morning :)

@latkin
Copy link
Contributor

latkin commented Jan 31, 2015

Don't forget the portable stuff in the core\array tests, too ;-)

@dsyme
Copy link
Contributor Author

dsyme commented Feb 13, 2015

Now updated

@dsyme dsyme closed this in 35e6a00 Feb 13, 2015
@latkin latkin added the fixed label Feb 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants