-
Notifications
You must be signed in to change notification settings - Fork 127
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
Don't remove unused interfaces in library mode #2711
Merged
Merged
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
0388e48
Consider all public types in a library as instantiated
vitek-karas 39ffb9f
Change the solution to rely on the optimization setting only
vitek-karas e5b3788
wip
jtschuster 61115f6
Add implicit interface implementation case
jtschuster c4ebd45
Edit tests to remove issue-specific code and names
jtschuster d7cc881
Mark members of CollectedType as kept
jtschuster 3a8b76d
Add private interface test and simplify external interface example
jtschuster 1c04ad2
Replace early exit for non-interfaces
jtschuster be37029
Merge branch 'main' into FixLibraryInterface
jtschuster 7ad5afc
License headers and use IsMethodNeededByTypeDueToPreservedScope inste…
jtschuster 07143dd
Add check for static methods before skipping virtual marking
jtschuster a7c7e13
Add more clarifying comments, move static method check, rename method
jtschuster 64692a0
Renames and comment cleanup + tests
vitek-karas 4ea2991
More tests for interface behavior
vitek-karas 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 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 |
---|---|---|
|
@@ -2287,12 +2287,18 @@ void MarkGenericParameter (GenericParameter parameter) | |
} | ||
|
||
/// <summary> | ||
/// Returns true if any of the base methods of the <paramref name="method"/> passed is in an assembly that is not trimmed (i.e. action != trim) | ||
/// Returns true if any of the base methods of the <paramref name="method"/> passed is in an assembly that is not trimmed (i.e. action != trim). | ||
/// Meant to be used to determine whether methods should be marked regardless of whether it is instantiated or not. | ||
/// </summary> | ||
/// <remarks>This ignores any base methods defined in interfaces. To also check methods defined in interfaces</remarks> | ||
/// <remarks> | ||
/// When the unusedinterfaces optimization is on, this is used to mark methods that override a virtual from a non-link assembly and must be kept. | ||
/// When the unusedinterfaces optimization is off, this will do the same as when on but will also mark interface methods from interfaces defined in a non-link assembly. | ||
/// If the containing type is instantiated, the caller should use <see cref="IsOverrideNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition)" /> | ||
/// </remarks> | ||
bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rename this to |
||
{ | ||
if (!method.IsVirtual) | ||
// Static methods may also have base methods in static interface methods. These methods are not captured by IsVirtual and must be checked separately | ||
if (!(method.IsVirtual || method.IsStatic)) | ||
return false; | ||
|
||
var base_list = Annotations.GetBaseMethods (method); | ||
|
@@ -2322,12 +2328,17 @@ bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method) | |
} | ||
|
||
/// <summary> | ||
/// Returns true if any of the base methods of <paramref name="method" /> is defined in an assembly that is not trimmed (i.e. action!=trim) | ||
/// Returns true if any of the base methods of <paramref name="method" /> is defined in an assembly that is not trimmed (i.e. action!=trim). | ||
/// This is meant to be used on methods from a type that is known to be instantiated. | ||
/// </summary> | ||
/// <remarks>This is very similar to <see cref="IsVirtualNeededByTypeDueToPreservedScope(MethodDefinition)"/>, except this also checks if the base method is an interface.</remarks> | ||
bool IsVirtualNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method) | ||
/// <remarks> | ||
/// This is very similar to <see cref="IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition)"/>, | ||
/// but will mark methods from an interface defined in a non-link assembly regardless of the optimization, and does not handle static interface methods. | ||
/// </remarks> | ||
bool IsOverrideNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method) | ||
{ | ||
if (!(method.IsVirtual || method.IsStatic)) | ||
// Any static interface methods are captured by <see cref="IsVirtualNeededByTypeDueToPreservedScope">, which should be called on all relevant methods so no need to check again here. | ||
if (!method.IsVirtual) | ||
vitek-karas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return false; | ||
|
||
var base_list = Annotations.GetBaseMethods (method); | ||
|
@@ -3126,7 +3137,7 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type | |
protected virtual IEnumerable<MethodDefinition> GetRequiredMethodsForInstantiatedType (TypeDefinition type) | ||
{ | ||
foreach (var method in type.Methods) { | ||
if (IsVirtualNeededByInstantiatedTypeDueToPreservedScope (method)) | ||
if (IsOverrideNeededByInstantiatedTypeDueToPreservedScope (method)) | ||
yield return method; | ||
} | ||
} | ||
|
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.