Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable single file analyzer in the runtime #50894
Enable single file analyzer in the runtime #50894
Changes from 17 commits
937f893
1b3dc8e
f839417
f92327e
ae1bd07
23828b9
9ec8520
eba095e
67dbd63
63a395a
91ecb47
eb6d83a
4bfc9a8
6367cb9
6bad341
0597dde
4786f73
33f6916
b232294
2947adb
f120ae9
874f872
1debcce
870dc92
670bdcb
dbb0690
0994fec
2d47bad
df3c4bd
c2dc595
ff7f84f
305b675
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you look at only making the getter for CodeBase "unsafe" and leave the setter without an attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the annotation is in the getter of codebase in the file
src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs
in this PR.I didn't change the analyzer behavior for now since it will mean more work to special case the codebase getter in the analyzer. The plan is to annotate first, then get rid of the special casing in the analyzers for CodeBase/EscapedCodeBase since they are deprecated and provide a misleading warning, and once the analyzer doesn't check for those we can delete the UnconditionalSuppressMessage and tests in the linker. Also cannot remove it for now since the annotations are not in place and we wouldn't warn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if we didn't suppress this - how much would it spread (propagating the attribute to affected public APIs).
The functionality will not work in single-file - so silencing it until it gets fixed is wrong. The warnings are there for this specific reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding [RequiresAssemblyFiles] in this function doesn't propagate to other places. I think is a limitation in the analyzer, because the attribute is on an override, but the methods call a virtual function in LicenseContext.cs
If propagated to the callers, then GetCurrentContextInfo uses LicenseUsageMode.Designtime and it's called by COM so it doesn't propagate more.
The other caller is GetLicense which uses LicenseUsageMode.Runtime, for me is uncertain if that will result in calling the override in DesigntimeLicenseContext. But that method has about 10 different call sites that would need to be annotated most of them in System.ComponentModel.LicenseManger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well - it doesn't propagate in the analyzer - but functionally it does propagate - so this is the killer argument for me. It would basically mean we should potentially annotate the entire COM this way.
Regarding the analyzer: We should add a similar check to what RequiresUnreferencedCode has (at least in the linker) - that is, all methods in a virtual override chain should have the same annotations. So if an override has the attribute, the base method should have it as well, and vice-versa. If this is not the case, please file a bug for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened dotnet/linker#1985 to track work for virtual calls in the analyzers