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

CA1812 Shouldn't Fire for DebuggerTypeProxy #1708

Closed
dcwuser opened this issue Jun 1, 2018 · 7 comments · Fixed by #4241
Closed

CA1812 Shouldn't Fire for DebuggerTypeProxy #1708

dcwuser opened this issue Jun 1, 2018 · 7 comments · Fixed by #4241
Labels
Category-Performance False_Positive A diagnostic is reported for non-problematic case
Milestone

Comments

@dcwuser
Copy link

dcwuser commented Jun 1, 2018

To change how a class is displayed in the VS debugger, one defines a type proxy and then adds the DebuggerTypeProxy attribute to the original class, so that the proxy class determines what is displayed by VS instead of the original class. It is natural to make the proxy class internal, since it is not intended to be part of the API surface. (VS finds and instantiates it via reflection.) When one does this, the proxy class triggers a CA1812 warning "XXX is an internal class that is apparently never instantiated."

I propose that if an internal class is referenced in any DebuggerTypeProxy attribute in an assembly, it should not trigger CA1812.

@twsouthwick
Copy link
Member

As a further suggestion, if a type is referenced via typeof(...) it seems like it shouldn't trigger this as well

@mavasani mavasani added this to the Unknown milestone Aug 7, 2018
@mavasani mavasani added Up for Grabs help wanted The issue is up-for-grabs, and can be claimed by commenting labels Aug 7, 2018
@sharwell sharwell added 4 - In Review and removed help wanted The issue is up-for-grabs, and can be claimed by commenting labels Dec 4, 2019
@KalleOlaviNiemitalo
Copy link

In my ASP.NET Web API (not ASP.NET Core) application, I got a CA1812 warning for internal class RawStringBinder : IModelBinder, which I use only in [FromUri(BinderType = typeof(RawStringBinder))]. So yes, I'd like the analyzer to recognize typeof(...).

@Evangelink
Copy link
Member

I think that the DebuggerTypeProxy makes sense. Regarding the FromUri I cannot find the doc for the old api so I am not entirely sure.

It might be too strong but we could also think about considering types declared in a typeof invocation of an attribute to always be instantiated, WDYT?

Tagging @sharwell and @mavasani

@KalleOlaviNiemitalo
Copy link

Regarding the FromUri I cannot find the doc for the old api so I am not entirely sure.

In ASP.NET Web API, System.Web.Http.FromUriAttribute inherits the BinderType property from System.Web.Http.ModelBinding.ModelBinderAttribute, which eventually gets an instance of that type from IDependencyResolver.GetService or Activator.CreateInstance.

@Evangelink
Copy link
Member

Thanks @KalleOlaviNiemitalo so it also makes sense to support this attribute.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Aug 4, 2020

dotnet/runtime#39126 added [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] to the Type type and string typeName parameters of the DebuggerTypeProxyAttribute constructors. The analyzer could recognize DynamicallyAccessedMembersAttribute and suppress the CA1812 warning, at least if any of the DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors bits is set.

For the sake of code that targets framework versions lower than .NET 5 though, the analyzer should recognize DebuggerTypeProxyAttribute and System.Web.Http.ModelBinding.ModelBinderAttribute as well, or just typeof(…) in all attributes in general.

@Evangelink
Copy link
Member

@KalleOlaviNiemitalo I have moved your 2 suggestions as different thread so that this one can move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category-Performance False_Positive A diagnostic is reported for non-problematic case
Projects
None yet
6 participants