Fix GetHashCode on constructed types with annotated type arguments by jcouv · Pull Request #35613 · dotnet/roslyn

jcouv · 2019-05-09T21:18:12Z

If you have C<T~> (definition) and a type C<T!> or C<T?> constructed from it, they should have the same value for GetHashCode(), namely the object-hash of the definition.

Fixes #30673

If this looks okay, then I'll extend this PR to also cover VB.

Fixes #30677

Tagging @AlekseyTs

jcouv · 2019-05-09T21:22:17Z

+        // The implemented interface is Outer<T!>.Inner<U!>.Interface
+        internal class Derived6 : Outer<T>.Inner<U>.Interface
+        {
+            // The explicit interface is Outer<T>.Inner<U!>.Interface


Tagging @AlekseyTs for discussion.
This explicit interface uses the definition of Outer<T>, but I'm thinking it should use a fully constructed type instead: Outer<T!>.Inner<U!>.Interface as when the type is completely spelled out (in the implemented interface two lines above, or in the test ExplicitInterface_WithExplicitOuter below).
What do you think? #Closed

This explicit interface uses the definition of Outer, but I'm thinking it should use a fully constructed type instead: Outer<T!>.Inner<U!>.Interface as when the type is completely spelled out (in the implemented interface two lines above, or in the test ExplicitInterface_WithExplicitOuter below).

I think this is something worth discussing. But I am not sure why Outer<T!>.Inner<U!> is a better choice and whether current annotation context should play a role the same way as it does when the name is spelled out.

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

Filed #35619

In reply to: 283579930 [](ancestors = 283579930,282670985)

jcouv · 2019-05-10T17:46:00Z

@AlekseyTs Could you take another look when you have some time?

I debated testing scenarios where hashes should be different C<string> vs. C<T>, as the hash codes should probably be different but that's not deterministic... What do you think?

Still have not looked at VB. #Closed

AlekseyTs · 2019-05-10T17:58:01Z

I debated testing scenarios where hashes should be different C vs. C, as the hash codes should probably be different but that's not deterministic... What do you think?

Testing inequality of hash code wouldn't be the right thing to do. There is no requirement for hash code to be different for not equal objects. #Closed

AlekseyTs · 2019-05-13T23:16:49Z

+            if (wasConstructedForAnnotations(type))
+            {
+                // A type that uses its own type parameters as type arguments was constructed only for the purpose of adding annotations
+                // In that case we'll want to compute a hash that ignores such annotations and matches the object-hash from the definition


object-hash [](start = 107, length = 11)

I think we should simply say "hash code fro the definition", whatever that hash code might be. #Closed

AlekseyTs · 2019-05-13T23:18:52Z

+                do
+                {
+                    var typeArguments = type.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics;
+                    var typeParameters = type.ConstructedFrom.TypeParameters;


ConstructedFrom [](start = 46, length = 15)

I think this should be OriginalDefinition instead of ConstructedFrom. #Closed

AlekseyTs · 2019-05-13T23:20:58Z

+
+                    for (int i = 0; i < typeArguments.Length; i++)
+                    {
+                        if (!typeArguments[i].Type.OriginalDefinition.Equals(typeParameters[i], TypeCompareKind.CLRSignatureCompareOptions))


TypeCompareKind.CLRSignatureCompareOptions [](start = 96, length = 42)

Passing TypeCompareKind here is not necessary, we are comparing type parameter symbols. #Closed

That is assuming that both sides are definitions

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

AlekseyTs · 2019-05-13T23:21:46Z

+
+                    type = type.ContainingType;
+
+                }


} [](start = 16, length = 1)

It looks like there is an unnecessary empty line above. #Closed

AlekseyTs · 2019-05-13T23:24:15Z

+            Assert.False(c.Equals(c2));
+            Assert.True(c.Equals(c2, TypeCompareKind.CLRSignatureCompareOptions));
+
+            Assert.True(c2.GetHashCode() == c.GetHashCode(), "hash codes differed even though types matched (ignoring nullability)");


Assert.True [](start = 12, length = 11)

Can we use Assert.Equal instead? #Closed

I chose to use Assert.True so that I could put a message as a comment.

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

AlekseyTs · 2019-05-13T23:27:13Z

+            Assert.True(type.IsDefinition);
+
+            var type2 = comp.GetMember<MethodSymbol>("C.M").ReturnType;
+            Assert.Equal("C<T>", type2.ToTestDisplayString());


() [](start = 58, length = 2)

Consider using includeNonNullable: true to see the difference with oblivious. #Closed

AlekseyTs · 2019-05-13T23:36:10Z

+            var c2 = cDefinition.Construct(ImmutableArray.Create(TypeWithAnnotations.Create(cDefinition.TypeParameters.Single(), NullableAnnotation.NotAnnotated)));
+            var i2 = c2.GetTypeMember("I");
+            Assert.Equal("C<T>.I<U>", i2.ToTestDisplayString());
+            Assert.True((object)i2.OriginalDefinition == iDefinition);


Assert.True [](start = 12, length = 11)

Please use Assert.Same(iDefinition, i2.OriginalDefinition) instead. #Closed

AlekseyTs · 2019-05-13T23:48:10Z

+            Assert.Equal("C<T>.I<U>", i2constructed.ToTestDisplayString());
+            AssertHashCodesMatch(iDefinition, i2constructed);
+
+            var i2b = i2.Construct(ImmutableArray.Create(TypeWithAnnotations.Create(i2.TypeParameters.Single(), NullableAnnotation.Annotated)));


var i2b = i2.Construct(ImmutableArray.Create(TypeWithAnnotations.Create(i2.TypeParameters.Single(), NullableAnnotation.Annotated))); [](start = 12, length = 132)

Consider also adding scenario when iDefinition is constructed this way. Consider also testing scenario when iDefinition.TypeParameters are used to construct i2 in the same manner. #Closed

I'll stop by tomorrow to understand better.

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

AlekseyTs · 2019-05-13T23:56:59Z

 ";

-            // https://github.com/dotnet/roslyn/issues/30677, https://github.com/dotnet/roslyn/issues/30673 - Expect no errors
+            // https://github.com/dotnet/roslyn/issues/30677 - Expect no errors


// #30677 - Expect no errors [](start = 12, length = 67)

It looks like this can be removed? #Closed

AlekseyTs · 2019-05-13T23:57:48Z

Done with review pass (iteration 2) #Closed

AlekseyTs · 2019-05-14T00:05:34Z

+
+                    for (int i = 0; i < typeArguments.Length; i++)
+                    {
+                        if (!typeArguments[i].Type.OriginalDefinition.Equals(typeParameters[i], TypeCompareKind.CLRSignatureCompareOptions))


typeParameters[i] [](start = 77, length = 17)

Consider calling equality of off that instead of typeArguments[i].Type.OriginalDefinition. I.e. switching sides, this way we know it will always call code for TypeParameters. #Closed

AlekseyTs · 2019-05-14T17:32:04Z

The iteration 3 LGTM, modulo the suggested tests and the remaining VB work. You might want to update the #35613 (comment) comment to reflect the current state of the PR more accurately. #Closed

jcouv · 2019-05-14T18:06:36Z

Thanks. Updated the original description to say that #30677 is now fixed.
I'll work on VB now and push to this same PR if that's ok. #Closed

jcouv · 2019-06-19T03:50:35Z

@AlekseyTs I'll stop by tomorrow for some tips. I spent some time today digging into the VB testing again, and I'm stumbling on the same issue I did a few weeks ago: I can't find how to make a substituted method with different type modifiers. The MethodSymbol.Construct in VB only takes type arguments (without modifiers).

Here's what I have so far for VB side. #Closed

AlekseyTs · 2019-06-19T18:24:12Z

For example:
In TypeSubstitution

        Public Shared Function Create(
            targetGenericDefinition As Symbol,
            params As ImmutableArray(Of TypeParameterSymbol),
            args As ImmutableArray(Of TypeWithModifiers),
            Optional allowAlphaRenamedTypeParametersAsArguments As Boolean = False
        ) As TypeSubstitution

then in NamedTypeSymbol:

        Friend Function Construct(substitution As TypeSubstitution) As NamedTypeSymbol
``` #Closed

jcouv · 2019-06-21T22:47:20Z

@AlekseyTs I'll stop by to discuss.
The VB test differs from the C# test in two regards:

in VB, Construct can only be applied to definitions
in VB, there may be a bug in comparisons with IsSameType ignoring custom modifiers #Closed

jcouv · 2019-06-24T14:31:57Z

@AlekseyTs I think this is ready for another look. Thanks #Closed

AlekseyTs · 2019-06-24T22:32:41Z

+                Dim typeParameters = type.OriginalDefinition.TypeParameters
+
+                For i = 0 To typeArguments.Length - 1
+                    If Not typeParameters(i).IsSameType(typeArguments(i).OriginalDefinition, TypeCompareKind.ConsiderEverything) Then


typeParameters(i).IsSameType(typeArguments(i).OriginalDefinition, TypeCompareKind.ConsiderEverything) [](start = 27, length = 101)

Why not use simple Equals ? #Closed

AlekseyTs · 2019-06-24T22:34:27Z

            _substitution = substitution
        End Sub

+        Protected Shared Function WasConstructedForModifiers(type As NamedTypeSymbol) As Boolean


WasConstructedForModifiers [](start = 34, length = 26)

Can this be an instance method instead? #Closed

AlekseyTs · 2019-06-24T22:35:37Z

+                Dim typeArguments = type.TypeArgumentsNoUseSiteDiagnostics
+                Dim typeParameters = type.OriginalDefinition.TypeParameters
+
+                For i = 0 To typeArguments.Length - 1


For i = 0 To typeArguments.Length - 1 [](start = 16, length = 37)

It feels like it would be better to iterate through the chain of TypeSubstitutions instead. #Closed

AlekseyTs · 2019-06-24T22:45:53Z

+                    Return False ' different definition
+                End If
+
+                Return t1.ContainingSymbol.ContainingType.IsSameType(t2.ContainingSymbol.ContainingType, compareKind)


ContainingType [](start = 43, length = 14)

This doesn't look right. Why re we skipping through the container? Can't that just give us a null reference? #Closed

AlekseyTs · 2019-06-24T22:51:51Z

Done with review pass (iteration 5) #Closed

jcouv · 2019-06-24T23:02:11Z

        End Sub
+
+        <WorkItem(30673, "https://github.com/dotnet/roslyn/issues/30673")>
+        <Fact>


Fact [](start = 9, length = 4)

Test with SubstitutedErrorTypeSymbol too #Closed

AlekseyTs · 2019-06-24T23:20:21Z

@@ -491,6 +509,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols

        Public Overrides Function GetHashCode() As Integer


GetHashCode [](start = 34, length = 11)

It looks like SubstitutedErrorType has similar issue. #Closed

AlekseyTs · 2019-06-25T14:11:06Z

Continue Do [](start = 28, length = 11)

It looks like this is going to cause an infinite loop, we are not advancing. #Closed

AlekseyTs · 2019-06-25T14:13:38Z

ContainingType [](start = 26, length = 14)

~~This doesn't look right, we should be comparing ContainingSymbol otherwise we will skip through a generic method.~~ I guess methods would be equal if containing types are. #Closed

AlekseyTs · 2019-06-25T14:22:26Z

Done with review pass (iteration 7) #Closed

AlekseyTs

LGTM (iteration 8)

jcouv · 2019-06-25T19:56:40Z

Thanks @AlekseyTs
@dotnet/roslyn-compiler for a second review. Thanks

chsienki

Conflicts: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

jcouv · 2019-06-25T23:31:33Z

Rebased/squashed on top of latest master and resolved conflict in test file.

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels May 9, 2019

jcouv added this to the 16.2.P1 milestone May 9, 2019

jcouv requested a review from a team as a code owner May 9, 2019 21:18

jcouv self-assigned this May 9, 2019

jcouv commented May 9, 2019

View reviewed changes

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 9, 2019

jcouv mentioned this pull request May 10, 2019

Decide nullable context for implicit outer type #35619

Open

jcouv force-pushed the hash-code branch from c585479 to b56db98 Compare May 10, 2019 01:41

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 10, 2019

AlekseyTs reviewed May 13, 2019

View reviewed changes

AlekseyTs reviewed May 14, 2019

View reviewed changes

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 14, 2019

jinujoseph modified the milestones: 16.2.P1, 16.2 Jun 9, 2019

jcouv force-pushed the hash-code branch from 5949926 to fe78619 Compare June 21, 2019 22:44

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 21, 2019

jcouv commented Jun 21, 2019

View reviewed changes

Comment thread src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Outdated

jcouv force-pushed the hash-code branch 2 times, most recently from af02a2e to 26f2ba9 Compare June 22, 2019 00:44

jcouv modified the milestones: 16.2, 16.2.P4 Jun 22, 2019

AlekseyTs reviewed Jun 24, 2019

View reviewed changes

jcouv commented Jun 24, 2019

View reviewed changes

AlekseyTs reviewed Jun 24, 2019

View reviewed changes

AlekseyTs reviewed Jun 25, 2019

View reviewed changes

AlekseyTs approved these changes Jun 25, 2019

View reviewed changes

chsienki approved these changes Jun 25, 2019

View reviewed changes

AlekseyTs mentioned this pull request Jun 25, 2019

Do not warn about an oblivious mismatch for an explicitly implemented interface. #36761

Merged

Fix GetHashCode on annotated type arguments

df12a7f

Conflicts: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

jcouv force-pushed the hash-code branch from 2d0ecb5 to df12a7f Compare June 25, 2019 23:30

AlekseyTs merged commit 8ae86d7 into dotnet:master Jun 26, 2019

jcouv deleted the hash-code branch June 26, 2019 02:33

AlekseyTs mentioned this pull request Jan 23, 2020

Substituted symbol equality #41123

Merged

		@@ -491,6 +509,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols

		Public Overrides Function GetHashCode() As Integer

Conversation

jcouv commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv May 9, 2019 • 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 May 13, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv May 14, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 10, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs May 13, 2019 • 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 May 13, 2019 • 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 May 13, 2019 • 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 May 14, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 13, 2019 • 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 May 13, 2019 • 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!

jcouv May 14, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 13, 2019 • 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 May 13, 2019 • 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 May 13, 2019 • 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!

jcouv May 14, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jcouv commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jcouv commented May 9, 2019 •

edited

Loading

jcouv May 9, 2019 •

edited

Loading

jcouv commented May 10, 2019 •

edited

Loading

AlekseyTs commented May 10, 2019 •

edited by jcouv

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs May 13, 2019 •

edited

Loading

AlekseyTs commented May 13, 2019 •

edited by jcouv

Loading

AlekseyTs May 14, 2019 •

edited

Loading

AlekseyTs commented May 14, 2019 •

edited

Loading

jcouv commented May 14, 2019 •

edited

Loading

jcouv commented Jun 19, 2019 •

edited

Loading

AlekseyTs commented Jun 19, 2019 •

edited

Loading

jcouv commented Jun 21, 2019 •

edited

Loading

jcouv commented Jun 24, 2019 •

edited

Loading

AlekseyTs Jun 24, 2019 •

edited

Loading

AlekseyTs Jun 24, 2019 •

edited

Loading

AlekseyTs Jun 24, 2019 •

edited

Loading

AlekseyTs Jun 24, 2019 •

edited

Loading

AlekseyTs commented Jun 24, 2019 •

edited by jcouv

Loading

jcouv Jun 24, 2019 •

edited

Loading

AlekseyTs Jun 24, 2019 •

edited

Loading

AlekseyTs Jun 25, 2019 •

edited

Loading

AlekseyTs Jun 25, 2019 •

edited

Loading

AlekseyTs commented Jun 25, 2019 •

edited

Loading