-
Notifications
You must be signed in to change notification settings - Fork 128
Process static interface methods as virtual methods #2926
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
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
c5b9145
Use methodsWithBases instead of virtual methods to include static int…
jtschuster f48dd05
Add debug assert
jtschuster b5ce50c
rename to _methodsWithBases
jtschuster 34c160f
Use newer Microsoft.CodeAnalysis
jtschuster 374d7df
Add DAM annotation tests
jtschuster 7c95868
Formatting
jtschuster 9855745
Add test for interface without DAM
jtschuster ba5a5c2
formatting
jtschuster 1a11018
Process both bases and overrides in ProcessOverride
jtschuster b928bb8
Missing brace
jtschuster efad134
Add virtual methods in MarkBaseMethods
jtschuster 296f70a
Merge branch 'main' into methodsWithBases
jtschuster 3015b45
Only enqueue static virtual methods if the base or override is annotated
jtschuster 93e3702
Don't make array of virtual methods
jtschuster 63652a8
Merge branch 'main' into methodsWithBases
jtschuster 577f98c
Check for annotation matches for all interface methods
jtschuster c408ff0
formatting
jtschuster 2df2e1f
Undo removed line
jtschuster 5990074
Merge branch 'main' into methodsWithBases
jtschuster 5beef2c
PR Feedback
jtschuster File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
66 changes: 66 additions & 0 deletions
66
test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/Library.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Mono.Linker.Tests.Cases.DataFlow.Dependencies | ||
| { | ||
| public class Library | ||
| { | ||
| public interface IAnnotatedMethods | ||
| { | ||
| static abstract void GenericWithMethodsStatic<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> (); | ||
|
|
||
| static abstract void ParamWithMethodsStatic ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t); | ||
|
|
||
| [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] | ||
| static abstract Type ReturnWithMethodsStatic (); | ||
|
|
||
| void GenericWithMethods<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> (); | ||
|
|
||
| void ParamWithMethods ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t); | ||
|
|
||
| [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] | ||
| Type ReturnWithMethods (); | ||
| } | ||
|
|
||
| public interface IUnannotatedMethods | ||
| { | ||
| static abstract void GenericStatic<T> (); | ||
|
|
||
| static abstract void ParamStatic (Type t); | ||
|
|
||
| static abstract Type ReturnStatic (); | ||
|
|
||
| void Generic<T> (); | ||
|
|
||
| void Param (Type t); | ||
|
|
||
| Type Return (); | ||
| } | ||
|
|
||
| public abstract class AnnotatedMethods | ||
| { | ||
| public abstract void GenericWithMethods<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> (); | ||
|
|
||
| public abstract void ParamWithMethods ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t); | ||
|
|
||
| [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] | ||
| public abstract Type ReturnWithMethods (); | ||
| } | ||
|
|
||
| public abstract class UnannotatedMethods | ||
| { | ||
| public abstract void Generic<T> (); | ||
|
|
||
| public abstract void Param (Type t); | ||
|
|
||
| public abstract Type Return (); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
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.
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.
We are tracking implementations of static interface methods now in
_virtual_methods, so could we adjust the check above (if (!method.IsVirtual)) to allow these through instead?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 actually removed the implementations of static interface methods from _virtual_methods, so it should only be methods that are actually 'virtual' in IL in the list. Do you think it's worth it to add both the base and the implementations?
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.
Ah, I misread what https://github.com/dotnet/linker/pull/2926/files#diff-f3ab7d627296ba105613b9cc039ca6f4ddc7a9aa66c6060ca82f6456ae0ede4fR3295-R3296 was doing. Why do we need to add base methods to the list there? I'd have expected them to be included already by https://github.com/dotnet/linker/pull/2926/files#diff-f3ab7d627296ba105613b9cc039ca6f4ddc7a9aa66c6060ca82f6456ae0ede4fR3115.
Uh oh!
There was an error while loading. Please reload this page.
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 them in MarkBaseMethods is necessary for static methods in a preserved scope. Otherwise we miss annotation checks and marking the implementations for "Base is in a preserved scope".
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.
Thanks - would you mind adding a testcase for that? The tests pass for me without that check.
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 think it would make sense to merge your other PR first, then maybe this can be simplified since we are already checking for interface methods in preserved scopes there: https://github.com/dotnet/linker/pull/2868/files#diff-f3ab7d627296ba105613b9cc039ca6f4ddc7a9aa66c6060ca82f6456ae0ede4fR612
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've added some tests here that fail when the line is removed. I also found that instance interface methods aren't checked for matching annotations if the base in a preserved scope, so I removed the IsStatic check. This may warn if the interface implementation is removed or if the static interface methodImpl is removed, but I think it makes sense to still warn there.
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 think there is some small risk that comes with warning even if the interface impl is removed, but the analyzer should in theory warn about it either way so I am fine with this. Please add a note that this is the case. cc @vitek-karas in case you have opinions on this.