dotnet / roslyn-analyzers

MIT License
1.6k stars 468 forks source link

CA1859 analyzer issue #7348

Open WeihanLi opened 9 months ago

WeihanLi commented 9 months ago

Describe the bug

CA1859 analyzer in .NET 9 SDK may be too aggressive, think it should not report when interface default implemented method used

Apologize if this is not the correct place, please help move the issue to the Roslyn or runtime repo if needed, thanks very much.

To Reproduce

sample code as below:

public interface IReference
{
    string Reference { get; }

    string ReferenceWithSchema =>
        $"{ReferenceResolverHelper.ReferenceTypeSchemaCache[ReferenceType]}: {Reference}";

    ReferenceType ReferenceType { get; }
}

[System.Diagnostics.DebuggerDisplay("nuget: {Reference}")]
public sealed record NuGetReference(string PackageId, NuGetVersion? PackageVersion) : IReference
{
    public NuGetReference(string packageId, string? packageVersion)
        : this(packageId, string.IsNullOrEmpty(packageVersion) ? null : NuGetVersion.Parse(packageVersion))
    {
    }

    public string Reference => PackageVersion is null
        ? $"{PackageId}"
        : $"{PackageId}, {PackageVersion}";

    public ReferenceType ReferenceType => ReferenceType.NuGetPackage;
}

IReference reference = new NuGetReference("WeihanLi.Common", "1.0.60");
Assert.Equal($"nuget: {reference.Reference}", reference.ReferenceWithSchema);

It works fine in .NET 8, while when updated to .NET 9 SDK, it reports

A full runnable project here: https://github.com/WeihanLi/dotnet-exec/tree/19b9865319c5b2e21a46c55ad8ff4dd7bd3e609d

Exceptions (if any)

error CA1859: Change type of variable 'reference' from 'ReferenceResolver.IReference' to 'ReferenceResolver.NuGetReference' for improved performance 
(https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1859)

Further technical details

.NET SDK:
 Version:           9.0.100-preview.1.24101.2
 Commit:            6bbd460f4d
 Workload version:  9.0.100-manifests.c840f51f

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.26058
 OS Platform: Windows
 RID:         win-x64
 Base Path:   C:\Program Files\dotnet\sdk\9.0.100-preview.1.24101.2\

.NET workloads installed:
There are no installed workloads to display.

Host:
  Version:      9.0.0-preview.1.24080.9
  Architecture: x64
  Commit:       1d1bf92fcf

.NET SDKs installed:
  3.1.426 [C:\Program Files\dotnet\sdk]
  5.0.408 [C:\Program Files\dotnet\sdk]
  6.0.417 [C:\Program Files\dotnet\sdk]
  7.0.402 [C:\Program Files\dotnet\sdk]
  8.0.101 [C:\Program Files\dotnet\sdk]
  9.0.100-preview.1.24101.2 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 9.0.0-preview.1.24081.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 9.0.0-preview.1.24080.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 7.0.12 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 8.0.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 9.0.0-preview.1.24081.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found:
  None

Environment variables:
  Not set

global.json file:
  Not found

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
KieranFoot commented 9 months ago

What do you mean by default implementation?

It's recommending that, for performance, you pass the concrete type. I imagine that doing so does result in more performant code.

Note, you always have the ability to to silence specific warnings at your chosen level, weather that be a line of code, file, assembly or with a properties file to cover multiple projects at once.

I don't see how changing the warning or modifying it's behaviour would be of benefit, you just need to make a judgement call and decide if it is relevant to you or not.

WeihanLi commented 9 months ago

What do you mean by default implementation?

When using the interface default implementation method, we have to use the interface instead

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/default-interface-methods

image

Although we could suppress the diagnostic warning(I also do this way in my code), while think maybe we could improve the analyzer logic for a better developer experience

KieranFoot commented 9 months ago

Apologies, I see now.

WeihanLi commented 4 months ago

Maybe this should be moved to the Roslyn issues? Please help move the issue if needed, thanks very much

MiYanni commented 4 months ago

@jaredpar Do you have access to transfer this to https://github.com/dotnet/roslyn-analyzers?