IDE0051 should factor in new dependency attributes in .NET 5

[stephentoub]

Version Used:
3.9.0-1.20480.1+b3c3f4875957e51526000898673291d11708d35b

Steps to Reproduce:

using System;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

class Program
{
    static void Main() => new Program().Bar();

    [DynamicDependency("Foo")]
    private void Bar() => typeof(Program).GetMethod("Foo", BindingFlags.Instance | BindingFlags.NonPublic).Invoke(this, null);

    private void Foo() => Console.WriteLine("Hello");
}

Expected Behavior:
.NET 5 adds new attributes that enable dependencies not obvious in the code, e.g. those via reflection, to be used for dead code elimination tools like https://github.com/mono/linker to better understand such dependencies. IDE0051 is such a tool and should ideally respect these attributes, such that in this example, IDE0051 doesn't fire.

Actual Behavior:
IDE0051 fires and suggests deleting the "unused" method:

cc: @eerhardt, @NickCraver

[Youssef1313]

@stephentoub Couple of questions to better understand this:

• Can the attribute reference a field/method outside of the current class?
• If there are method overloads, what's the correct way to use the attribute to specify a specific overload?
• For Visual Basic, which is case insensitive, should the attribute be case insensitive too?

[stephentoub]

Can the attribute reference a field/method outside of the current class?

Yes. Even outside of the current assembly. So, there are certainly places where there's no way IDE0051 could appropriately respect the attribute (since it can't necessarily see the entire application), and potentially even times when it could theoretically but doing so could be prohibitively expensive. It'd be good, though, if the appropriate thought could be put into if/where/when/how it would make sense, so as to have a complete rather than disjoint story around this kind of trimming / dead-code removal.

If there are method overloads, what's the correct way to use the attribute to specify a specific overload?

@marek-safar, @eerhardt, where is the syntax documented? @Youssef1313, you can use the simple name, in which case it says that all of the overloads should be considered used, or you can name a specific overload by specifying the parameter types, e.g. https://github.com/dotnet/runtime/blob/c44154b15126786260f8a6397248dd04c0e015e3/src/libraries/System.Private.CoreLib/src/System/String.cs#L48

For Visual Basic, which is case insensitive, should the attribute be case insensitive too?

@marek-safar, what does the linker do here?

[Youssef1313]

It'd be good, though, if the appropriate thought could be put into if/where/when/how it would make sense, so as to have a complete rather than disjoint story around this kind of trimming / dead-code removal.

Probably the IDE team will want to take this to design meeting to decide what are the essential situations to handle and what are the situations that doesn't need to be handled.

[marek-safar]

where is the syntax documented

ILLinker uses the same syntax as Roslyn for documentation so it's documented at https://github.com/dotnet/csharplang/blob/master/spec/documentation-comments.md#processing-the-documentation-file

For Visual Basic, which is case insensitive, should the attribute be case insensitive too?

Not sure why it should be. ILLinker is IL-based tool and the attribute specifies metadata value.

[CyrusNajmabadi]

you can use the simple name, in which case it says that all of the overloads should be considered used,

fwiw, that's challenging as i don't think we have such a concept at all in roslyn. Would it be more appropriate to remove hte 'overload' concept from these strings and say that all dependencies must be individually specified?

[Youssef1313]

@CyrusNajmabadi Would this be very similar to what's done in https://github.com/dotnet/roslyn/blob/5b1f31a1326768e46d3ac021cb5001b33955bc8c/src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder_GlobalSuppressions.cs? (#44368)

[huoyaoyuan]

dotnet/roslyn-analyzers#4243 (comment)

[mavasani]

Tagging @sbomer who implemented the parser for the string arguments to DynamicDependencyAttribute in dotnet/linker#1197.

@sbomer What is the best way for Roslyn based tooling to convert the string argument to this attribute to Roslyn symbols? Given that the parser you added is adapted from Roslyn's DocumentationCommentId parser, would passing the string to DocumentationCommentId .GetSymbolsForDeclarationId cover all valid allowed values?