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

Implement the ModuleInitializerAttributeShouldNotBeUsedInLibraries analyzer #5347

Conversation

jeffhandley
Copy link
Member

Fixes dotnet/runtime#43328 by raising a diagnostic any time the [ModuleInitializer] attribute is applied to a method (in conformance with the C# language feature that supports this attribute).

Per the API review of this analyzer, the analyzer will be defaulted to on as a warning.

@jeffhandley jeffhandley requested a review from a team as a code owner August 5, 2021 06:34
Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyzer doesn't really check whether we're in a library or not.
So the false positive rate can be high.

@jeffhandley
Copy link
Member Author

The analyzer doesn't really check whether we're in a library or not.
So the false positive rate can be high.

We expect the [ModuleInitializer] attribute to be used very rarely, so we can tolerate a level of false positives.

Is it possible to check if the type is in a class library assembly?

@Youssef1313
Copy link
Member

The analyzer doesn't really check whether we're in a library or not.
So the false positive rate can be high.

We expect the [ModuleInitializer] attribute to be used very rarely, so we can tolerate a level of false positives.

Is it possible to check if the type is in a class library assembly?

Yeah I think it's possible: if (context.Compilation.Options.OutputKind != OutputKind.DynamicallyLinkedLibrary) return;

@jeffhandley
Copy link
Member Author

Yeah I think it's possible: if (context.Compilation.Options.OutputKind != OutputKind.DynamicallyLinkedLibrary) return;

Thanks! This will still allow the analyzer to run for web and mobile apps I presume, but it will remove false positives from console apps and windows apps, so that's still an improvement.

@Youssef1313
Copy link
Member

This will still allow the analyzer to run for web and mobile apps I presume

I think it won't run for web apps. Not sure about mobile.

@jeffhandley
Copy link
Member Author

Thanks for the thorough review, @Youssef1313! I've addressed all of your feedback if you want to take another look.

@jeffhandley
Copy link
Member Author

I forgot to run msbuild /t:Pack on that commit. I'm correcting the resource strings and running that command, which should fix the build.

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #5347 (d7a7799) into release/6.0.1xx (92cc133) will increase coverage by 0.01%.
The diff coverage is 98.34%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5347      +/-   ##
===================================================
+ Coverage            95.60%   95.62%   +0.01%     
===================================================
  Files                 1236     1238       +2     
  Lines               283895   284725     +830     
  Branches             17032    17083      +51     
===================================================
+ Hits                271414   272261     +847     
+ Misses               10179    10157      -22     
- Partials              2302     2307       +5     

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left few NITs overall looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants