Implement IDiscardSymbol.Equals and GetHashCode by gafter · Pull Request #26720 · dotnet/roslyn

gafter · 2018-05-08T16:38:23Z

@jcouv @AlekseyTs Please review this tiny bug fix.
/cc @dotnet/roslyn-compiler

cston · 2018-05-08T16:42:52Z

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

@@ -7,16 +7,14 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
 {
    internal class DiscardSymbol : Symbol, IDiscardSymbol


sealed #Resolved

AlekseyTs · 2018-05-08T17:27:39Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+            foreach (var discard in discards)
+            {
+                var symbol = (IDiscardSymbol)model.GetSymbolInfo(discard).Symbol;
+                set.Add(symbol);


set.Add(symbol); [](start = 16, length = 16)

It would be good to assert that all the symbols are distinct reference-wise. #Closed

Including the symbol0.

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

Such an assertion would not be correct. #ByDesign

These symbols are not specified to be distinct reference-wise, and adding a test constraining the implementation in that way would be counterproductive. #ByDesign

I argue that this is important to confirm the value of this test. Verifying equality of an instance to itself is not the goal of this test. Quite the opposite and we should clearly assert this fact.

In reply to: 186806231 [](ancestors = 186806231,186805636)

These symbols are not specified to be distinct reference-wise, and adding a test constraining the implementation in that way would be counterproductive.

Quite the opposite. It will make it easy to confirm that the test is testing (and will continue doing so in the future) scenario it is supposed to test.

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

The scenario the test is supposed to test is not the identity of the symbols, but their equality. And it does that, in both positive and negative scenarios. It is not intended to enforce that the discards are reference unequal, and in fact I can easily imagine that that would change in the future. The intended behavior is the behavior that is tested.

In reply to: 186883163 [](ancestors = 186883163,186881434)

AlekseyTs · 2018-05-08T17:30:59Z

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

        public override void Accept(CSharpSymbolVisitor visitor) => visitor.VisitDiscard(this);
        public override TResult Accept<TResult>(CSharpSymbolVisitor<TResult> visitor) => visitor.VisitDiscard(this);
+
+        public override bool Equals(object obj) => obj is DiscardSymbol other && this.Type.Equals(other.Type);


obj is DiscardSymbol other && this.Type.Equals(other.Type) [](start = 51, length = 58)

I think there a should be a short-cut when the same instance is compared to itself #Closed

I added it, but then removed it in a later iteration. It isn't much of an optimization, as the type equality already has that shortcut one level down. I'd rather not use identity of discard symbols in any way in the implementation. #Resolved

AlekseyTs · 2018-05-08T17:32:03Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+
+            var discards = GetDiscardIdentifiers(tree).ToArray();
+            Assert.Equal(4, discards.Length);
+            var symbol0 = (IDiscardSymbol)model.GetSymbolInfo(discards[0]).Symbol;


symbol0 [](start = 16, length = 7)

It would be good to assert that symbol0 equals to itself #Closed

AlekseyTs · 2018-05-08T17:32:18Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+                set.Add(symbol);
+                Assert.Equal(SymbolKind.Discard, symbol.Kind);
+                Assert.Equal("int _", symbol.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat));
+                Assert.Equal(symbol0, symbol);


Assert.Equal(symbol0, symbol); [](start = 16, length = 30)

It would be good to assert that symbol equals to itself #Closed

AlekseyTs · 2018-05-08T17:35:45Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+                if (discard == discards[0])
+                {
+                    Assert.Equal("double _", symbol.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat));
+                    Assert.Equal(symbol0, symbol);


Assert.Equal(symbol0, symbol); [](start = 20, length = 30)

Consider adding Assert.NotSame(symbol0, symbol) #Closed

These symbols are not required to be reference-distinct. #Closed

These symbols are not required to be reference-distinct.

But it is required to confirm that the test does what you assume it does. All assumptions should be verified

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

The API does not require that, and I would prefer it not be enforced.

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

I am not assuming it produces distinct objects. I am testing that the equality and hash code of the objects it does produce satisfy the requirements of the api.

In reply to: 186883547 [](ancestors = 186883547,186881725)

AlekseyTs · 2018-05-08T17:36:45Z

Done with review pass (iteration 1) #Closed

AlekseyTs · 2018-05-08T17:38:21Z

Please add explicit asserts for equality of GetHashCode for equal objects #Closed

AlekseyTs

The tests should be adjusted, or a new test should be added so that it was clearly verified that distinct instances (reference-wise) of IDiscardSymbol are equal when this is appropriate and that they have equal GetHashCode. Without that, there is no guarantee that this scenario is covered and will remain to be covered.

gafter · 2018-05-08T22:24:29Z

The test should be adjusted, or a new test should be added so that it was clearly verified that distinct instances (reference-wise) of IDiscardSymbol are equal when this is appropriate and that they have equal GetHashCode. Without that, there is no guarantee that this scenario is covered and will remain to be covered.

This test checks that all returned discard symbols that should compare the same do so, whether they are reference equals or not. A test that requires them to be reference unequals would be requiring things that are not intended to be required by this shape of the API. Reference distinctness is not part of the contract, and there is no guarantee that the compiler APIs could ever be used to produce distinct but equals discard symbols. #Resolved

AlekseyTs · 2018-05-08T22:29:59Z

Reference equality is not part of the contract.

I cannot verify that the test coverage is adequate. We are testing "value" equality, there must be a test that clearly test equality of distinct symbols. This is part of the contract - distinct symbols with the same Type must be equal. That should be clearly tested. At the moment this is not the case. #Closed

AlekseyTs · 2018-05-08T22:31:57Z

Removing discard symbol identity shortcut doesn't change anything regarding the requirements for tests. We are not testing specific implementation, these tests should catch regressions as well. #Closed

Tests added for behavior in the case of reference inequality.

AlekseyTs · 2018-05-09T17:40:19Z

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

+        /// Produce a fresh discard symbol for testing.
+        /// </summary>
+        internal static DiscardSymbol CreateForTest(ITypeSymbol type) => new DiscardSymbol((TypeSymbol)type);



The entire type is internal, it looks like using constructor would be sufficient. #ByDesign

AlekseyTs · 2018-05-09T17:44:25Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+
+                // Test to show that reference-unequal discards are equal by type.
+                IDiscardSymbol symbolClone = DiscardSymbol.CreateForTest(symbol.Type);
+                Assert.True(symbol != (object)symbolClone);


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

Please use Assert.NotSame method instead. In general, the use of Assert.True is discouraged when more specialized methods exist because they provide more valuable information in the log when the assert fails. #Closed

AlekseyTs

Please add an explicit test that verifies equality/hashcode for symbols with reference-distinct, but equal underlying type symbols. It would also be good to adjust existing test to make it explicit that it targets scenario when the underlying types are reverence identical.

AlekseyTs · 2018-05-09T17:55:44Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+                Assert.Equal("int _", symbolClone.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat));
+                Assert.Equal(symbol.Type, symbolClone.Type);
+                Assert.Equal(symbol0, symbolClone);
+                Assert.Equal(symbol, symbolClone);


Assert.Equal(symbol, symbolClone) [](start = 16, length = 33)

Consider adding an assert that the underlying types are reference identical, i.e. Assert.Same. #Closed

…tint symbols This is not part of the compiler's contract, so it may well break as the compiler evolves.

Tests added as requested.

jcouv

LGTM Thanks

AlekseyTs

Please do not merge this PR until I can confirm that the test coverage is adequate.

gafter · 2018-05-09T20:15:51Z

test windows_debug_vs-integration_prtest please

obsolete

AlekseyTs

LGTM (iteration 5)

* dotnet/master: (732 commits) Disable procdump on Spanish runs use named args. Exclude System.Diagnostics.DiagnosticSource from PortableFacades Exclude System.Net.Http from PortableFacades Align the implementations of IAssemblySymbol.GivesAccessTo (#26727) Fixed exception message in GetSemanticModel (#26659) Implement IDiscardSymbol.Equals and GetHashCode (#26720) Produce correct diagnostics on implicit operator conversions for stackalloc expressions (#26248) Remove special case handling of CreateDiagnosticProviderAndFixer throwing an exception Add verbose logging to DevDivInsertionFiles if you want it Exclude Microsoft.Composition as something we build a dependency map for Fix the message for Make Field Readonly Update LangVersion to 7.3 (#26698) fix extension deployment and add explainatory comment Add TaskExtensions to compiler package Add System.Diagnostics.DiagnosticSource to SignToolData.json Include missing facade: System.Diagnostics.DiagnosticSource Use default expressions. Remove forwarding method. Revert project chnage. ...

Implement IDiscardSymbol.Equals and GetHashCode

491978c

Fixes dotnet#25829

gafter added Bug Area-Compilers Feature - Tuples Tuples labels May 8, 2018

gafter added this to the 15.8 milestone May 8, 2018

gafter self-assigned this May 8, 2018

gafter requested review from AlekseyTs and jcouv May 8, 2018 16:38

gafter requested a review from a team as a code owner May 8, 2018 16:38

cston reviewed May 8, 2018

View reviewed changes

cston approved these changes May 8, 2018

View reviewed changes

AlekseyTs reviewed May 8, 2018

View reviewed changes

Minor changes per code review.

9e3c503

AlekseyTs previously requested changes May 8, 2018

View reviewed changes

Remove discard symbol identity shortcut.

856a227

Add test to show equality for reference-distinct discard symbols.

3e28313

AlekseyTs reviewed May 9, 2018

View reviewed changes

AlekseyTs previously requested changes May 9, 2018

View reviewed changes

AlekseyTs reviewed May 9, 2018

View reviewed changes

Add a test asserting that distinct constructions in source create dis…

27085ad

…tint symbols This is not part of the compiler's contract, so it may well break as the compiler evolves.

jcouv approved these changes May 9, 2018

View reviewed changes

AlekseyTs previously requested changes May 9, 2018

View reviewed changes

AlekseyTs approved these changes May 9, 2018

View reviewed changes

gafter merged commit bfe982d into dotnet:master May 10, 2018

ivanbasov mentioned this pull request Jun 25, 2018

GetSemanticSymbols does not support discard expressions #26730

Closed

		@@ -7,16 +7,14 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
		{
		internal class DiscardSymbol : Symbol, IDiscardSymbol

Conversation

gafter commented May 8, 2018

Uh oh!

cston May 8, 2018 • edited by gafter 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 8, 2018 • 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 8, 2018

Choose a reason for hiding this comment

Uh oh!

gafter May 8, 2018 • 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 May 8, 2018 • 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 8, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2018

Choose a reason for hiding this comment

Uh oh!

gafter May 8, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2018 • 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 May 8, 2018 • 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 8, 2018 • 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 8, 2018 • 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 8, 2018 • 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 May 8, 2018 • edited by AlekseyTs 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 8, 2018

Choose a reason for hiding this comment

Uh oh!

gafter May 8, 2018

Choose a reason for hiding this comment

Uh oh!

gafter May 8, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs left a comment • 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 commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

cston May 8, 2018 •

edited by gafter

Loading

AlekseyTs May 8, 2018 •

edited

Loading

gafter May 8, 2018 •

edited

Loading

gafter May 8, 2018 •

edited

Loading

AlekseyTs May 8, 2018 •

edited

Loading

gafter May 8, 2018 •

edited

Loading

AlekseyTs May 8, 2018 •

edited

Loading

AlekseyTs May 8, 2018 •

edited

Loading

AlekseyTs May 8, 2018 •

edited

Loading

gafter May 8, 2018 •

edited by AlekseyTs

Loading

AlekseyTs commented May 8, 2018 •

edited

Loading

AlekseyTs commented May 8, 2018 •

edited

Loading

AlekseyTs left a comment •

edited

Loading

gafter commented May 8, 2018 •

edited

Loading

AlekseyTs commented May 8, 2018 •

edited

Loading

AlekseyTs commented May 8, 2018 •

edited

Loading

AlekseyTs May 9, 2018 •

edited by gafter

Loading

AlekseyTs May 9, 2018 •

edited

Loading

AlekseyTs May 9, 2018 •

edited

Loading