Preserve enum type in custom attribute argument of type obj (#995)#19975
Preserve enum type in custom attribute argument of type obj (#995)#19975edgarfgp wants to merge 18 commits into
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
9649085 to
013cb43
Compare
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
Nice, targeted fix for #995 — the ILAttribElem.Enum carrier plus the symmetric encode tag (0x55 + type name) are the right model, and consolidating the 0x55 decode into decodeCustomAttrElemType removes the duplicated named-arg branch. Two correctness concerns about the decode side, both in decodeILAttribData/parseVal in src/Compiler/AbstractIL/il.fs — see inline comments.
Summary:
- The encode path now writes the enum's true underlying width, but the decode fallback still reads a fixed
int32, so boxed enums whose underlying type isn'tint32(e.g. theLongEnum/int64case this PR adds) don't round-trip throughdecodeILAttribDataand can desync the blob pointer for any following named args. The runtime test passes only because the CLR — notdecodeILAttribData— reads the metadata. - The new
ILType.Value _ -> ILAttribElem.Enum(ty, v)wrap in theSystem.Objectbranch also fires for boxed primitives (int/bool/char/byte…), not just enums, mislabeling them asEnumon decode.
Boxed primitives (et_I4, et_BOOLEAN, ...) also decode to ILType.Value, so the previous match wrapped them as Enum too. Gate the wrap on the 0x55 enum tag so only real enums become ILAttribElem.Enum; primitives stay as their primitive element. Addresses PR review feedback.
| ILAttribElem.Array(elemTy, elems), sigptr | ||
| | ILType.Value _ -> (* assume it is an enumeration *) | ||
| | ILType.Value _ -> | ||
| // Assume an enumeration. Note: the underlying integer width is not present in the |
There was a problem hiding this comment.
When is this code path exercised?
Pls demonstrate with a test cases that uses long-backed enum and also has value(s) which would not fit an int32.
| // integer (e.g. 'Prop = MyEnum.B' surfaces as boxed int32). See | ||
| // https://github.com/dotnet/fsharp/issues/995. The enum type is carried alongside the | ||
| // underlying integer value, which is computed by recursing with the underlying IL type. | ||
| | Expr.Const(c, m, ty), _ when ilArgTy.TypeSpec.Name = "System.Object" && isEnumTy g ty -> |
There was a problem hiding this comment.
Don't we have a function for checking ilArgTy being object instead of a vanilla name check?
- GenAttribArg: check ilArgTy against g.ilg.typ_Object (both the enum guard and the isobj binding) instead of a string name check. - decodeILAttribData: deduplicate the enum-decode comments (single source for the int32 note).
…ck comment unchanged
ilArgTy = g.ilg.typ_Object is not structurally equal to a System.Object arg imported from a referenced assembly (different scope), so string/const args to object-typed attribute members from external attributes (e.g. NUnit's [<TestCase(...)>]) hit the "may not be used as a custom attribute value" internal error. Use the scope-agnostic name check, as the surrounding code does.
Problem
Assigning an enum to a custom attribute argument of type
objloses the enum type. The value is stored in metadata as the underlyingint32, so reading the attribute back gives a plainintrather than the enum. C# stores the same thing as the enum, so this is also a cross-language inconsistency.Fixes #995.
Before
After
The enum type is now written into the attribute blob using the ECMA-335 enum tag (
0x55+ type name), the same encoding C# produces, so it round-trips as the enum. Reading that encoding back (which previously threw) is also handled, and decode/encode are kept symmetric so static linking preserves it.Quotations
The original triage worried this needed quotation/TAST changes. It doesn't: the fix only reads the enum type (already on the
Constnode) when emitting the attribute. Quotations are untouched and already keep the enum type added a test to confirm.