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

Separate interface method trimming logic and update for static interface methods #2868

Merged
merged 53 commits into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
da56f3d
Mark static virtual interface methods
jtschuster Jun 27, 2022
869748e
Undo IgnoreScope changes
jtschuster Jun 28, 2022
f4af27f
Add tests for virtual statics in preserve scope
jtschuster Jun 29, 2022
80ddcd2
add references folder to test output for skip assemblies
jtschuster Jun 29, 2022
deba9dd
Remove implicit implementations
jtschuster Jun 29, 2022
26fdc82
formatting
jtschuster Jun 29, 2022
d66f76c
Undo cecil and sdk changes
jtschuster Jun 29, 2022
9b9cfae
Update StaticVirtualInterfaceMethodsLibrary.cs
jtschuster Jun 30, 2022
c9c7e88
clean up
jtschuster Jun 30, 2022
8dfbdc9
formatting
jtschuster Jun 30, 2022
9076e0c
formatting
jtschuster Jul 1, 2022
32978f3
formatting
jtschuster Jul 1, 2022
fb5254a
New PreserveScope logic
jtschuster Jul 1, 2022
f522641
Better wording
jtschuster Jul 1, 2022
6a24f50
Move matrix below preserveScope check
jtschuster Jul 1, 2022
9a5fd73
Run dotetn-format latest version
jtschuster Jul 2, 2022
471af7b
whip, switching machines
jtschuster Jul 6, 2022
57e7ea9
Merge remote-tracking branch 'upstream/main' into wip
jtschuster Jul 7, 2022
e6e69a7
Wip
jtschuster Jul 7, 2022
a446e6f
Track interface overrides differently than virtual methods
jtschuster Jul 12, 2022
50f2664
Merge remote-tracking branch 'upstream/main' into markStaticVirtuals
jtschuster Jul 12, 2022
f5502c9
formatting
jtschuster Jul 12, 2022
182b293
clean up after merge
jtschuster Jul 12, 2022
882a3ea
Remove generated tests
jtschuster Jul 12, 2022
602f015
Undo test change from another issue
jtschuster Jul 12, 2022
ff46aed
Add comments to IsInterfaceMethodNeededByTypeDueToInterface
jtschuster Jul 12, 2022
0a40b0b
formatting
jtschuster Jul 12, 2022
dab83a9
remove using
jtschuster Jul 13, 2022
267b080
Use HashSet and add comment
jtschuster Jul 13, 2022
0ce04c7
Remove unnecessary check
jtschuster Jul 13, 2022
fb31da8
Style changes
jtschuster Jul 14, 2022
f2e53ae
Remove RootAssemblyMode check
jtschuster Jul 15, 2022
e554231
Add docs
jtschuster Jul 15, 2022
840b2a4
Update docs/optimizations.md
jtschuster Jul 25, 2022
59a2cc3
PR Feedback and test for virtual base type methods with default imple…
jtschuster Jul 26, 2022
2c6a512
Remove check for unusedinterfaces optimization
jtschuster Jul 26, 2022
e4ece71
Move interface PreservedScope logic to single function
jtschuster Jul 27, 2022
12486ee
Move interface PreservedScope logic to single function
jtschuster Jul 27, 2022
fe409cb
Merge branch 'markStaticVirtuals' of https://github.com/jtschuster/li…
jtschuster Jul 27, 2022
9fa904f
Update MarkStep.cs
jtschuster Jul 28, 2022
0960f91
wip
jtschuster Jul 29, 2022
513ec3a
Update src/linker/Linker.Steps/MarkStep.cs
jtschuster Jul 29, 2022
5ad5c34
Update docs/methods-kept-by-interface.md
jtschuster Jul 29, 2022
a20ef79
Update src/linker/Linker.Steps/MarkStep.cs
jtschuster Jul 29, 2022
4a2a4dc
Merge remote-tracking branch 'origin/markStaticVirtuals' into markSta…
jtschuster Jul 29, 2022
0d6cef7
PR Feedback
jtschuster Jul 29, 2022
8bd3d7c
Formatting and link PR to commented ExpectedWarning
jtschuster Jul 29, 2022
b9ac22c
Add test for base provides static interface method
jtschuster Jul 31, 2022
f9f4c38
Remove additions from another branch
jtschuster Aug 1, 2022
1d0bddc
Remove commented code
jtschuster Aug 1, 2022
4f1252d
formatting
jtschuster Aug 1, 2022
47cb5da
Merge branch 'main' into markStaticVirtuals
jtschuster Aug 1, 2022
cc870cb
Update docs
jtschuster Aug 3, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 145 additions & 0 deletions docs/methods-kept-by-interface.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
# Interface Implementation Methods Marking
#### (Does this method need to be kept due to the interface method it overrides)

The following behavior is expected for interface methods. This logic could be used to begin marking and sweeping the `.Override` of a method since if the method isn't a dependency due to the interface/base type, we should be able to remove the methodImpl. Right now, the methodImpl is always kept if both the interface method and overriding method is kept, but that isn't always necessary.

Whether or not a method implementing an interface method is required due to the _interface_ is affected by the following cases / possibilities (the method could still be kept for other reasons):
- Base method is abstract or has a default implementation (`virtual` in C#)
- Method is Instance or Static
- Linker is in library mode or exe mode
- Implementing type is relevant to variant casting or not
- Relevant to variant casting means the type token appears, the type is passed as a type argument or array type, or is reflected over.
- Base method is marked as used or not
- Base method is from preserved scope or not
- Implementing type is marked as instantiated or not
- Interface Implementation is marked or not

Note that in library mode, interface methods that can be accessed by COM or native code are marked by the linker.

### If Linker is in library mode, mark the implementation method
sbomer marked this conversation as resolved.
Show resolved Hide resolved
sbomer marked this conversation as resolved.
Show resolved Hide resolved
All interfaces and interface methods should be kept for library mode. COM in the runtime library may expect the interfaces to exist, so we should keep them.

Cases left (bold means we know it is only one of the possible options now):
- Base method is abstract or has a default implementation
- Method is Instance or Static
- Linker is in library mode or exe mode
- Implementing type is relevant to variant casting or not
- Base method is marked as used or not
- Base method from preserved scope or not
- Implementing type is marked as instantiated or not
- Interface Implementation is marked

### If the interface implementation is not marked, do not mark the implementation method
A type that doesn't implement the interface isn't required to have methods that implement the interface.

Cases left (bold means we know it is only one of the possible options now):
- Base method is abstract or has a default implementation
- Method is Instance or Static
- Linker is in library mode or exe mode
- Implementing type is relevant to variant casting or not
- Base method is marked as used or not
- Base method from preserved scope or not
- Implementing type is marked as instantiated or not
- __Interface Implementation is marked__

### If the interface method is not marked and the interface doesn't come from a preserved scope, do not mark the implementation method
Unmarked interface methods from `link` assemblies will be removed so the implementing method does not need to be kept.

Cases left:
- Base method is abstract or has a default implementation
- Method is Instance or Static
- Linker is in library mode or exe mode
- Implementing type is relevant to variant casting or not
- ~~Base method is marked as used or not~~
- ~~Base method from preserved scope or not~~
- _Base method is either marked as used or from preserved scope (combine above)_
- Implementing type is marked as instantiated or not
- __Interface Implementation is marked__

### If the interface method is abstract, mark the implementation method
The method is needed for valid IL.

Cases left:
- __Base method has a default implementation__
- Method is Instance or Static
- Linker is in library mode or exe mode
- Implementing type is relevant to variant casting or not
- Base method is marked as used or from preserved scope
- Implementing type is marked as instantiated or not
- __Interface Implementation is marked__

### If the interface is from a preserved scope and the linker is in library mode, we should treat the base method as marked
sbomer marked this conversation as resolved.
Show resolved Hide resolved
#### If the method is static, mark the implementation method
An application may use this method through a constrained type parameter
#### If the method is an instance method, and the type is instantiated or has a non-private constructor, mark the implementation method
An application can create an instance and call the method through the interface

All other behaviors are the same regardless of whether or not the linker is in library or exe mode, or whether or not the interface is in a preserved scope.

Cases left:
- __Base method has a default implementation__
- Method is Instance or Static
- ~~Linker is in library mode or exe mode~~
- Implementing type is relevant to variant casting or not
- Base method is marked as used _or not_ ~~or from preserved scope~~
- Implementing type is marked as instantiated or not
- __Interface Implementation is marked__

### If the interface method is not marked, do not mark the implementation method
We know the method cannot be called if it is not marked.

Cases left:
- __Base method has a default implementation__
- Method is Instance or Static
- Implementing type is relevant to variant casting or not
- __Base method is marked as used__
- Implementing type is marked as instantiated or not
- __Interface Implementation is marked__

### If the method is static and the implementing type is relevant to variant casting, mark the implementation method. If the method is static and the implementing type is not relevant to variant casting, do not mark the implementation method.
A static method may only be called through a constrained call if the type is relevant to variant casting.

Cases left:
- __Base method has a default implementation__
- __Method is Instance__
- Implementing type is relevant to variant casting or not
- __Base method is marked as used__
- Implementing type is marked as instantiated or not
- __Interface Implementation is marked__

Instance methods are not affected by whether or not it's relevant to variant casting

Cases left:
- __Base method has a default implementation__
- __Method is Instance__
-~~Implementing type is relevant to variant casting or not~~
- __Base method is marked as used__
- Implementing type is marked as instantiated or not
- __Interface Implementation is marked__


### If the implementing type is marked as not instantiated, do not mark the implemetation method. If the implementing type is marked as instantiated, mark the implementation method.

This should cover all the cases, but let me know if there are cases I don't mention or factors that should affect this. I don't think all of this behavior are all implemented and more tests need to be made to check each of these cases.

Summary:

if __Interface Implementation is not marked__ then do not mark the implementation method.

else if __Base method is marked as not used__ AND __Interface is not from preserved scope__ do not mark the implementation method

else if __Base method does not have a default implementation__ then mark the implementation method

else if __Interface is from preserved scope__ AND __Linker is in library mode__ AND __Method is static__ then mark the implementation method

else if __Interface is from preserved scope__ AND __Linker is in library mode__ AND __Method is instance__ AND __Implementing type is marked as instantiated__ then mark the implementing method

else if __Interface is from preserved scope__ AND __Base method is marked as not used__ then do not mark the implementation method

else if __Method is Static__ AND __Implementing type is relevant to variant casting__ then mark the implementation method

else if __Method is Static__ AND __Implementing type is not relevant to variant casting__ then do not mark the implementation method

else if __Method is marked as instantiated__ then mark the implementing method

else do not mark the implementation method
5 changes: 5 additions & 0 deletions docs/optimizations.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Optimizations definitions

## `unusedinterfaces` optimization

The `unusedinterfaces` optimization controls whether or not trimming may remove the `interfaceimpl` annotation which denotes whether a class implements an interface. When the optimization is off, the linker will not remove the annotations regardless of the visibility of the interface (even private interface implementations will be kept). However, unused methods from interfaces may still be removed, as well as `.override` directives from methods that implement an interface method. When the optimization is on and the linker can provably determine that an interface will not be used on a type, the annotation will be removed. In order to know whether it is safe to remove an interface implementation, it is necessary to have a "global" view of the code. In other words, if an assembly is passed without selecting `link` for the `action` option for the linker, all classes that implement interfaces from that assembly will keep those interface implementation annotations. For example, if you implement `System.IFormattable` from the `System.Runtime` assembly, but pass the assembly with `--action copy System.Runtime`, the interface implementation will be kept even if your code doesn't use it.
71 changes: 0 additions & 71 deletions docs/removal-behavior.md

This file was deleted.

2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<MicrosoftDotNetApiCompatVersion>7.0.0-beta.22327.2</MicrosoftDotNetApiCompatVersion>
<MicrosoftDotNetCodeAnalysisVersion>6.0.0-beta.21271.1</MicrosoftDotNetCodeAnalysisVersion>
<MicrosoftCodeAnalysisCSharpCodeStyleVersion>3.10.0-2.final</MicrosoftCodeAnalysisCSharpCodeStyleVersion>
<MicrosoftCodeAnalysisVersion>4.3.0-1.22206.2</MicrosoftCodeAnalysisVersion>
<MicrosoftCodeAnalysisVersion>4.3.0-2.final</MicrosoftCodeAnalysisVersion>
<MicrosoftCodeAnalysisCSharpAnalyzerTestingXunitVersion>1.0.1-beta1.*</MicrosoftCodeAnalysisCSharpAnalyzerTestingXunitVersion>
<MicrosoftCodeAnalysisBannedApiAnalyzersVersion>3.3.2</MicrosoftCodeAnalysisBannedApiAnalyzersVersion>
<!-- This controls the version of the cecil package, or the version of cecil in the project graph
Expand Down
Loading