Process static interface methods as virtual methods #2926
Conversation
| if (!method.IsVirtual) | ||
| return; | ||
|
|
||
| // Implementations of static interface methods are not virtual and won't reach here |
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
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.
Thanks - would you mind adding a testcase for that? The tests pass for me without that check.
There was a problem hiding this comment.
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.
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.
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.
| if (!method.IsVirtual) | ||
| return; | ||
|
|
||
| // Implementations of static interface methods are not virtual and won't reach here |
There was a problem hiding this comment.
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.
sbomer
left a comment
sbomer
left a comment
There was a problem hiding this comment.
A few nits, otherwise looks good to me. Thanks!
Static methods implementing static interface methods are not marked as virtual and were not checked for matching RUC and DAM annotations. This change adds all base methods to the _virtual_methods list to be checked for matching DAM and RUC annotations. This may cause extra/unnecessary warnings for mismatched annotations even a method is removed by the linker. Commit migrated from dotnet/linker@e45a935
2 participants
Static interface methods that are implicitly implemented are not virtual in IL, so they are not processed in the same ways other methods are. This changes the collection to be _methodsWithBases and adds methods that have base methods in addition to virtual methods to that collection.
Also adds tests for ensuring DAM and RUC annotations are the same for static interface overrides.
Completes work for #2058