Fix regression of LambdaSymbol.GetHashCode by Youssef1313 · Pull Request #58247 · dotnet/roslyn

Youssef1313 · 2021-12-10T07:03:40Z

cston · 2021-12-10T07:26:34Z

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

        public override int GetHashCode()
        {
-            return syntaxReferenceOpt.GetHashCode();
+            return Syntax.GetHashCode();


Syntax

Thanks @Youssef1313.

This looks distinct from Equals() which is checking the fields of syntaxReferenceOpt. Should Equals() be reverted as well, to check lambda.Syntax == Syntax instead of checking syntaxReferenceOpt?

@cston I think both should work the same. I'll go with lambda.Syntax == Syntax. It's more clear, and is the original behavior before the lambda improvements work.

roslyn/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs

Line 388 in 4463994

&& lambda._syntax == _syntax

cston · 2021-12-10T17:11:13Z

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

        public override int GetHashCode()
        {
-            return syntaxReferenceOpt.GetHashCode();
+            return Syntax.GetHashCode();


Syntax.GetHashCode();

Is Syntax.GetHashCode() guaranteed to return the same value each time given that Syntax is implemented as syntaxReferenceOpt.GetSyntax()?

If we want to use the Syntax for GetHashCode() and Equals(), then LambdaSymbol should probably hold onto the SyntaxNode in a readonly field instead. Otherwise GetHashCode() and Equals() should rely on the fields of syntaxReferenceOpt rather than syntaxReferenceOpt.GetSyntax().

For the latter approach, perhaps:

public override int GetHashCode() { return Hash.Combine( syntaxReferenceOpt.SyntaxTree.GetHashCode(), syntaxReferenceOpt.Span.GetHashCode()); }

@cston From the unit test, it seems to be working as expected. But I'm okay with either holding Syntax as a _syntax field, or implementing it using SyntaxReference.SyntaxTree|Span.

AlekseyTs · 2021-12-10T17:40:45Z

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

        public override int GetHashCode()
        {
-            return syntaxReferenceOpt.GetHashCode();
+            return Syntax.GetHashCode();


Syntax.GetHashCode();

I assume this is where the root cause of the problem was. SyntaxReference uses reference equality, therefore distinct syntax references almost always have different hash code. If this is correct, then I think we should simply align this implementation with the original implementation of Equals. I.e. combine hash codes of the tree and span and revert changes to the Equals. #Closed

@AlekseyTs It was exactly my intention to keep the original implementation. The changes that used the tree and the span along the GetHashCode change were all introduced in #52178. This simply reverts this small portion of #52178. I'm okay with any of the suggested approaches by you or @cston

I just wanted to point out to #52178 since this is what took me this way.

It was exactly my intention to keep the original implementation.

When I said "original", I meant "original" in context of this PR. And the suggestion to revert Equals is also in context of changes made in this PR.

To clarify. I think that we shouldn't change Equals in this PR and should simply align implementation of GetHashCode with what Equals is doing.

@AlekseyTs Thanks. Fixed.

My initial thought was to keep the implementation that has been working for years, but I can also see this is not necessarily correct (ie, it may have changed for a good reason).

Though for this PR, it's likely both implementations will work equivalently anyway, so keeping Equals as-is and do minimal change in GetHashCode does make more sense.

Thanks again :)

AlekseyTs · 2021-12-10T17:51:04Z

Done with review pass (commit 2)

AlekseyTs · 2021-12-10T18:12:43Z

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

        public override int GetHashCode()
        {
-            return syntaxReferenceOpt.GetHashCode();
+            return HashCode.Combine(syntaxReferenceOpt.SyntaxTree, syntaxReferenceOpt.Span);


HashCode.Combine

It looks like we standardize on Hash.Combine. Also, is this call going to box the span? #Closed

AlekseyTs · 2021-12-10T18:14:14Z

Done with review pass (commit 3).

cston

Thanks @Youssef1313!

AlekseyTs

LGTM (commit 4)

cston · 2021-12-10T20:31:52Z

It looks like there are related test failures with distinct LambdaSymbol instances considered equal in error scenarios.

For instance, GetSemanticInfoTests.LookupFromRangeVariableAfterFromClause() includes the following

var q = from i in new int[] { 4, 5 } where /*pos*/

which results in LambdaSymbols for the where clause and the missing select clause, both with empty Spans of [124..124), so the syntaxReferenceOpt fields are identical, and LambdaSymbol.Equals() returns true.

The simplest solution may be to add back the private readonly SyntaxNode _syntax; field from before #52178, and revert Equals() to use lambda._syntax == _syntax and revert GetHashCode() to use _syntax.GetHashCode().

AlekseyTs · 2021-12-10T22:09:03Z

The simplest solution may be to add back the private readonly SyntaxNode _syntax; field from before #52178, and revert Equals() to use lambda._syntax == _syntax and revert GetHashCode() to use _syntax.GetHashCode().

I do not think going back to an extra field is necessary. The correctness problem in the Equals that is revealed by the failures can be addressed by comparing underlying syntax nodes. Leaving GatHashCode as it is in commit 4 looks safe to me.

cston · 2021-12-10T22:42:41Z

Leaving GatHashCode as it is in commit 4 looks safe to me.

Let's add back the _syntax field from before #52178 and let's revert Equals() and GetHashCode() to use _syntax, so that both methods are using the SyntaxNode rather than different state, and so it is clear we've reverted the behavior.

@Youssef1313, please add back private readonly SyntaxNode _syntax; and set the field in the constructor. The field should be returned from the Syntax property, and used in Equals() and GetHashCode(). If you'd prefer that I push a commit to your PR, let me know. Thanks.

cston · 2021-12-11T06:19:08Z

    internal SyntaxNode Syntax => syntaxReferenceOpt.GetSyntax();

Can the Syntax property be removed and the use-sites updated to use _syntax instead. (This property was only introduced because the _syntax field was removed previously.)

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs:268 in a8122f7. [](commit_id = a8122f7, deletion_comment = False)

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs

cston · 2021-12-13T20:38:34Z

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs

+            var syntaxRefs = lambdas.SelectAsArray(l => l.SyntaxRef);
+            Assert.Equal(syntaxRefs[0].Span, syntaxRefs[1].Span);
+            Assert.NotEqual(lambdas[0], lambdas[1]);
+        }


@Youssef1313, it looks like a couple of namespaces are missing:

using System.Collections.Immutable; using Microsoft.CodeAnalysis.Operations;

That's what happens when I edit via GitHub directly 😄

AlekseyTs · 2021-12-13T21:33:59Z

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs

+            var lambdas = operation.Descendants().OfType<AnonymousFunctionOperation>().
+                Select(op => op.Symbol.GetSymbol<LambdaSymbol>()).ToImmutableArray();
+            Assert.Equal(2, lambdas.Length);
+            var syntaxRefs = lambdas.SelectAsArray(l => l.SyntaxRef);


SelectAsArray

It looks like we could do without building this array. #Closed

AlekseyTs

LGTM (commit 9)

AlekseyTs · 2021-12-14T16:32:05Z

@Youssef1313 Thank you for the contribution.

…rations * upstream/main: (87 commits) Add support for nullable analysis in interpolated string handler constructors (dotnet#57780) Record list-patterns and newlines in interpolations as done (dotnet#58250) Swithc to acquiring the component model on a BG thread. Fix failure to propagate cancellation token [main] Update dependencies from dotnet/arcade (dotnet#58327) Change PROTOTYPE to issue reference (dotnet#58336) Resolve PROTOTYPE comments in param null checking (dotnet#58324) Address various minor param-nullchecking issues (dotnet#58321) Nullable enable GetTypeByMetadataName (dotnet#58317) Support CodeClass2.Parts returning parts in source generated files Allow the FileCodeModel.Parent to be null Ensure the CodeModel tests are using VisualStudioWorkspaceImpl Fix the bad words in TestDeepAlternation Fix regression in Equals/GetHashCode of LambdaSymbol. (dotnet#58247) Support "Enable nullable reference types" from disable keyword Support "Enable nullable reference types" from restore keyword Support "Enable nullable reference types" on the entire directive Add comment Remove descriptor Update src/Workspaces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.cs ...

Fix regression of LambdaSymbol.GetHashCode

b82c5b0

Youssef1313 requested a review from a team as a code owner December 10, 2021 07:03

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels Dec 10, 2021

cston reviewed Dec 10, 2021

View reviewed changes

Compare Syntax in LambdaSymbol.Equals

f0550e6

Youssef1313 mentioned this pull request Dec 10, 2021

Add assertions for GetHashCode #58248

Closed

cston reviewed Dec 10, 2021

View reviewed changes

AlekseyTs reviewed Dec 10, 2021

View reviewed changes

Update LambdaSymbol.cs

aee37cd

AlekseyTs reviewed Dec 10, 2021

View reviewed changes

Update LambdaSymbol.cs

6c52510

cston approved these changes Dec 10, 2021

View reviewed changes

AlekseyTs approved these changes Dec 10, 2021

View reviewed changes

AlekseyTs enabled auto-merge (squash) December 10, 2021 18:35

cston disabled auto-merge December 10, 2021 21:25

Address feedback

a8122f7

Remove Syntax property

defc55c

cston self-requested a review December 12, 2021 12:07

cston reviewed Dec 13, 2021

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs Show resolved Hide resolved

Add test

7242877

cston reviewed Dec 13, 2021

View reviewed changes

Add missing using directives

87e6d42

AlekseyTs reviewed Dec 13, 2021

View reviewed changes

Update SymbolEqualityTests.cs

aab365c

cston approved these changes Dec 14, 2021

View reviewed changes

AlekseyTs approved these changes Dec 14, 2021

View reviewed changes

AlekseyTs merged commit 2cd1e6f into dotnet:main Dec 14, 2021

ghost added this to the Next milestone Dec 14, 2021

Youssef1313 deleted the gethashcode-fix branch December 14, 2021 18:50

Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022

Youssef1313 mentioned this pull request Nov 12, 2022

The equal symbols have a different hash code #65393

Closed

Conversation

Youssef1313 commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Dec 10, 2021 • 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 Dec 10, 2021 • 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 Dec 10, 2021 • 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 commented Dec 10, 2021

Uh oh!

AlekseyTs Dec 10, 2021 • 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 Dec 10, 2021

Uh oh!

cston left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Dec 10, 2021

Uh oh!

cston commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 13, 2021 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Youssef1313 commented Dec 10, 2021 •

edited

Loading

cston Dec 10, 2021 •

edited

Loading

AlekseyTs Dec 10, 2021 •

edited

Loading

AlekseyTs Dec 10, 2021 •

edited

Loading

AlekseyTs Dec 10, 2021 •

edited

Loading

cston commented Dec 10, 2021 •

edited

Loading

cston commented Dec 10, 2021 •

edited

Loading

cston commented Dec 11, 2021 •

edited

Loading

AlekseyTs Dec 13, 2021 •

edited

Loading