search

dotnet / roslyn-analyzers

MIT License
1.58k stars 463 forks source link

CA1812: Handle DynamicallyAccessedMembers #4243

Open Evangelink opened 4 years ago

Evangelink commented 4 years ago

https://github.com/dotnet/runtime/pull/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.

Originally posted by @KalleOlaviNiemitalo in https://github.com/dotnet/roslyn-analyzers/issues/1708#issuecomment-668643577

Evangelink commented 4 years ago

This one makes sense to me. @mavasani do you +1?

huoyaoyuan commented 3 years ago

This should be a good candidate for programmatic suppressions. Any indirect usage pattern can take a analyzer for suppression.

KalleOlaviNiemitalo commented 3 years ago

@huoyaoyuan, do you mean AvoidUninstantiatedInternalClassesAnalyzer should not recognize DynamicallyAccessedMembersAttribute, and instead a separate analyzer should recognize it and use the https://github.com/dotnet/roslyn/issues/30172 DiagnosticSuppressor API to suppress the CA1812 warnings? If so, would this split make the analyzers easier to maintain or what would be the benefit?

Evangelink commented 3 years ago

My understanding is that programmatic suppression is a way for a library to provide some specific knowledge (e.g. Application_Start cannot be marked static) to the analyzer. In this case, if the attribute doesn't exist yes IMO that's a good way to provide this knowledge. But since we are now talking about a built-in attribute I think that the analyzer should handle it rather than expecting all libraries to provide this knowledge.

Just my 2 cents.

huoyaoyuan commented 3 years ago

would this split make the analyzers easier to maintain

It should. I can tell several patterns now: ioc constructors for MEF and Extensions.DI, json constructors, data validation method referenced by name. They are independent with each other, and some of them lives in layer above.

KalleOlaviNiemitalo commented 3 years ago

@huoyaoyuan, do you think the DynamicallyAccessedMembersAttribute CA1812 suppressor analyzer should ship in the same Microsoft.CodeQuality.Analyzers package as AvoidUninstantiatedInternalClassesAnalyzer itself?

huoyaoyuan commented 3 years ago

I think it should be in NetAnalyzers, the one implicit installed with .NET 5 SDK; and maybe NetCoreAnalyzers. Suppressor should be associated with the target api package. For DynamicallyAccessedMembers, it's a .NET Core 5+ concept, so it should be in the package handles the platform.

KalleOlaviNiemitalo commented 3 years ago

A project could target both .NET 5 and lower versions, and conditionally define DynamicallyAccessedMembersAttribute and DynamicallyAccessedMemberTypes as internal types. Such a project might benefit from having the analyzer recognize those types even when they are not in the targeted API. I don't know whether any projects actually do so.