LibraryImportGenerator, .NET 7: possible error in the 'Equals' method implementation

[VasilievSerg]

Description

Code:

public bool Equals(IncrementalStubGenerationContext? other)
{
  return other is not null
               && StubEnvironment.AreCompilationSettingsEqual(Environment, other.Environment)
               && SignatureContext.Equals(other.SignatureContext)
               && ContainingSyntaxContext.Equals(other.ContainingSyntaxContext)
               && StubMethodSyntaxTemplate.Equals(other.StubMethodSyntaxTemplate)
               && LibraryImportData.Equals(other.LibraryImportData)
               && DiagnosticLocation.Equals(DiagnosticLocation)               // <=
               && ForwardedAttributes.SequenceEqual(other.ForwardedAttributes, (IEqualityComparer<AttributeSyntax>)SyntaxEquivalentComparer.Instance)
               && GeneratorFactoryKey.Equals(other.GeneratorFactoryKey)
               && Diagnostics.SequenceEqual(other.Diagnostics);
            }

Link to the sources.

Looks suspicious that DiagnosticLocation compares to itself.

Reproduction Steps

Expected behavior

Actual behavior

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Code:

public bool Equals(IncrementalStubGenerationContext? other)
{
  return other is not null
               && StubEnvironment.AreCompilationSettingsEqual(Environment, other.Environment)
               && SignatureContext.Equals(other.SignatureContext)
               && ContainingSyntaxContext.Equals(other.ContainingSyntaxContext)
               && StubMethodSyntaxTemplate.Equals(other.StubMethodSyntaxTemplate)
               && LibraryImportData.Equals(other.LibraryImportData)
               && DiagnosticLocation.Equals(DiagnosticLocation)               // <=
               && ForwardedAttributes.SequenceEqual(other.ForwardedAttributes, (IEqualityComparer<AttributeSyntax>)SyntaxEquivalentComparer.Instance)
               && GeneratorFactoryKey.Equals(other.GeneratorFactoryKey)
               && Diagnostics.SequenceEqual(other.Diagnostics);
            }

Link to the sources.

Looks suspicious that DiagnosticLocation compares to itself.

Reproduction Steps

Expected behavior

Actual behavior

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	VasilievSerg
Assignees:	-
Labels:	area-System.Runtime.InteropServices, untriaged
Milestone:	-

[AaronRobinsonMSFT]

@VasilievSerg Agreed, this does look odd. Care to submit a PR for adding the other.?

@jkoritzinsky Are we missing a test for this or is this one of those checks that doesn't matter often enough in practice?

[jkoritzinsky]

I fixed this in .NET 8 and found that we did have a test validating the behavior. Unfortunately, the test is wrong in .NET 7 and didn't catch it.

The fix and the updated test were part of https://github.com/dotnet/runtime/pull/76474/files#diff-e5ffd09434c9daa4ab5234b83ae383b344c4c20d15a93216b93d358226a10586

In particular, the updates to the AppendingUnrelatedSource_DoesNotRegenerateSource test capture the result of fixing this bug.

[jkoritzinsky]

Updating the milestone to .NET 7.0.X as this was already fixed in .NET 8.

[VasilievSerg]

I found two more similar places:

• JSImportGenerator.cs (link);
• JSExportGenerator.cs (link).

[jkoritzinsky]

The fix for those was also included in #76474 for .NET 8. It might be worthwhile backporting these fixes to .NET 7 (or all of #76474, which I mentioned in that PR).

[AaronRobinsonMSFT]

@jkoritzinsky What is the fallout for this? How are customers impacted by this bug?

[jkoritzinsky]

Diagnostics from the source generator would be reported in the wrong location if the user changes the file above the LibraryImport. This can affect both the IDE experience and an inner-loop command line build experience as the compiler server also has the incremental generator support enabled.

[AaronRobinsonMSFT]

I see. The referenced PR isn't too big. We can try porting this now or wait until we hear from people. Since it doesn't impact the actual generated code, I don't have a strong opinion either way.

[AaronRobinsonMSFT]

This doesn't see to be actionable any longer. Closing.