-
Notifications
You must be signed in to change notification settings - Fork 128
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
[feature/dataflow] Port Expression.* pattern recognition to new infra #1039
[feature/dataflow] Port Expression.* pattern recognition to new infra #1039
Conversation
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.
The uniqueness problem in the tests is the most important comment - that needs to be fixed.
But otherwise it looks good.
src/linker/Linker.Steps/MarkStep.cs
Outdated
} else if (stringParam is NullValue) { | ||
reflectionContext.RecordHandledPattern (); | ||
} else if (stringParam is MethodParameterValue) { | ||
// TODO: Check if parameter is annotated. |
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.
Method name parameters won't have annotations, currently the plan is to only have annotations for Type or String-representing-a-type parameters.
The right question here is whether we should fallback to including all methods on the type or not. See #988 (comment) for some discussion on the topic.
For now - could you please create a new issue for this problem and leave a TODO in the code describing the problem?
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.
Similar comment applies to Field
and Property
- although in those cases I think we're pretty sure we want to fall back to keeping all fields/properties.
if (value is SystemTypeValue systemTypeValue) { | ||
foreach (var stringParam in methodParams [1].UniqueValues ()) { | ||
if (stringParam is KnownStringValue stringValue) { | ||
MarkMethodsFromReflectionCall (ref reflectionContext, systemTypeValue.TypeRepresented, stringValue.Contents, BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic); |
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.
Related note: This should also work on methods on a base class - see #1042. No need to address that in this PR, but maybe as a follow-up change we could fix it here as well.
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.
Same comment applies to Property
and Field
.
test/Mono.Linker.Tests.Cases/Reflection/ExpressionCallString.cs
Outdated
Show resolved
Hide resolved
test/Mono.Linker.Tests.Cases/Reflection/ExpressionFieldString.cs
Outdated
Show resolved
Hide resolved
test/Mono.Linker.Tests.Cases/Reflection/ExpressionPropertyString.cs
Outdated
Show resolved
Hide resolved
f257ef9
to
94da209
Compare
src/linker/Linker.Steps/MarkStep.cs
Outdated
|
||
foreach (var value in methodParams [0].UniqueValues ()) { | ||
if (value is SystemTypeValue systemTypeValue) | ||
MarkMethodsFromReflectionCall (ref reflectionContext, systemTypeValue.TypeRepresented, ".ctor", BindingFlags.Instance, parametersCount: 0); |
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.
Technically this should include Public | NonPublic - since the runtime version will handle both cases.
[feature/dataflow] Port Expression.* pattern recognition to new infra Commit migrated from dotnet/linker@4a33d67
No description provided.