Skip to content

Fix inconsistencies in Requires attribute checks #2553

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.

Sign up for GitHub

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

Jump to bottom
Merged
sbomer merged 5 commits into from
Jan 31, 2022
Merged

Fix inconsistencies in Requires attribute checks #2553

sbomer merged 5 commits into from
Jan 31, 2022

Conversation

@sbomer
Copy link
Member

@sbomer sbomer commented Jan 27, 2022 •
edited
Loading

While investigating failures in #2536 I came across a bunch of places where we check for RequiresAttribute without considering RUC on type.

  • DAM analyzer:
  • Requires analyzer:
    • checking for attributes on ctors of types substituted for generic parameters with new constraint
    • the above should be silenced if the declaring type of the caller has RequiresAttribute
    • checking for RequiresAttribute on attribute ctors should check the attribute type for RequiresAttribute
    • the above should be silenced if the declaring type of the attribute provider has RequiresAttribute (but not when the provider is itself a type) (Inconsistent behavior for Requires on attribute constructor #2529)
  • RUC analyzer:
    • warnings for use of dynamic should be silenced in a method whose declaring type has RequiresAttribute

I added tests for all of the above. Also fixes #2529.

I noticed another inconsistency in the attribute checks that I'm not addressing here: #2554

@sbomer sbomer requested a review from marek-safar as a code owner January 27, 2022 17:34
tlakollo
tlakollo reviewed Jan 27, 2022
View reviewed changes
tlakollo
tlakollo reviewed Jan 27, 2022
View reviewed changes
sbomer added 2 commits January 27, 2022 21:31
@sbomer
PR feedback
ea615af 
- Remove unnecessary check for ITypeSymbol
- Clean up null check logic
@sbomer
containingSymbol -> symbol
88a472b
tlakollo
tlakollo approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@tlakollo tlakollo left a comment

Choose a reason for hiding this comment

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

Looks good to me

vitek-karas
vitek-karas approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Looks good - maybe just one nit about making it future proof by always validating the RUC attribute.

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs
Comment on lines +186 to +187
if (instanceCtor.TargetHasRequiresAttribute (RequiresAttributeName, out var requiresAttribute) &&
VerifyAttributeArguments (requiresAttribute)) {
Copy link
Member

@vitek-karas vitek-karas Jan 31, 2022

Choose a reason for hiding this comment

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

In @tlakollo PR there will be a single method to do this at once: https://github.com/dotnet/linker/pull/2418/files#diff-6417c9266a49f206907dd0ece745dc0c77c5f5c9a0c92ec3bb749d4c8e2c039aR14-R15
Please make sure we end up using it here as well.

src/ILLink.RoslynAnalyzer/RequiresISymbolExtensions.cs Outdated
if (symbol is ITypeSymbol)
return false;

if (symbol.HasAttribute (requiresAttribute)
Copy link
Member

@vitek-karas vitek-karas Jan 31, 2022

Choose a reason for hiding this comment

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

I'm thinking a safer thing would be to call "Validate" on the attribute as well - it basically checks that the attribute is in the form we recognize. As a safe guard against potential future changes which could for example add some kind of "feature condition" to the RUC attribute (I know we've discussed this), in which case the attribute is not always "active", so it's safer to NOT recognize it.

@sbomer sbomer mentioned this pull request Jan 31, 2022
Open
@sbomer
Copy link
Member Author

sbomer commented Jan 31, 2022

Thanks @vitek-karas, I will track those suggestions in #2554

@sbomer sbomer merged commit b2e8419 into dotnet:main Jan 31, 2022
sbomer added a commit to sbomer/linker that referenced this pull request Feb 1, 2022
@sbomer
Fix inconsistencies in Requires attribute checks (dotnet#2553)
a0d79f5 
* Fix inconsistencies in Requires attribute checks

* Fix build and continue verifying attribute arguments

* PR feedback

- Remove unnecessary check for ITypeSymbol
- Clean up null check logic

* containingSymbol -> symbol

* Rename symbol -> member
@sbomer sbomer deleted the fixAttributeChecks branch February 22, 2022 18:13
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
@sbomer
Fix inconsistencies in Requires attribute checks (dotnet/linker#2553)
4d1ee8a 
* Fix inconsistencies in Requires attribute checks

* Fix build and continue verifying attribute arguments

* PR feedback

- Remove unnecessary check for ITypeSymbol
- Clean up null check logic

* containingSymbol -> symbol

* Rename symbol -> member

Commit migrated from dotnet/linker@b2e8419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vitek-karas vitek-karas vitek-karas approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

@jtschuster jtschuster Awaiting requested review from jtschuster

@agocke agocke Awaiting requested review from agocke

+1 more reviewer

@tlakollo tlakollo tlakollo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent behavior for Requires on attribute constructor

4 participants

@sbomer @vitek-karas @tlakollo @jtschuster