Expose specific members of System.[U]IntPtr from native integer types by cston · Pull Request #43766 · dotnet/roslyn

cston · 2020-04-28T23:14:09Z

Only exclude a few specific members from the underlying types from native integer types. (Previously, only a few specific members were included.) Include all interfaces implemented by the underlying types.

Fixes #42453
Fixes #42957
Fixes #43657

Test plan #38821

AlekseyTs · 2020-04-30T13:32:31Z

@cston I think the title of the PR is confusing. It doesn't exclude members it includes them, but not all of them. Consider renaming the PR before people start commenting on it and adjusting PR's description to accurately reflect the nature of the changes. #Closed

AlekseyTs · 2020-04-30T13:35:10Z

Also, if I remember correctly, a number of issues were raised on a previous PR that tried to expose members. Were they taken into consideration in this PR? #Closed

cston · 2020-04-30T15:44:40Z

I think the title of the PR is confusing. It doesn't exclude members it includes them, but not all of them. Consider renaming the PR

I wanted the title to reflect the resulting behavior, that a few specific members are excluded, rather than the previous behavior where specific members were included. I agree it's not completely clear though.

AlekseyTs · 2020-04-30T15:52:14Z

rather than the previous behavior where specific members were included.

I believe that the previous behavior (before this PR) is that none of the members were included. What am I missing? #Closed

cston · 2020-04-30T16:13:08Z

I believe that the previous behavior (before this PR) is that none of the members were included.

Sorry, you are correct. I was thinking of virtual members defined on System.Object, but the native integer types did not expose any members directly.

cston · 2020-04-30T20:49:47Z

Also, if I remember correctly, a number of issues were raised on a previous PR that tried to expose members. Were they taken into consideration in this PR?

The previous PR was #41728. I've updated this PR based on that feedback, thanks.

AlekseyTs · 2020-04-30T21:28:29Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs


                    return methodRef;
                }
+                else if (methodSymbol is NativeIntegerMethodSymbol { UnderlyingMethod: MethodSymbol underlyingMethod })


else if (methodSymbol is NativeIntegerMethodSymbol { UnderlyingMethod: MethodSymbol underlyingMethod }) [](start = 16, length = 103)

Please make sure we have a test for an expression lambda using the method, including the case when the method is generic. #Closed

NativeIntegerTests.ReadOnlyField_VirtualMethods

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

AlekseyTs · 2020-04-30T21:29:48Z

            return methodSymbol;

It feels like we might need to do something here for a constructed generic method. Please add a test like that. #Closed

Refers to: src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs:1182 in 08c6617. [](commit_id = 08c6617, deletion_comment = False)

AlekseyTs · 2020-04-30T21:45:15Z

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

            considerCallingConvention: false, //ignore static-ness
            considerRefKindDifferences: false,
-            typeComparison: TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreDynamic);
+            typeComparison: TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreDynamic | TypeCompareKind.IgnoreNativeIntegers);


TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreDynamic | TypeCompareKind.IgnoreNativeIntegers [](start = 28, length = 194)

TypeCompareKind.AllIgnoreOptions & ~TypeCompareKind.IgnoreTupleNames? #Closed

AlekseyTs · 2020-04-30T21:45:57Z

    public static readonly MemberSignatureComparer CSharpWithoutTupleNamesComparer = new MemberSignatureComparer(

AllIgnoreComparer? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs:146 in 08c6617. [](commit_id = 08c6617, deletion_comment = False)

AlekseyTs · 2020-04-30T21:47:51Z

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

            considerCallingConvention: false, //ignore static-ness
            considerRefKindDifferences: false,
-            typeComparison: TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreDynamicAndTupleNames);
+            typeComparison: TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreDynamicAndTupleNames | TypeCompareKind.IgnoreNativeIntegers);


TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreDynamicAndTupleNames | TypeCompareKind.IgnoreNativeIntegers [](start = 28, length = 207)

TypeCompareKind.AllIgnoreOptions? It looks like other comparers could also use the same simplification. #Closed

AlekseyTs · 2020-04-30T21:48:09Z

    public static readonly MemberSignatureComparer CSharpWithTupleNamesComparer = new MemberSignatureComparer(

The name of this comparer (and the next one) is somewhat misleading consider renaming. For example, this could be name IgnoreAllButTupleNamesComparer #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs:133 in 08c6617. [](commit_id = 08c6617, deletion_comment = False)

AlekseyTs · 2020-04-30T22:04:17Z

            return methodSymbol;

I see that we do not expose generic methods at the moment, consider adding an assert here that methodSymbol.OriginalDefinition and methodSymbol.ConstructedFrom are not NativeIntegerMethodSymbol

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

Refers to: src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs:1182 in 08c6617. [](commit_id = 08c6617, deletion_comment = False)

AlekseyTs · 2020-04-30T22:12:23Z

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

+                                        }
+                                    }
+                                    break;
+                                case MethodKind.ExplicitInterfaceImplementation:


MethodKind.ExplicitInterfaceImplementation [](start = 37, length = 42)

I do not think we can trust this completely. I.e. I think it is possible to have a different kind and still be explicit interface implementation. What exactly are we trying to accomplish here? Perhaps we should simply add everything that is MetadataVirtual, that would take care of all overrides and interface implementations. #Closed

cston · 2020-04-30T22:17:58Z

    public static readonly MemberSignatureComparer CSharpWithoutTupleNamesComparer = new MemberSignatureComparer(

It does not include TypeCompareKind.ObliviousNullableModifierMatchesAny which is part of TypeCompareKind.AllIgnoreOptions.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs:146 in 08c6617. [](commit_id = 08c6617, deletion_comment = False)

AlekseyTs · 2020-04-30T22:18:28Z

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

+            {
+                var builder = ArrayBuilder<Symbol>.GetInstance();
+                builder.Add(new SynthesizedInstanceConstructor(this));
+                foreach (var underlyingMember in underlyingMembers)


foreach (var underlyingMember in underlyingMembers) [](start = 16, length = 51)

Are we interested in cloning static members? #Closed

Yes.

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

AlekseyTs · 2020-04-30T22:22:22Z

    public static readonly MemberSignatureComparer CSharpWithoutTupleNamesComparer = new MemberSignatureComparer(

It does not include TypeCompareKind.ObliviousNullableModifierMatchesAny which is part of TypeCompareKind.AllIgnoreOptions.

When we use TypeCompareKind.IgnoreNullableModifiersForReferenceTypes, presence or absence of TypeCompareKind.ObliviousNullableModifierMatchesAny makes no difference and we should include it anyway for completeness.

In reply to: 622145910 [](ancestors = 622145910,622132199)

Refers to: src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs:146 in 08c6617. [](commit_id = 08c6617, deletion_comment = False)

AlekseyTs · 2020-04-30T22:37:48Z

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

+            {
+                var builder = ArrayBuilder<Symbol>.GetInstance();
+                builder.Add(new SynthesizedInstanceConstructor(this));
+                foreach (var underlyingMember in underlyingMembers)


foreach (var underlyingMember in underlyingMembers) [](start = 16, length = 51)

Are we interested in cloning members inaccessible outside the type? #Closed

gafter

AlekseyTs · 2020-05-04T21:07:01Z

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

        internal sealed override NamedTypeSymbol NativeIntegerUnderlyingType => _underlyingType;

-        internal override bool Equals(TypeSymbol t2, TypeCompareKind comparison, IReadOnlyDictionary<TypeParameterSymbol, bool>? isValueTypeOverrideOpt = null) => _underlyingType.Equals(t2, comparison, isValueTypeOverrideOpt);
+        internal override bool Equals(TypeSymbol? other, TypeCompareKind comparison, IReadOnlyDictionary<TypeParameterSymbol, bool>? isValueTypeOverrideOpt = null)


Equals [](start = 31, length = 6)

I would expect us to also override equality for error types representing native/regular ints. #Closed

AlekseyTs · 2020-05-04T22:38:34Z

                            return includeUnderlyingMember(method.AssociatedSymbol);

Is this check useful? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:400 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-04T22:45:18Z

    private sealed class SymbolComparer : IEqualityComparer<Symbol>

Can we use symbol comparer that we have in the compiler? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:618 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-04T22:49:19Z

            if (includesIEquatable)

Does it make sense to add some verification for the else case? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:1470 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

cston · 2020-05-04T22:53:54Z

    private sealed class SymbolComparer : IEqualityComparer<Symbol>

Do we have a symbol comparer that compares Symbol instances rather than ISymbol?

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:618 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-04T22:54:26Z

    IntPtr I<IntPtr>.F() => this;

These are going to be private and will be filtered out. It is not clear how exactly we are testing ExplicitImplementations if none of the members we are going to cone have them. Spcifically, I am referring to the comment on the test: "ExplicitImplementations properties are empty on native integer types." #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2160 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-04T23:04:51Z

    private sealed class SymbolComparer : IEqualityComparer<Symbol>

I am not sure how should I interpret closing the thread "By Design" with a question. If there is question, should the thread be kept active?

Do we have a symbol comparer that compares Symbol instances rather than ISymbol?

Would Microsoft.CodeAnalysis.CSharp.Symbols.SymbolEqualityComparer work?

In reply to: 623749896 [](ancestors = 623749896,623747048)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:618 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-04T23:21:26Z

Done with review pass (iteration 13) #Closed

cston · 2020-05-05T03:43:18Z

                            return includeUnderlyingMember(method.AssociatedSymbol);

We should ignore accessors for properties that are ignored.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:400 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

cston · 2020-05-05T03:44:09Z

    private sealed class SymbolComparer : IEqualityComparer<Symbol>

The comparer wasn't used after all.

In reply to: 623753405 [](ancestors = 623753405,623749896,623747048)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:618 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

cston · 2020-05-05T04:24:18Z

    IntPtr I<IntPtr>.F() => this;

Updated comment.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2160 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-05T16:10:30Z

            if (includesIEquatable)

Does it make sense to add some verification for the else case?

This thread is marked as resolved without any response or obviously related code change. Could you please elaborate?

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:1470 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-05T16:12:34Z

    // Explicit implementations of methods and properties are not included in native integer types.

Are we actually testing what the original comment "ExplicitImplementations properties are empty on native integer types." used to imply? Methods and properties with ExplicitImplementations do not have to be private. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2110 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

cston · 2020-05-05T16:13:26Z

            if (includesIEquatable)

Added a call to VerifyInterfaces() above.

In reply to: 624148862 [](ancestors = 624148862,623748417)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:1470 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

cston · 2020-05-05T16:14:55Z

    // Explicit implementations of methods and properties are not included in native integer types.

Line 537 checks ExplicitImplementations for all members.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2110 in cb7b6ac. [](commit_id = cb7b6ac, deletion_comment = False)

AlekseyTs · 2020-05-05T16:17:01Z

    // Explicit implementations of methods and properties are not included in native integer types.

Line 537 checks ExplicitImplementations for all members.

But do we test a scenario where the original method/property that we clone has non empty ExplicitImplementations?

In reply to: 624151322 [](ancestors = 624151322,624149990)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2110 in cb1ed0e. [](commit_id = cb1ed0e, deletion_comment = False)

AlekseyTs · 2020-05-05T16:18:40Z

Done with review pass (iteration 15) #Closed

AlekseyTs

LGTM (iteration 16)

* upstream/master: (495 commits) Simplify PEPropertySymbol constructor (dotnet#43990) Remove duplicate calls to ThrowIfCancellationRequested Allow msbuild to pass properties and metadata as analyzerconfig: (dotnet#43617) Use a private exception type for error types encountered instead of ArgumentException. Remove unnecessary check Code review feedback Adding tests and expanding side effect cases Expose specific members of System.[U]IntPtr from native integer types (dotnet#43766) Remove trailing space from BoundDecisionDagNode Don't cache FileInfo instances. Add 16.7 Preview 1 to the publish data config Bump prerelease version for 16.7 preview 2 Only run code style analyzers when nullable warnings are enabled Remove Code review feedback Lint Undo Fix regression in global:: qualified constant in a switch case. Fixes dotnet#43960 Undo Simplify ...

cston added the Area-Compilers label Apr 28, 2020

cston added this to the 16.7 milestone Apr 28, 2020

cston force-pushed the new-members branch from 2d6b385 to b2ac71a Compare April 29, 2020 05:31

Exclude specific members of System.[U]IntPtr from native integer types

7b05157

cston force-pushed the new-members branch from b2ac71a to 7b05157 Compare April 29, 2020 18:58

cston marked this pull request as ready for review April 29, 2020 19:57

cston requested a review from a team as a code owner April 29, 2020 19:57

cston added 2 commits April 29, 2020 15:46

Fix tests

e02302a

PEModuleBuilder.Translate

6a653de

cston changed the title ~~Exclude specific members of System.[U]IntPtr from native integer types~~ Expose specific members of System.[U]IntPtr from native integer types Apr 30, 2020

cston added 2 commits April 30, 2020 13:50

PR feedback from dotnet#41728

fd89628

Revert change

08c6617

AlekseyTs reviewed Apr 30, 2020

View reviewed changes

gafter approved these changes May 3, 2020

View reviewed changes

Misc.

cb1ed0e

AlekseyTs reviewed May 4, 2020

View reviewed changes

PR feedback

0da8843

Update MissingMetadataTypeSymbol.TopLevel.Equals()

cb7b6ac

PR feedback

76f4bb8

AlekseyTs approved these changes May 5, 2020

View reviewed changes

cston merged commit 26222e1 into dotnet:master May 5, 2020

ghost modified the milestones: 16.7, Next May 5, 2020

cston deleted the new-members branch May 5, 2020 20:58

JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

cston commented Apr 30, 2020

Uh oh!

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

Uh oh!

Uh oh!

cston commented Apr 30, 2020

Uh oh!

cston commented Apr 30, 2020

Uh oh!

AlekseyTs Apr 30, 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 Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs Apr 30, 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 Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 30, 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 Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Apr 30, 2020

Uh oh!

AlekseyTs Apr 30, 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 commented Apr 30, 2020

Uh oh!

AlekseyTs Apr 30, 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 Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs Apr 30, 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!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 4, 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 May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

cston commented Apr 28, 2020 •

edited by jcouv

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs May 4, 2020 •

edited

Loading

AlekseyTs commented May 4, 2020 •

edited

Loading

AlekseyTs commented May 4, 2020 •

edited

Loading

AlekseyTs commented May 4, 2020 •

edited

Loading

AlekseyTs commented May 4, 2020 •

edited

Loading

AlekseyTs commented May 4, 2020 •

edited

Loading

AlekseyTs commented May 5, 2020 •

edited

Loading

AlekseyTs commented May 5, 2020 •

edited

Loading