Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

True-negative in GetUsedAssemblyReferences #66188

Closed
stan-sz opened this issue Jan 2, 2023 · 8 comments · Fixed by #67437
Closed

True-negative in GetUsedAssemblyReferences #66188

stan-sz opened this issue Jan 2, 2023 · 8 comments · Fixed by #67437
Assignees
Milestone

Comments

@stan-sz
Copy link

stan-sz commented Jan 2, 2023

Version Used: 4.4.0

Steps to Reproduce:

Given:

foo.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Library</OutputType>
    <TargetFramework>net6.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="13.0.2" />
  </ItemGroup>

</Project>

and foo.cs

namespace Library
{
    public static class Foo
    {
        public static string Bar() => "Baz";
    }
}

Repro requires an analyzer that dumps the output of GetUsedAssemblyReferences. An example implementation is in https://github.com/dfederm/ReferenceTrimmer/pull/20/files, the failed test case (where the above repo is taken from) is dotnet test ReferenceTrimmer.sln --filter UnusedPackageReference

Expected Behavior:

The result of CSharpCompilation.GetUsedAssemblyReferences does not contain Newtonsoft.Json.dll reference

Actual Behavior:

The result of CSharpCompilation.GetUsedAssemblyReferences does contain <home>\.nuget\packages\newtonsoft.json\13.0.2\lib\net6.0\Newtonsoft.Json.dll

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 2, 2023
@jcouv
Copy link
Member

jcouv commented Jan 3, 2023

I wasn't able to repro as unittest (copied below) or with provided repro steps. Could you set a breakpoint in the analyzer after GetUsedAssemblyReferences returned incorrect results, dump the process and share it with me (post it on dropbox, live drive or some such)?

When I checkout your PR and run dotnet test ReferenceTrimmer.sln --filter UnusedPackageReference, I get the following output, which indicates that Newtonsoft.Json was not found unused. ([update: turns out that is actually a valid repro, more details below])

  Determining projects to restore...
  All projects are up-to-date for restore.
C:\Program Files\dotnet\sdk\7.0.200-preview.22605.5\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(275,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [d:\repos\ReferenceTrimmer\test\Refe
renceTrimmer.Tests.csproj]
C:\Program Files\dotnet\sdk\7.0.200-preview.22605.5\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(275,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [d:\repos\ReferenceTrimmer\src\Refer
enceTrimmer.csproj]
C:\Program Files\dotnet\sdk\7.0.200-preview.22605.5\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(275,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [d:\repos\ReferenceTrimmer\analyzer\
ReferenceTrimmerAnalyzer.csproj]
  ReferenceTrimmerAnalyzer -> d:\repos\ReferenceTrimmer\analyzer\bin\Debug\netstandard2.0\ReferenceTrimmerAnalyzer.dll
  ReferenceTrimmer -> d:\repos\ReferenceTrimmer\src\bin\Debug\netstandard2.0\ReferenceTrimmer.dll
  ReferenceTrimmer.Tests -> d:\repos\ReferenceTrimmer\test\bin\Debug\net6.0\ReferenceTrimmer.Tests.dll
Test run for d:\repos\ReferenceTrimmer\test\bin\Debug\net6.0\ReferenceTrimmer.Tests.dll (.NETCoreApp,Version=v6.0)
Microsoft (R) Test Execution Command Line Tool Version 17.5.0-preview-20221130-01 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
  Failed UnusedPackageReference [1 s]
  Error Message:
   Assert.IsTrue failed.
Expected warnings:
PackageReference Newtonsoft.Json can be removed

Actual warnings:
<none>
  Stack Trace:
     at ReferenceTrimmer.Tests.E2ETests.RunMSBuild(String projectFile, String[] expectedWarnings) in d:\repos\ReferenceTrimmer\test\E2ETests.cs:line 261
   at ReferenceTrimmer.Tests.E2ETests.UnusedPackageReference() in d:\repos\ReferenceTrimmer\test\E2ETests.cs:line 111


Failed!  - Failed:     1, Passed:     0, Skipped:     0, Total:     1, Duration: 1 s - ReferenceTrimmer.Tests.dll (net6.0)

Here's the unittest I tried and debugged (usedReferences only had a corlib, but no reference for NewtonSoft):

        [Fact]
        public void AnalyzerTest()
        {
            var libComp = CreateCompilation("", assemblyName: "NewtonSoft");
            var source = @"
namespace Library
{
    public static class Foo
    {
        public static string Bar() => ""Baz"";
    }
}";

            var comp = CreateCompilation(source, references: new[] { libComp.EmitToImageReference() });
            comp.VerifyDiagnostics();
            comp.VerifyAnalyzerDiagnostics(new[] { new UsedAssemblyReferencesDumper() }, null, null);
        }

        public class UsedAssemblyReferencesDumper : DiagnosticAnalyzer
        {
            internal static readonly string Title = "Unused references should be removed";
            internal static DiagnosticDescriptor Rule = new DiagnosticDescriptor("RT0001", Title, "'{0}'", "References", DiagnosticSeverity.Warning, true, customTags: WellKnownDiagnosticTags.Unnecessary);

            public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

            public override void Initialize(AnalysisContext context)
            {
                context.EnableConcurrentExecution();
                context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
                context.RegisterCompilationAction(DumpUsedReferences);
            }

            private static void DumpUsedReferences(CompilationAnalysisContext context)
            {
                Compilation compilation = context.Compilation;
                if (compilation.Options.Errors.IsEmpty)
                {
                    IEnumerable<MetadataReference> usedReferences = compilation.GetUsedAssemblyReferences();
                    AdditionalText analyzerOutputFile = context.Options.AdditionalFiles.FirstOrDefault(file => file.Path.EndsWith("_ReferenceTrimmer_GetUsedAssemblyReferences.txt", StringComparison.OrdinalIgnoreCase));
                    Directory.CreateDirectory(Path.GetDirectoryName(analyzerOutputFile.Path));
                    File.WriteAllLines(analyzerOutputFile.Path, usedReferences.Select(reference => reference.Display));
                }
            }
        }

@jcouv jcouv added the Need More Info The issue needs more information to proceed. label Jan 3, 2023
@stan-sz stan-sz changed the title False-positive in GetUsedAssemblyReferences True-negative in GetUsedAssemblyReferences Jan 3, 2023
@stan-sz
Copy link
Author

stan-sz commented Jan 3, 2023

"which indicates that Newtonsoft.Json was not found unused."

That's precisely the true-negative bug I'm reporting (thanks for taking the time to do the repro from another repo!). The problem is that Newtonsoft is declared as a PackageReference in that project and despite it is not referenced in the code, GetUsedAssemblyReferences returns it as a used reference. To prove it, remove the PackageReference and the project still compiles.

@ghost ghost removed the Need More Info The issue needs more information to proceed. label Jan 3, 2023
@jcouv
Copy link
Member

jcouv commented Jan 3, 2023

Notes from offline chat with @stan-sz:
We indeed have a working repro. After running the dotnet test command, there are binary logs (in D:\repos\ReferenceTrimmer\test\TestResults\Deploy_jcouv 2023-01-03 00_09_40\UnusedPackageReference\Logs) in my case. From that, we extracted the command-line for csc. It can be run from d:\repos\ReferenceTrimmer\test\TestResults\Deploy_jcouv 2023-01-03 00_09_40\UnusedPackageReference\Library. This allows debugging the analyzer (using a Debugger.Launch()).

image

@jcouv jcouv self-assigned this Jan 3, 2023
@jcouv jcouv added this to the 17.5 milestone Jan 3, 2023
@jcouv
Copy link
Member

jcouv commented Jan 3, 2023

Isolated a repro (see below). A significant variable is whether documentation mode is turned on.
See relevant code and also mention in docs.

So, as far as I can tell, this is by design and you should turn on documentation mode to resolve this problem.

        [Fact]
        public void AnalyzerTest()
        {
            var libComp = CreateCompilation("namespace System { class C { } }", assemblyName: "NewtonSoft");
            var source = @"
using System;
namespace Library
{
    public static class Foo
    {
        public static string Bar() => ""Baz"";
    }
}";

            var comp = CreateCompilation(source, references: new[] { libComp.EmitToImageReference() }, 
                parseOptions: CSharpParseOptions.Default.WithDocumentationMode(DocumentationMode.None));
            comp.VerifyAnalyzerDiagnostics(new[] { new UsedAssemblyReferencesDumper() }, null, null);
        }

        public class UsedAssemblyReferencesDumper : DiagnosticAnalyzer
        {
            internal static readonly string Title = "Unused references should be removed";
            internal static DiagnosticDescriptor Rule = new DiagnosticDescriptor("RT0001", Title, "'{0}'", "References", DiagnosticSeverity.Warning, true, customTags: WellKnownDiagnosticTags.Unnecessary);

            public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

            public override void Initialize(AnalysisContext context)
            {
                context.EnableConcurrentExecution();
                context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
                context.RegisterCompilationAction(DumpUsedReferences);
            }

            private static void DumpUsedReferences(CompilationAnalysisContext context)
            {
                Compilation compilation = context.Compilation;
                if (compilation.Options.Errors.IsEmpty)
                {
                    IEnumerable<MetadataReference> usedReferences = compilation.GetUsedAssemblyReferences();
                    AdditionalText analyzerOutputFile = context.Options.AdditionalFiles.FirstOrDefault(file => file.Path.EndsWith("_ReferenceTrimmer_GetUsedAssemblyReferences.txt", StringComparison.OrdinalIgnoreCase));
                    Directory.CreateDirectory(Path.GetDirectoryName(analyzerOutputFile.Path));
                    File.WriteAllLines(analyzerOutputFile.Path, usedReferences.Select(reference => reference.Display));
                }
            }
        }

@jcouv jcouv added Question Resolution-Answered The question has been answered and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 3, 2023
@stan-sz
Copy link
Author

stan-sz commented Jan 3, 2023

What is the switch to enable documentation mode?

@jcouv
Copy link
Member

jcouv commented Jan 4, 2023

The doc command-line option controls the documentation mode in CSharpParseOptions (logic).

@jcouv
Copy link
Member

jcouv commented Jan 4, 2023

From discussion with @stan-sz, it would be good to improve the documentation. The xml doc for GetUsedAssemblyReferences and https://github.com/dotnet/roslyn/blob/main/docs/features/UsedAssemblyReferences.md could mention that the DocumentationMode is relevant (and its effect).

@ericstj
Copy link
Member

ericstj commented Feb 6, 2023

I just hit the same thing. It was non-intuitive that GetUsedAssemblyReferences would report nearly everything as used unless I set GenerateDocumentationFile, those seem to be unrelated options to me. If it could be made to work without this setting I think it would be valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment