Support specific members from System.[U]IntPtr by cston · Pull Request #41728 · dotnet/roslyn

cston · 2020-02-15T22:07:40Z

Expose overridden methods GetHashCode(), Equals(object obj), and ToString() from nint and nuint.
Test plan #38821

cston · 2020-02-20T16:18:10Z

@dotnet/roslyn-compiler please review, thanks.

AlekseyTs · 2020-02-20T22:25:18Z

src/Compilers/CSharp/Portable/Symbols/NativeIntegerTypeSymbol.cs

        /// sizeof(nint) should be used instead of Size;
        /// + and - should be used instead of Add() and Subtract();
-        /// ToInt32(), ToInt64(), ToPointer() should be used from System.IntPtr only;
-        /// overridden methods Equals(), GetHashCode(), and ToString() are referenced from System.Object.


/// overridden methods Equals(), GetHashCode(), and ToString() are referenced from System.Object. [](start = 8, length = 97)

Just curious, why doing this wasn't good enough? #Closed

It resulted in bad code generated for callers such as F() below:

class C { readonly nint n; string F() => n.ToString(); }

Result:

IL_0000: ldarg.0 IL_0001: ldflda native int C::n IL_0006: constrained. [mscorlib]System.IntPtr IL_000c: callvirt instance string [mscorlib]System.Object::ToString() IL_0011: ret

Expected:

.locals init (native int V_0) IL_0000: ldarg.0 IL_0001: ldfld native int C::n IL_0006: stloc.0 IL_0007: ldloca.s V_0 IL_0009: call instance string [mscorlib]System.IntPtr::ToString() IL_000e: ret

See ReadOnlyField_VirtualMethods test.

In reply to: 382292681 [](ancestors = 382292681)

It resulted in bad code generated for callers

Let's imaging there is a virtual method in base that IntPtr doesn't override. Would we generate the same bad code for that method invocation?

In reply to: 382302890 [](ancestors = 382302890,382292681)

It sounds like there is a bug in the verifier. I can build and successfully run this code:

class C { readonly nint n1; readonly System.IntPtr n2; string F1() => n1.ToString(); string F2() => n2.ToString(); static void Main() { System.Console.WriteLine(new C().F1()); System.Console.WriteLine(new C().F2()); } } struct nint {}

But PEVerify complains about IL in F1.

So, if that verification error is the only reason to synthesize members, I think we should give this another thought.

In reply to: 382316178 [](ancestors = 382316178,382302890,382292681)

I'll remove the members from NativeIntegerTypeSymbol for now.

In reply to: 382719607 [](ancestors = 382719607,382316178,382302890,382292681)

AlekseyTs · 2020-02-20T22:29:20Z

src/Compilers/CSharp/Portable/Symbols/SubstitutedMethodSymbol.cs

            }
        }

-        public sealed override bool ReturnsVoid


public sealed override bool ReturnsVoid [](start = 8, length = 39)

Is this covered by WrappedMethodSymbol? #Closed

Yes.

In reply to: 382294420 [](ancestors = 382294420)

AlekseyTs · 2020-02-20T22:36:14Z

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

            internal string lazyDefaultMemberName;
            internal NamedTypeSymbol lazyComImportCoClassType = ErrorTypeSymbol.UnknownResultType;
            internal ThreeState lazyHasEmbeddedAttribute = ThreeState.Unknown;
+            internal NativeIntegerTypeSymbol lazyNativeInt;


internal NativeIntegerTypeSymbol lazyNativeInt; [](start = 12, length = 47)

I think we can cache this on the assembly symbol. It feels wasteful to spend this memory for every "uncommon" type symbol just so we could use this slot on two types. #Closed

See MetadataOrSourceAssemblySymbol._lazySpecialTypes

In reply to: 382297303 [](ancestors = 382297303)

We can simply add two more slots at the end of the array

In reply to: 382299283 [](ancestors = 382299283,382297303)

AlekseyTs · 2020-02-20T22:42:22Z

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


        private ThreeState _lazyIsExplicitDefinitionOfNoPiaLocalType = ThreeState.Unknown;

+        private NativeIntegerTypeSymbol _lazyNativeInt;


private NativeIntegerTypeSymbol _lazyNativeInt; [](start = 8, length = 47)

Same comment as for PE #Closed

AlekseyTs · 2020-02-20T22:47:32Z

src/Compilers/CSharp/Portable/Symbols/NativeIntegerTypeSymbol.cs

-        /// There are no members on <see cref="System.IntPtr"/> that should be exposed directly on nint:
+        /// There only members of <see cref="System.IntPtr"/> that are exposed directly on nint
+        /// are the overridden methods Equals(), GetHashCode(), and ToString(). For the other members:
        /// constructors for nint other than the default parameterless constructor are not supported;


constructors for nint other than the default parameterless constructor are not supported; [](start = 12, length = 89)

Should we add synthesized parameter-less constructors then? I believe we do this for all structs, even imported ones. #Closed

AlekseyTs · 2020-02-20T22:49:47Z

src/Compilers/CSharp/Portable/Symbols/NativeIntegerTypeSymbol.cs

+                    accessWithinOpt: null);
+                if (method is object)
+                {
+                    builder.Add(new NativeIntegerMethodSymbol(containingType, method));


new NativeIntegerMethodSymbol(containingType, method) [](start = 32, length = 53)

Where do we make sure the methods are unified with underlying methods for the purpose of IL generation? #Closed

Same for types, for example typeof(nint).

In reply to: 382302745 [](ancestors = 382302745)

I think it would be good to find a good place to assert that Cci layer never sees instances of these synthetic symbols, both types and members.

In reply to: 382303254 [](ancestors = 382303254,382302745)

AlekseyTs · 2020-02-20T22:56:10Z

src/Compilers/CSharp/Portable/Symbols/NativeIntegerTypeSymbol.cs


 namespace Microsoft.CodeAnalysis.CSharp.Symbols
 {
    internal sealed class NativeIntegerTypeSymbol : WrappedNamedTypeSymbol


NativeIntegerTypeSymbol [](start = 26, length = 23)

It feels like we need to be able to retarget these types. Do we handle that? #Pending

Added a PROTOTYPE comment for now.

In reply to: 382305226 [](ancestors = 382305226)

AlekseyTs · 2020-02-20T23:03:08Z

src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntTests.cs

+        private static void VerifyMembers(NamedTypeSymbol type)
+        {
+            var memberNames = type.MemberNames;
+            var allMembers = type.GetMembers();


allMembers [](start = 16, length = 10)

I think it would be good to assert that, each time we call GetMembers, we get the same instances for members. #Closed

AlekseyTs · 2020-02-20T23:05:09Z

src/Compilers/CSharp/Portable/Symbols/NativeIntegerTypeSymbol.cs

        /// <summary>
-        /// There are no members on <see cref="System.IntPtr"/> that should be exposed directly on nint:
+        /// There only members of <see cref="System.IntPtr"/> that are exposed directly on nint
+        /// are the overridden methods Equals(), GetHashCode(), and ToString(). For the other members:


are the overridden methods Equals(), GetHashCode(), and ToString() [](start = 12, length = 66)

Should we instead build the set dynamically by picking up all members that are overriding members from underlying? #Closed

AlekseyTs · 2020-02-20T23:19:45Z

src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntTests.cs

-        // CodeGenerator.EmitCallExpression() is not expecting a System.ValueType without an overload of ToString().
-        [Fact(Skip = "PEVerify failure")]
+        [Fact]
        public void ReadOnlyField_VirtualMethods()


ReadOnlyField_VirtualMethods [](start = 20, length = 28)

I think it would be good to also test delegate creations with these methods. Also, expression trees with calls and delegate creations. #Closed

AlekseyTs · 2020-02-20T23:23:47Z

Done with review pass (iteration 2) #Closed

AlekseyTs · 2020-02-20T23:31:23Z

src/Compilers/CSharp/Portable/Symbols/NativeIntegerTypeSymbol.cs

+            }
+        }
+
+        private sealed class NativeIntegerMethodSymbol : WrappedMethodSymbol


NativeIntegerMethodSymbol [](start = 29, length = 25)

It looks like container is equal to underlying, but members are not. Should they be? #Closed

AlekseyTs · 2020-02-20T23:31:57Z

src/Compilers/CSharp/Portable/Symbols/NativeIntegerTypeSymbol.cs

+            internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree localTree) => throw ExceptionUtilities.Unreachable;
+        }
+
+        private sealed class NativeIntegerParameterSymbol : WrappedParameterSymbol


NativeIntegerParameterSymbol [](start = 29, length = 28)

Same question about equality. #Closed

AlekseyTs · 2020-02-24T21:31:29Z

Done with review pass (iteration 3) #Closed

AlekseyTs · 2020-02-24T21:54:34Z

src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntTests.cs

+{42.GetHashCode()}");
+            // PROTOTYPE: Should we generate the following instead, which would correspond to the equivalent
+            // code generated for `_i.ToString()` if `_i` was `int`? If NativeIntegerTypeSymbol.GetMembers() included
+            // members for ToString(), Equals(), GetHashCode(), that would fix this issue and the PEVerify failure above.


members for ToString(), Equals(), GetHashCode(), that would fix this issue and the PEVerify failure above. [](start = 15, length = 106)

Consider explicitly stating that the code we generate right now is equivalent to a code for a regular structure that doesn't override ToString() #Closed

cston · 2020-02-24T23:36:40Z

@dotnet/roslyn-compiler, please review. The change is now limited to caching symbols on the containing assembly.

AlekseyTs

LGTM (iteration 7). Please make sure to adjust final commit message to reflect the actual changes in the PR.

gafter

Support specific members from System.[U]IntPtr

e2b2561

cston requested a review from a team as a code owner February 15, 2020 22:07

Misc.

619efc6

cston added the Area-Compilers label Feb 17, 2020

AlekseyTs reviewed Feb 20, 2020

View reviewed changes

PR feedback

012333d

Remove wrapper methods

2c99564

AlekseyTs reviewed Feb 24, 2020

View reviewed changes

cston added 3 commits February 24, 2020 14:36

PR feedback

4d824c9

Misc.

e8b3cd6

Misc.

8011f2d

AlekseyTs approved these changes Feb 25, 2020

View reviewed changes

gafter approved these changes Feb 26, 2020

View reviewed changes

cston merged commit 3ec85a1 into dotnet:features/NativeInt Feb 26, 2020

cston deleted the members branch February 26, 2020 20:46

cston mentioned this pull request Apr 30, 2020

Expose specific members of System.[U]IntPtr from native integer types #43766

Merged

cston added a commit to cston/roslyn that referenced this pull request Apr 30, 2020

PR feedback from dotnet#41728

fd89628


		private ThreeState _lazyIsExplicitDefinitionOfNoPiaLocalType = ThreeState.Unknown;

		private NativeIntegerTypeSymbol _lazyNativeInt;

Conversation

cston commented Feb 15, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Feb 20, 2020

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cston commented Feb 15, 2020 •

edited by jcouv

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

cston Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited by cston

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs commented Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs Feb 20, 2020 •

edited

Loading

AlekseyTs commented Feb 24, 2020 •

edited

Loading

AlekseyTs Feb 24, 2020 •

edited

Loading