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

Match Attributes for virtual methods and overrides #2046

Merged
merged 16 commits into from
Jun 22, 2021

Conversation

tlakollo
Copy link
Contributor

Add analyzer logic to detect when a method has RUC or RAF but its base method or override method doesn't
Use IL2046 to display errors for RUC
Use IL3003 to display errors for RAF
Add IL3003 to error-codes.md
Add tests
Fixes #1985

method or override doesn't
Use IL2046 to display errors for RUC
Use IL3003 to display errors for RAF
Add IL3003 to error-codes.md
Add tests
docs/error-codes.md Outdated Show resolved Hide resolved
Add support for properties and events
Add more tests
Use a single resource for different requiresattributes
Change error code IL3003 to be generic to requiresattributes
src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs Outdated Show resolved Hide resolved
}

[Fact]
public Task OverrideHasAttributeButBaseDoesnt ()
Copy link
Member

@agocke agocke May 26, 2021

Choose a reason for hiding this comment

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

Time to move these tests to a base class, I think -- rather than continue to duplicate tests between RequiresAssemblyFiles and RequiresUnreferencedCode

@agocke
Copy link
Member

agocke commented May 26, 2021

I'm surprised not to see any tests being unskipped for the linker -- since we should be running the analyzer over the linker tests I assumed we would be seeing those warnings now

Add test with explicit interface implementation
discussed in dotnet#2077
Now interfaces matching are lookup on types instead of asking per member
Added support for specifying in Expected Warning and LogContains where
the diagnostic is spected to be produced by default is LinkerAndAnalyzer
Renaming methods and classes to specify Requires instead of
RequiresUnreferencedCode
Merge branch 'main' of https://github.com/mono/linker into MatchOverrideAttributes
@mateoatr
Copy link
Contributor

mateoatr commented Jun 9, 2021

I see you added some of the changes discussed in #2077, could we get these in a separate PR to make it easier to review 😄?

Add Explicit Interface testing
src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Outdated Show resolved Hide resolved
src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Outdated Show resolved Hide resolved
src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Outdated Show resolved Hide resolved
src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs Outdated Show resolved Hide resolved
Create test were the attributes between metadata and source dont match
following the implementation of ObsoleteAttribute messages
Verify in linker tests that the warnings for base and derived methods
are generated correctly
Add new warnings to error-codes.md
Add new resource messages
Missing testing on interface/implementation on linker
interfaces, changed from LogContains to ExpectedWarning to find the
error codes easier

Change condition for linker to ask about interfaces instead of asking if
a method is virtual
…ideAttributes

Add diagnostics to shared resource file
Update documentation, resource and tests
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

Looks like the tests still need to move to a base class

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Outdated Show resolved Hide resolved
src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Outdated Show resolved Hide resolved
src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs Outdated Show resolved Hide resolved
src/ILLink.Shared/MessageFormat.cs Outdated Show resolved Hide resolved
src/ILLink.Shared/MessageFormat.cs Outdated Show resolved Hide resolved
src/ILLink.Shared/MessageFormat.cs Outdated Show resolved Hide resolved
src/ILLink.Shared/MessageFormat.cs Outdated Show resolved Hide resolved
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@tlakollo tlakollo merged commit a07cab7 into dotnet:main Jun 22, 2021
@tlakollo tlakollo deleted the MatchOverrideAttributes branch June 22, 2021 21:39
akoeplinger added a commit to dotnet/sdk that referenced this pull request Jun 23, 2021
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
Add analyzer logic to create a diagnostic if attributes don't match between all interface implementations or overrides
Use IL2046 to display errors for RUC
Use IL3003 to display errors for RAF
Add IL3003 to error-codes.md
Add tests
Adds support for nullable attribute operations
Add support for adding metadata information into the compilation
Divide the current warning message into 4 different more insightful diagnostic messages following the structure implemented in ObsoleteAttribute messages
Add IL2046 to diagnostic format to ILLink.Shared

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

Successfully merging this pull request may close these issues.

Analyzer needs to warn on virtual methods
4 participants