Substituted symbol equality by chsienki · Pull Request #41123 · dotnet/roslyn

chsienki · 2020-01-21T21:55:09Z

Third attempt at a sane way of doing this: (see #40939, #38408)

We simply defer to the substituted field + method equal implementations if we're comparing them, and defer back to the non-substituted versions for the hash code.

- Defer the equality of method/field symbol to the substituted symbol when compared - Make the hashcode of substituted method/field defer to the original definition

chsienki · 2020-01-21T23:33:10Z

@dotnet/roslyn-compiler for review

AlekseyTs · 2020-01-23T19:47:03Z

            return new PublicModel.FieldSymbol(this);
        }
+
+        public override bool Equals(Symbol other, TypeCompareKind compareKind)


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

If we override Equals, I would expect us to override GetHashCode as well. #Closed

It will just call down into the base though, so it seems redundant? #Resolved

It will just call down into the base though, so it seems redundant?

That will be informative for the reader and then future changes to Equals might require changes to GetHashCode too, which is easy to forget unless the methods next to each other.

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

AlekseyTs · 2020-01-23T19:47:54Z

            return new PublicModel.MethodSymbol(this);
        }
+
+        public override bool Equals(Symbol other, TypeCompareKind compareKind)


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

If we override Equals, I would expect us to override GetHashCode as well. #Closed

AlekseyTs · 2020-01-23T20:00:25Z

+            // the substituted type is considered equal (e.g. when ignoring nullability).
+            // We defer the hashcode to the original definition so it will match in those scenarios. 
+            // See https://github.com/dotnet/roslyn/issues/38195
+            return this.OriginalDefinition.GetHashCode();


this.OriginalDefinition.GetHashCode() [](start = 19, length = 37)

I think it is still valid to return Hash.Combine(_containingType, OriginalDefinition.GetHashCode());, unless TypeSymbol.Equals(_containingType, other.ContainingType, ALL_IGNORE_OPTIONS) is true #Closed

For performance, we can replace this condition for containing types with comparison of hash codes

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

But how would we know that in the hashcode function? We wouldn't have an other to compare against? If we put a substituted method symbol into a dictionary, then did a .Contains() with a regular symbol that would compare equal when ignoring nullability we would fail, even with a comparer that accounted for it. #Resolved

To clarify, the other in the condition is this.OriginalDefinition

In reply to: 370326741 [](ancestors = 370326741,370326121)

AlekseyTs · 2020-01-23T20:06:35Z

+            // the substituted type is considered equal (e.g. when ignoring nullability).
+            // We defer the hashcode to the original definition so it will match in those scenarios. 
+            // See https://github.com/dotnet/roslyn/issues/38195
+            return this.OriginalDefinition.GetHashCode();


return this.OriginalDefinition.GetHashCode(); [](start = 12, length = 45)

Same comment as for substituted fields, we could keep original calculation under some conditions. #Closed

AlekseyTs · 2020-01-23T20:35:56Z

+
+        [Fact]
+        [WorkItem(8195, "https://github.com/dotnet/roslyn/issues/38195")]
+        public void SemanticModel_SubstitutedEvent_Equality()


SemanticModel_SubstitutedEvent_Equality [](start = 20, length = 39)

It doesn't look like we made changes for events. Why does this test succeed? #Closed

EventSymbol already has a custom equality/hashcode implementation and SubsititutedEventSymbol doesn't override it.

(note: my previous PR attempted to go this route for method + field symbol, but it proved annoyingly complicated to get right in all cases) #Resolved

AlekseyTs · 2020-01-23T20:42:51Z

Do we need to make similar changes to VB, it doesn't have nullable annotations, but has custom modifiers. See similar change for types #35613.

AlekseyTs · 2020-01-23T20:43:49Z

Done with review pass (iteration 2) #Closed

chsienki · 2020-01-29T18:11:37Z

Yes, it turns out we're not correctly handling the custom modifier case for VB type comparisons. I took a look at this, but it turned out to be more complicated than expected: we're not actually respecting the type compare kind anywhere in VB symbol comparison, and it's not a simple change to fix it. Given that this isn't directly related to the issue we're trying to fix, I think we should create an issue and fix that in a separate PR.

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

jaredpar · 2020-01-29T19:22:23Z

Given that this isn't directly related to the issue we're trying to fix, I think we should create an issue and fix that in a separate PR.

Agree. I really want this fix to get into 16.5 sooner vs. later, as with all our other nullable changes, so we get customer validation on the change. Can take up the VB modopt one separately.

AlekseyTs · 2020-01-29T23:00:29Z

            if (code == 0)

We probably do not want to do this tweaking for our special case. Perhaps this logic should be moved into ComputeHashCode. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/SubstitutedMethodSymbol.cs:444 in c2e2d31. [](commit_id = c2e2d31, deletion_comment = False)

AlekseyTs · 2020-01-29T23:37:13Z

+            // (e.g, ignoring nullability) and want to retain the same hashcode. As such only make
+            // the containing type part of the hashcode when we know equality isn't possible
+            var containingHashCode = _containingType.GetHashCode();
+            if (containingHashCode != this.OriginalDefinition.ContainingType.GetHashCode())


if (containingHashCode != this.OriginalDefinition.ContainingType.GetHashCode()) [](start = 12, length = 79)

I think we should reverse the condition and return what we have. Otherwise, do what we did before. The if about type arguments below shouldn't be executed if this condition is false. #Closed

AlekseyTs · 2020-01-29T23:39:22Z

Done with review pass (iteration 3) #Closed

- Make SMS hashcode an optional int, rather than using zero to signify not initialized - Invert hashcode condition and exit early

AlekseyTs · 2020-01-29T23:57:08Z

 Microsoft Visual Studio Solution File, Format Version 12.00
-# Visual Studio 15
-VisualStudioVersion = 15.0.27102.0
+# Visual Studio Version 16


Are changes in this file intentional? #Closed

AlekseyTs · 2020-01-30T00:01:01Z

        private OverriddenOrHiddenMembersResult _lazyOverriddenOrHiddenMembers;

-        private int _hashCode; // computed on demand
+        private int? _hashCode; // computed on demand


int? [](start = 16, length = 4)

This probably makes the code that initializes this field not thread-safe. The assignment is no longer guaranteed to be atomic. I am not sure if the change is warranted, it is unlikely that definition's hash code is 0. We can even assert that in the ComputeHashCode. #Closed

AlekseyTs · 2020-01-30T00:03:01Z

        return code;

Consider incrementing if the value is 0. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/SubstitutedMethodSymbol.cs:395 in c2e2d31. [](commit_id = c2e2d31, deletion_comment = False)

AlekseyTs · 2020-01-30T00:05:09Z

-            int code = _hashCode;
-
-            if (code == 0)
+            if (!_hashCode.HasValue)


!_hashCode.HasValue [](start = 16, length = 19)

The access to properties is probably not thread safe. #Closed

AlekseyTs · 2020-01-30T00:06:18Z

-                }
-
-                _hashCode = code;
+                _hashCode = ComputeHashCode();


_hashCode = ComputeHashCode(); [](start = 16, length = 30)

This assignment is no longer atomic. I recommend reverting the type change and moving increment into the ComputeHashCode. #Closed

AlekseyTs · 2020-01-30T00:07:28Z

Done with review pass (iteration 4) #Closed

AlekseyTs

LGTM (iteration 7), assuming the tests are passing.

chsienki · 2020-01-30T21:58:51Z

Created #41319 to track VB custom modifiers comparison issue

cston · 2020-01-31T18:56:05Z

+            // If the containing type of the of the original definition is the same as our containing type
+            // it's possible that we will compare equal to the original definition under certain conditions 
+            // (e.g, ignoring nullability) and want to retain the same hashcode. As such, consider only
+            // the the original definition for the hashcode when we know equality is possible


the the [](start = 15, length = 7)

Typo.

cston · 2020-01-31T18:59:04Z

+            var containingHashCode = _containingType.GetHashCode();
+            if (containingHashCode == this.OriginalDefinition.ContainingType.GetHashCode())
+            {
+                return code;


return code [](start = 16, length = 11)

Any chance this returns 0?

The tweak to the hash code should probably be left in the caller.

But then we won't match the original definition hash code, which we have to do. If in the rare case it is 0 we'll just call CalculateHashCode each time rather than caching it, but it's pretty cheap as we are just going to call into the original definition and return.

333fred · 2020-01-31T21:17:30Z

+            code = Hash.Combine(containingHashCode, code);
+
+            // Unconstructed method may contain alpha-renamed type parameters while	
+            // may still be considered equal, we do not want to give different hashcode to such types.	


agocke · 2020-01-31T21:59:19Z

+        {
+            if (other is SubstitutedFieldSymbol sfs)
+            {
+                return sfs.Equals(this, compareKind);


Why did we swap the ordering of the Equals call here?

Because the substituted ones have special logic to compare against the non-substited. We've been through various iterations of solving this and this is sadly the simplest / easiest to reason about.

agocke · 2020-01-31T22:55:14Z

-            return Hash.Combine(_containingType, OriginalDefinition.GetHashCode());
+            var code = this.OriginalDefinition.GetHashCode();
+
+            // If the containing type of the of the original definition is the same as our containing type


Suggested change

// If the containing type of the of the original definition is the same as our containing type

// If the containing type of the original definition is the same as our containing type

agocke · 2020-01-31T22:55:52Z

-            //
-            //  In short - we are not interested in the type arguments of unconstructed methods.
+
+            // If the containing type of the of the original definition is the same as our containing type


Suggested change

// If the containing type of the of the original definition is the same as our containing type

// If the containing type of the original definition is the same as our containing type

agocke · 2020-01-31T22:57:43Z

            }

+            // 0 means that hashcode is not initialized. 
+            // in a case we really get 0 for the hashcode, tweak it by +1


* dotnet/master: (918 commits) Pure null test on unconstrained type (dotnet#41384) Fix file headers, bootsrap build issues. Make `elementLocations` accept an array of nullable locations in the public api. Learn from non-null tests on as operator (dotnet#41419) Use Microsoft.NET.Sdk.WindowsDesktop for XAML projects (dotnet#40234) Address feedback. Introduce `GetRequiredBinder`. Add missing `NotNullWhen` annotations. Annotate more of the binder Add version of zlib library to deterministic build inputs (dotnet#41385) [master] Update dependencies from dotnet/arcade (dotnet#41354) Simplify Simplify Do not simplify interpolation expressions on implicit receivers. Fix local function crash + add tests Substituted symbol equality (dotnet#41123) Fix test failures Rename from IncludeNonNullableReferenceTypeModifier to IncludeNotNullableReferenceTypeModifier (dotnet#41332) Set TEMP environment variable to $TempDir on CI machines (dotnet#41361) Document placeholders ...

This reverts commit 2512d80.

The changes made in dotnet#41123 fix the GetHashCode for SubstitutedMethodSymbol for more nullability scenarios, but end up hashing all generic methods with a non-generic containing type into the same bucket. This can cause large performance regressions for the compiler. This change ensures that substituted symbols which have substitutions equivalent to their original methods' type parameter continue to be equal, but substituted methods with different type substitutions are not considered equal.

The changes made in #41123 fix the GetHashCode for SubstitutedMethodSymbol for more nullability scenarios, but end up hashing all generic methods with a non-generic containing type into the same bucket. This can cause large performance regressions for the compiler. This change ensures that substituted symbols which have substitutions equivalent to their original methods' type parameter continue to be equal, but substituted methods with different type substitutions are not considered equal.

chsienki added 2 commits January 21, 2020 13:44

MethodSymbol and FieldSymbol equality fixes:

3b836f9

- Defer the equality of method/field symbol to the substituted symbol when compared - Make the hashcode of substituted method/field defer to the original definition

Cleanup tests

eeb0b5d

chsienki requested a review from a team as a code owner January 21, 2020 21:55

chsienki added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jan 21, 2020

chsienki changed the title ~~Substituted symbol equality2~~ Substituted symbol equality Jan 21, 2020

AlekseyTs reviewed Jan 23, 2020

View reviewed changes

Restore more detailed hashcodes when appropriate

c2e2d31

AlekseyTs reviewed Jan 29, 2020

View reviewed changes

PR Feedback:

75060b8

- Make SMS hashcode an optional int, rather than using zero to signify not initialized - Invert hashcode condition and exit early

AlekseyTs reviewed Jan 29, 2020

View reviewed changes

AlekseyTs reviewed Jan 30, 2020

View reviewed changes

chsienki added 2 commits January 30, 2020 09:47

Restore 0 based empty check

e3c1d46

Undo sln changes

61bd2d7

Undo sln changes

61901ab

AlekseyTs approved these changes Jan 30, 2020

View reviewed changes

cston reviewed Jan 31, 2020

View reviewed changes

333fred reviewed Jan 31, 2020

View reviewed changes

333fred approved these changes Jan 31, 2020

View reviewed changes

agocke reviewed Jan 31, 2020

View reviewed changes

chsienki added 2 commits February 3, 2020 11:04

PR feedback

67c9fc3

Fix typo

0bd43a8

cston approved these changes Feb 3, 2020

View reviewed changes

chsienki merged commit 2512d80 into dotnet:master Feb 3, 2020

This was referenced Feb 3, 2020

Incorrect IDE0044 when NRTs are enabled, the class is generic, and the field is only set by a non-this reference #38921

Closed

Field symbol from symbol tree and operation tree fail Equals check in presence of nullable enable #38195

Open

chsienki mentioned this pull request Feb 12, 2020

StackOverflowException in OOP #41598

Closed

agocke added a commit to agocke/roslyn that referenced this pull request Feb 13, 2020

Revert "Substituted symbol equality (dotnet#41123)"

91ae58d

This reverts commit 2512d80.

agocke mentioned this pull request Feb 13, 2020

Improve SubstitutedMethodSymbol.GetHashCode #41674

Merged

	// If the containing type of the of the original definition is the same as our containing type
	// If the containing type of the original definition is the same as our containing type

Conversation

chsienki commented Jan 21, 2020

Uh oh!

chsienki commented Jan 21, 2020

Uh oh!

AlekseyTs Jan 23, 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!

chsienki Jan 23, 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 Jan 23, 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 Jan 23, 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!

chsienki Jan 23, 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 Jan 23, 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 Jan 23, 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!

chsienki Jan 23, 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 Jan 23, 2020

Uh oh!

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

Uh oh!

Uh oh!

chsienki commented Jan 29, 2020

Uh oh!

jaredpar commented Jan 29, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs Jan 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 Jan 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

AlekseyTs Jan 23, 2020 •

edited

Loading

chsienki Jan 23, 2020 •

edited

Loading

AlekseyTs Jan 23, 2020 •

edited

Loading

AlekseyTs Jan 23, 2020 •

edited

Loading

chsienki Jan 23, 2020 •

edited

Loading

AlekseyTs Jan 23, 2020 •

edited

Loading

AlekseyTs Jan 23, 2020 •

edited

Loading

chsienki Jan 23, 2020 •

edited

Loading

AlekseyTs commented Jan 23, 2020 •

edited

Loading

AlekseyTs commented Jan 29, 2020 •

edited

Loading

AlekseyTs Jan 29, 2020 •

edited

Loading

AlekseyTs commented Jan 29, 2020 •

edited

Loading

AlekseyTs Jan 29, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs commented Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs commented Jan 30, 2020 •

edited

Loading

chsienki commented Jan 30, 2020 •

edited

Loading

cston Jan 31, 2020 •

edited

Loading