Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Implement IntPtr/UIntPtr enum casts in Linq expressions#13981

Closed
hughbe wants to merge 2 commits into
dotnet:masterfrom
hughbe:yes-intptr-enums-do-exist
Closed

Implement IntPtr/UIntPtr enum casts in Linq expressions#13981
hughbe wants to merge 2 commits into
dotnet:masterfrom
hughbe:yes-intptr-enums-do-exist

Conversation

@hughbe
Copy link
Copy Markdown

@hughbe hughbe commented Nov 25, 2016

@JonHanna @stephentoub @bartdesmet @VSadov

The strange creatures that are IntPtr and UIntPtr enums are explicitly supported in the CLI specification. Whether anyone has ever used one and if supporting them was accidental is a different story.

The previous behaviour was debug assertions in both compiler and interpreter on compilation and occasionally on creation of the trees

}
}

public static Type FloatEnumType
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is not currently used but snuck its way into the PR, the cheeky bugger. I can remove it if you want but when I attempt to add support for float/double enums, it'll come back

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Float enums? And people call mutable structs evil! Again though, if the spec allows such an enum, IMO expressions should support them.

@JonHanna
Copy link
Copy Markdown
Contributor

Gosh, I was really sure those weren't allowed, and ECMA-335 is turning to blur when I try to find it. Where is it said that it is?

If they're allowed, they're pretty horrid, but we should allow them all the same.

@hughbe
Copy link
Copy Markdown
Author

hughbe commented Nov 25, 2016

Specification:
II.14.3 Enums
An enum (short for enumeration) defines a set of symbols that all have the same type. A type shall be an enum if and only if it has an immediate base type of System.Enum. Since System.Enum itself has an immediate base type of System.ValueType, (see Partition IV) enums are value types (§II.13) The symbols of an enum are represented by an underlying integer type: one of { bool, char, int8, unsigned int8, int16, unsigned int16, int32, unsigned int32, int64, unsigned int64, native int, unsigned native int }

@hughbe
Copy link
Copy Markdown
Author

hughbe commented Nov 25, 2016

It worth mentioning float/double enums aren't explicitly mentioned in the spec, but the CLR does support them, again probably by accident, so we probably do need to.

@bartdesmet
Copy link
Copy Markdown
Contributor

@dotnet-bot test Innerloop Windows_NT Debug Build and Test please (https://github.com/dotnet/corefx/issues/3689)

@bartdesmet
Copy link
Copy Markdown
Contributor

Supporting IntPtr and UIntPtr per the CLI specification seems okay. Interesting to see floating points being supported in the implementation (maybe all value types are considered valid as underlying enum types by the CLR). Personally, I wouldn't bend over backwards to support these unspecified creatures.

@JonHanna
Copy link
Copy Markdown
Contributor

JonHanna commented Nov 25, 2016

Ah. For some reason I didn't make the native intIntPtr connection. Bug in my mental compiler :) (And also not that horrid, I was thinking of IntPtr as mapped to a pointer when I called it that).

Floating point enums would have some difficulties. Should a value that maps to signalling NaN equate to one that maps to quiet NaN? Or maybe it shouldn't even be equal to itself? Perhaps that it's accepted should be considered a bug. In any case if the CIL spec doesn't say it's allowed, I suggest Expressions shouldn't.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Nov 25, 2016

Floating point enums would have some difficulties

Floating point enums are a gray area: They are not explicitly rejected by the runtime type loader; but a lot of the corelib methods fail on them in spectacular ways.

@JonHanna
Copy link
Copy Markdown
Contributor

I think the half-cat/half-octopus at the top of this page is a good representation of a float enum. Except that it looks happy.
The rest of this PR LG2M.

@hughbe
Copy link
Copy Markdown
Author

hughbe commented Nov 25, 2016

Thanks Jon and Bart - I've remove the float/double helpers (dead code anyway!)

From a purely academic point of view I'd love to see Expressions supporting everything the runtime provides - if there were to be support for this in Expressions I think we'd just delegate to the runtime's implementation of float/double conversions:

  • conv.r4
  • conv.r8

etc. etc.

However, it seems that there's some pushback to the feature, so not worth doing?

@hughbe
Copy link
Copy Markdown
Author

hughbe commented Nov 25, 2016

Supporting IntPtr and UIntPtr per the CLI specification seems okay. Interesting to see floating points being supported in the implementation (maybe all value types are considered valid as underlying enum types by the CLR). Personally, I wouldn't bend over backwards to support these unspecified creatures.

Yeah its strange. The CLR supports primitives only, a subset of value types. I genuinely think it might have been an oversight by the original CLR author. Perhaps @jkotas could enlighten us on the history - was there a conscientious decision to support float/double enums? If so, why was this never specified? Thanks

@bartdesmet
Copy link
Copy Markdown
Contributor

Interestingly, the 2003 ECMA specification I have laying around defines the set of valid underlying types for enums in II.13.3 including float32 and float64. It seems that got removed.

ISBN 0-321-15493-2

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Nov 26, 2016

Perhaps @jkotas could enlighten us on the history - was there a conscientious decision to support float/double enums?

The type loader does not prohibit float/double enums. I believe that the original intent in CLR v1.0 was to support them. It is why they are in the old version of spec.

However, it was later discovered that there are like hundred things broken throughout the stack for float/double/intptr/uintptr enums. There was conscious decision made to not fix the whole stack to work well for them because of the right behavior is often unclear, and it is hard to test and very low value because of such enums cannot be expressed in C#.

One example from many: Enum.GetTypeCode(): https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Enum.cs#L992

@JonHanna
Copy link
Copy Markdown
Contributor

Hmm. @jkotas' comment above has made me cast doubt on this endeavour. If CLR is broken for such types, we will come up against it sooner or later.

@JonHanna
Copy link
Copy Markdown
Contributor

From a purely academic point of view I'd love to see Expressions supporting everything the runtime provides

Yes. Once one does a little in that regard it becomes a bit addictive.

@VSadov
Copy link
Copy Markdown
Member

VSadov commented Nov 29, 2016

I do not think it is valuable to support a feature that CLR itself fails to support properly. IMO that just adds support burden and a risk that CLR deprecates a broken feature entirely.

I am not even sure it is worth adding code that checks for these things and gives friendly errors. The probability that this will be useful to anybody is very low.

@hughbe
Copy link
Copy Markdown
Author

hughbe commented Dec 1, 2016

Fine by me, thanks for taking a look guys

@hughbe hughbe closed this Dec 1, 2016
@hughbe hughbe deleted the yes-intptr-enums-do-exist branch December 1, 2016 14:32
@karelz karelz added this to the 1.2.0 milestone Dec 3, 2016
@karelz karelz added this to the 1.2.0 milestone Dec 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants