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

[API Proposal]: System.Diagnostics.CodeAnalysis equivalent of JetBrains.Annotations.MeansImplicitUseAttribute/UsedImplicitlyAttribute #110215

Closed
weifenluo opened this issue Nov 27, 2024 · 11 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Tools-ILLink .NET linker development as well as trimming analyzers

Comments

@weifenluo
Copy link

Background and motivation

Some class members are called via reflection, or generated source code, with some special attribute, for example:

[Initializer]
private void Initialize()
{
    ...
}

Because there is no direct reference (generated source does not count either in VS), these members are displayed "grayed" in VS, implies the code should be deleted. There are two attributes [MeansImplicitUse] and [UsedImplicitly] in JetBrains.Annotations namespace to handle this exactly: we can either apply [MeansImplicitUse] attribute to [Initializer], or [UsedImplicitly] to Initialize() method, to mark the members as referenced.

There was also a question on stackexchange.com more than 7 years ago: Best practice to mark a method that is called via reflection?

I believe we should introduce these attributes under System.Diagnostics.CodeAnalysis too.

API Proposal

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
public sealed class MeansImplicitUseAttribute : Attribute
{
}

[AttributeUsage(AttributeTargets.All, AllowMultiple = false)]
public sealed class UsedImplicitlyAttribute : Attribute
{
}

API Usage

[MeansImplicitUse]
public sealed class InitializerAttribute : Attribute
{
}

[Initializer]
private void Initialize()
{
}

or,

[UsedImplicitly]
[Initializer]
private void Initialize()
{
}

Alternative Designs

There is no other easy way to work this around.

Risks

No risk at all because it's an additional feature for compile time only.

@weifenluo weifenluo added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 27, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 27, 2024
Copy link
Contributor

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

@julealgon
Copy link

julealgon commented Nov 27, 2024

I'd rather this was handled by the new trimming-related [DynamicallyAccessedMembers] attribute.

I was surprised when testing it locally and seeing it ignore the attribute and still show the method as unused. Then I found this:

Duplicate?

@weifenluo
Copy link
Author

weifenluo commented Nov 27, 2024

@julealgon [DynamicallyAccessedMembers] (or [DynamicDependency]) may have similar effects as [UsedImplicitly], but there is no equivalent of [MeansImplicitUse], which can apply to attribute, avoiding a lot of attributes noise.

In my case, [MeansImplicitUse] is what I really want.

@Sergio0694 Sergio0694 added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-System.Diagnostics labels Nov 28, 2024
Copy link
Contributor

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

@Sergio0694
Copy link
Contributor

@MichalStrehovsky I remember we discussed something conceptually very similar to this multiple times in the past in several occasions, and the answer was always that the fact there is no (easy) way to do this (outside of [DynamicDependency]) is actually completely intended, right? And that there's no interest in making this any easier because it would encourage bad patterns, as oppose to correctly annotating the flow of dynamically accessed members? It is true that the use case here is pretty specific, but I guess once one opens the door to this... 😅

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky I remember we discussed something conceptually very similar to this multiple times in the past in several occasions, and the answer was always that the fact there is no (easy) way to do this (outside of [DynamicDependency]) is actually completely intended, right? And that there's no interest in making this any easier because it would encourage bad patterns, as oppose to correctly annotating the flow of dynamically accessed members? It is true that the use case here is pretty specific, but I guess once one opens the door to this... 😅

Right, the difference is that this proposal is not in terms of trimming, but in terms of IDE experience. For both cases it kind of defeats the purpose of the tooling. The tooling tries to help you find unused code. Placing an attribute that says "this is always used" will always make it look used, even if it is in fact not used. "Go to references" in the IDE will not find anything.

My take is that this game can only be won if you don't play it. Don't use patterns that construct invisible references and if you do, don't be surprised static analysis gives you experience comparable to statically analyzing JavaScript.

@julealgon
Copy link

@MichalStrehovsky

My take is that this game can only be won if you don't play it. Don't use patterns that construct invisible references and if you do, don't be surprised static analysis gives you experience comparable to statically analyzing JavaScript.

One thing that helps in some cases is to rely on the compile-time nameof(YourNeverUsedMethod) when calling it through reflection or when propagating the method name to be used by reflection. That single reference from the nameof is enough for the IDE to realize it is being used in a way.

@weifenluo
Copy link
Author

weifenluo commented Nov 28, 2024

I disagree with the statement:

The tooling tries to help you find unused code. Placing an attribute that says "this is always used" will always make it look used, even if it is in fact not used.

Back to the example:

class ENCLOSING_TYPE
{
    ...
    [Initializer]
    private void Initialize()
    {
        ...
    }
}

The Initialize method IS actually used, not look used. There are two modes this code get used:

  1. Dynamically called via reflection;
  2. The source generator generates the type descriptor for the ENCLOSING_TYPE, which statically references the code:
class TypeDescriptor<ENCLOSING_TYPE>
{
    ...
    protected override void Build(MemberBuilder builder)
    {
        ...
        static Attribute GetTypeInfoInitializerAttribute0()
        {
            var result = new InitializerAttribute();
            result.Setup<ENCLOSING_TYPE>("Initialize", static (ENCLOSING_TYPE x) => x.Initialize());
            return result;
        }
    }
}

The generated TypeDescriptor<ENCLOSING_TYPE> class, is then used to provide implementation of a public interface.

Mode 1. and mode 2. are functionally equivalent, since in mode 2 the code IS statically referenced, I would rather consider the code IS used even for mode 1.

Unfortunately even for mode 2, VS does not count code reference for generated code. The tooling just reported unused code falsely.

@weifenluo
Copy link
Author

If the tooling cannot figure out unused code 100% correctly, we should provide some guidance to the tooling, help it to do its job right. This is exactly the purpose of this proposal.

@MichalPetryka
Copy link
Contributor

Unfortunately even for mode 2, VS does not count code reference for generated code.

As far as I remember, Rider and Resharper do handle this case so this is purely a VS issue and should be reported for that.

@MichalStrehovsky
Copy link
Member

You'll likely get more traction if you open a VS feedback issue. The VS team will look at how to solve this. If they come up with an attribute, they'll file an issue here to get it reviewed. We don't add attributes unless there's commitment to consume them. Adding attributes is easy. Consuming them is the actual work. We don't control VS/VSCode planning and schedules.

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
Status: No status
Development

No branches or pull requests

5 participants