Emit and roundtrip underlying instance field #73407

jcouv · 2024-05-09T19:12:06Z

Relates to test plan #66722
Corresponding spec details: dotnet/csharplang#8121

jcouv · 2024-05-10T19:56:33Z

AlekseyTs · 2024-05-13T15:25:31Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+                            return default;
+                        }
+
+                        Debug.Assert(foundUnderlyingInstanceField.IsNil);


Debug.Assert(foundUnderlyingInstanceField.IsNil);

Consider flagging the duplicate as an unsupported case instead of simply asserting. #Closed

AlekseyTs · 2024-05-13T15:31:39Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

-                            underlyingType = type;
+                            // Validate instance field
+                            if (!underlyingInstanceField.IsNil
+                                && !validateUnderlyingInstanceField(underlyingInstanceField, moduleSymbol, type))


!validateUnderlyingInstanceField(underlyingInstanceField, moduleSymbol, type)

It feels like field validation doesn't belong in the loop that checks parameters. Why can't we do this after the loop? #Closed

AlekseyTs · 2024-05-13T15:38:47Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

@@ -2046,6 +2086,26 @@ bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSym
                Debug.Assert(underlyingType is not null);
                return true;
            }
+
+            bool validateUnderlyingInstanceField(FieldDefinitionHandle underlyingInstanceFieldHandle, PEModuleSymbol moduleSymbol, TypeSymbol underlyingType)


bool validateUnderlyingInstanceField(FieldDefinitionHandle underlyingInstanceFieldHandle, PEModuleSymbol moduleSymbol, TypeSymbol underlyingType)

It is not obvious to me why do we care if the field is present and why do we need to perform any validation for it. #Closed

There's two reasons to care about the presence of the field:

for Unsafe.As to be safe, we need the extension to be properly formed. We could do lighter validation (just the type) but I don't see the point to accept ill-formed extension types

this.UnderlyingMember in an instance extension type member will need to be rewritten to access this field, something like this.<UnderlyingInstanceField>$.UnderlyingMember (this will be in a later PR)

this.UnderlyingMember in an instance extension type member will need to be rewritten to access this field, something like this.<UnderlyingInstanceField>$.UnderlyingMember.

I don't think we will ever need to do something like this for an imported extension type. We will never be emitting code for its members

You're correct. That leaves reason 1

AlekseyTs · 2024-05-13T15:40:10Z

                    type = new ExtendedErrorTypeSymbol(type, LookupResultKind.NotReferencable, info, unreported: true);

Does it make sense to continue after this point? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs:2045 in 6aa7e12. [](commit_id = 6aa7e12, deletion_comment = False)

AlekseyTs · 2024-05-13T15:41:45Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

            try
            {
                foreach (var fieldRid in module.GetFieldsOfTypeOrThrow(_handle))
                {
                    try
                    {
+                        if (!underlyingInstanceField.IsNil && fieldRid == underlyingInstanceField)


if (!underlyingInstanceField.IsNil && fieldRid == underlyingInstanceField)

Could we simply filter out any instance fields in an extension type? #Closed

AlekseyTs · 2024-05-13T15:47:15Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceExtensionTypeSymbol.cs

+
+                if (_lazyUnderlyingInstanceField is null)
+                {
+                    var field = new SynthesizedFieldSymbol(this, extendedType, WellKnownMemberNames.ExtensionFieldName);


new SynthesizedFieldSymbol(this, extendedType, WellKnownMemberNames.ExtensionFieldName);

Should this field be explicitly aligned? #Closed

Fields in structs use LayoutKind.Sequential by default. Extensions do the same:

internal sealed override TypeLayout Layout { get { var data = GetDecodedWellKnownAttributeData(); if (data != null && data.HasStructLayoutAttribute) { return data.Layout; } if (this.TypeKind is TypeKind.Struct or TypeKind.Extension) { // PROTOTYPE consider disallowing attribute for explicit layout on extension types // CLI spec 22.37.16: // "A ValueType shall have a non-zero size - either by defining at least one field, or by providing a non-zero ClassSize" // // Dev11 compiler sets the value to 1 for structs with no instance fields and no size specified. // It does not change the size value if it was explicitly specified to be 0, nor does it report an error. return new TypeLayout(LayoutKind.Sequential, this.HasInstanceFields() ? 0 : 1, alignment: 0); } return default(TypeLayout); } }

Fields in structs use LayoutKind.Sequential by default.

Could you point me to a documentation confirming that the first instance field will start at offset 0 then?

From ECMA CLI (II.10.1.2):

sequential: The CLI shall lay out the fields in sequential order, based on the order of the fields in the logical metadata table (§II.22.15).

[Rationale: [...] sequential layout is intended to instruct the CLI to match layout rules commonly followed by languages like C and C++ on an individual platform, where this is possible while still guaranteeing verifiable layout. [...]

From the C99 standard section 6.7.2.1 bullet point 13 (unfortunately, gated):

Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared. A pointer to a structure object, suitably converted, points to its initial member (or if that member is a bit-field, then to the unit in which it resides), and vice versa. There may be unnamed padding within a structure object, but not at its beginning.

AlekseyTs · 2024-05-13T15:53:58Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

+
+            if (!type.IsStatic)
+            {
+                var fieldDef = reader.GetFieldDefinition(fieldDefHandle);


var fieldDef = reader.GetFieldDefinition(fieldDefHandle);

It feels like it would be easier to crate an instance of PEFieldSymbol here and validate it instead. #Closed

Thanks. That is a nice simplification indeed

AlekseyTs · 2024-05-13T15:56:48Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

@@ -54,7 +57,7 @@ private static void AssertSetStrictlyEqual(string[] expected, string[] actual)
    }

    // Verify things that are common for all extension types
-    private static void VerifyExtension<T>(TypeSymbol type, bool? isExplicit, SpecialType specialType = SpecialType.None) where T : TypeSymbol
+    private static void VerifyExtension<T>(TypeSymbol type, bool? isExplicit, SpecialType specialType = SpecialType.None, string fieldType = null) where T : TypeSymbol


, string fieldType = null

Why do we need to pass this explicitly? Shouldn't the type match the underlyingType that we extract from type below? #Closed

AlekseyTs · 2024-05-13T15:58:31Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

+                var fieldTypeDisplay = decoder.DecodeFieldSignature(ref blob);
+
+                // The instance value field has the expected type
+                Assert.Equal(fieldType, fieldTypeDisplay);


Assert.Equal(fieldType, fieldTypeDisplay);

It feels somewhat surprising that validation on import is more thorough than in the tests. #Closed

I didn't follow. What do you mean?

I didn't follow. What do you mean?

I mean validateUnderlyingInstanceField function checks more things than this code.

Added missing checks

AlekseyTs · 2024-05-13T16:00:41Z

Are details of the field that we are supposed to emit specified anywhere? #Resolved

AlekseyTs · 2024-05-13T16:08:40Z

Done with review pass (commit 1)

AlekseyTs · 2024-05-13T20:57:50Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

@@ -18,6 +20,7 @@
 using Roslyn.Test.Utilities;
 using Roslyn.Utilities;
 using Xunit;
+using static Roslyn.Test.Utilities.MetadataReaderUtils;


Is this using necessary? #Closed

AlekseyTs · 2024-05-13T21:23:26Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+                if (mrEx is not null
+                    || signatureHeader.IsGeneric
+                    || paramInfos.Length <= 1
+                    || this.Layout is not { Kind: LayoutKind.Sequential, Alignment: 0, Size: 0 })


|| this.Layout is not { Kind: LayoutKind.Sequential, Alignment: 0, Size: 0 })

It feels like this can be checked even before we call to tryDecodeExtensionType. I think that one looking for this check will have a very hard time finding it because it is unlikely that one would expect to find the check where the method's signature is verified. #Closed

I think this location is find for this check. This method does not only concern itself with the method's signature.
I could move it earlier in the method if that helps.

The alternative would be putting the check in TryGetExtensionInfo, but then an unexpected layout would result in the type not being considered an extension at all.
The boundary between "not an extension" and a "bad extension" is fungible, but the layout check feels more like the latter category.

Oh, I think you're suggesting to do the check in this method, but before the local function is called. That's fine too

AlekseyTs · 2024-05-13T21:25:20Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

+                Assert.Equal(Accessibility.Private, peField.DeclaredAccessibility);
+                Assert.False(peField.IsStatic);
+                Assert.False(peField.IsReadOnly);
+                Assert.Equal(RefKind.None, peField.RefKind);


Assert.Equal(RefKind.None, peField.RefKind);

It looks like layout expectations are not asserted #Closed

AlekseyTs · 2024-05-13T21:38:12Z

Done with review pass (commit 5)

jcouv · 2024-05-14T02:37:27Z

Opened corresponding spec PR dotnet/csharplang#8121

In reply to: 2108098258

jjonescz · 2024-05-14T07:10:52Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

@@ -1954,6 +1984,14 @@ private void DecodeExtensionType(out bool isExplicit, out TypeSymbol underlyingT
            // PROTOTYPE consider optimizing/caching

            TypeSymbol? foundUnderlyingType;


Consider moving this declaration after the if since it's not used in there. #Resolved

jjonescz · 2024-05-14T07:21:27Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

@@ -3198,7 +3228,45 @@ public class C<T> { }
        var comp = CreateCompilation(src, targetFramework: TargetFramework.Net70);
        comp.VerifyDiagnostics();

-        CompileAndVerify(comp, symbolValidator: validate, sourceSymbolValidator: validate, verify: Verification.FailsPEVerify);
+        var verifier = CompileAndVerify(comp, symbolValidator: validate, sourceSymbolValidator: validate, verify: Verification.FailsPEVerify);
+        // Note: we don't emit a DynamicAttribute on synthesized field


Should this be in the speclet? #Resolved

I'll add a note there

jjonescz · 2024-05-14T07:24:14Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

+		73 00 00
+	)
+	// Fields
+	.field private class C`1<object> '<UnderlyingInstance>$'


'<UnderlyingInstance>$'

Did you want to use {{WellKnownMemberNames.ExtensionFieldName}} here like in most other tests? #Resolved

jjonescz

LGTM if runtime team agrees this representation is fine

AlekseyTs · 2024-05-14T19:05:15Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

+        {
+            var module = (PEModuleSymbol)type.ContainingModule;
+            var reader = module.Module.GetMetadataReader();
+            var fieldDefHandle = reader.GetTypeDefinition(peType.Handle).GetFields()


fieldDefHandle

It would be good to assert that there are no other instance fields in the type #Resolved

AlekseyTs

LGTM (commit 7)

jcouv added Area-Compilers New Language Feature - Roles Roles labels May 9, 2024

jcouv self-assigned this May 9, 2024

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label May 9, 2024

jcouv force-pushed the emit-instances branch 4 times, most recently from a215b6b to b78e1ea Compare May 9, 2024 22:09

Emit and roundtrip underlying instance field

6aa7e12

jcouv force-pushed the emit-instances branch from b78e1ea to 6aa7e12 Compare May 9, 2024 22:18

jcouv marked this pull request as ready for review May 9, 2024 22:18

jcouv requested a review from a team as a code owner May 9, 2024 22:18

jcouv requested review from jjonescz and AlekseyTs May 9, 2024 22:18

AlekseyTs reviewed May 13, 2024

View reviewed changes

jcouv added 3 commits May 13, 2024 13:18

Address feedback

9b00941

Add PROTOTYPE comment

b892e3c

Validate layout too

bcc08a4

AlekseyTs reviewed May 13, 2024

View reviewed changes

Address feedback

3b18289

AlekseyTs reviewed May 13, 2024

View reviewed changes

Address feedback

5fff14e

jjonescz reviewed May 14, 2024

View reviewed changes

Address feedback

be50c30

jcouv requested review from jjonescz and AlekseyTs May 14, 2024 16:18

jjonescz approved these changes May 14, 2024

View reviewed changes

AlekseyTs reviewed May 14, 2024

View reviewed changes

AlekseyTs approved these changes May 14, 2024

View reviewed changes

Check there are no other instance fields

cec3e26

jcouv enabled auto-merge (squash) May 15, 2024 02:19

jcouv merged commit 3dc87ab into dotnet:features/roles May 15, 2024
24 checks passed

		@@ -1954,6 +1984,14 @@ private void DecodeExtensionType(out bool isExplicit, out TypeSymbol underlyingT
		// PROTOTYPE consider optimizing/caching

		TypeSymbol? foundUnderlyingType;

Emit and roundtrip underlying instance field #73407

Emit and roundtrip underlying instance field #73407

Conversation

jcouv commented May 9, 2024 • edited

jcouv commented May 10, 2024

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 13, 2024 • edited

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 13, 2024 • edited by jcouv

AlekseyTs commented May 13, 2024

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 13, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs commented May 13, 2024

jcouv commented May 14, 2024

jjonescz May 14, 2024 • edited by jcouv

Choose a reason for hiding this comment

jjonescz May 14, 2024 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz May 14, 2024 • edited by jcouv

Choose a reason for hiding this comment

jjonescz left a comment

Choose a reason for hiding this comment

AlekseyTs May 14, 2024 • edited by jcouv

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented May 9, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs commented May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs commented May 13, 2024 •

edited by jcouv

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

AlekseyTs May 13, 2024 •

edited

jjonescz May 14, 2024 •

edited by jcouv

jjonescz May 14, 2024 •

edited by jcouv

jjonescz May 14, 2024 •

edited by jcouv

AlekseyTs May 14, 2024 •

edited by jcouv