-
Notifications
You must be signed in to change notification settings - Fork 5k
Add trim-safe ValidationContext constructor #113426
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
Conversation
Note regarding the
|
1 similar comment
Note regarding the
|
...em.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationContext.cs
Outdated
Show resolved
Hide resolved
Don't we need to modify the options source generator with this PR too?
|
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.
Can you update this line?
Lines 181 to 182 in d15a82e
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "The ctors are marked with RequiresUnreferencedCode.")] | |
private string? GetDisplayName() |
It still supports |
/// This constructor is trim-safe because it does not use reflection to resolve | ||
/// the Type of the <paramref name="instance" /> to support setting the DisplayName. | ||
/// </remarks> | ||
public ValidationContext(object instance, string displayName, IServiceProvider? serviceProvider, IDictionary<object, object?>? items) |
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.
Which part of the other constructor forces it to have an RUC annotation? This seems to be replicating most of the statements present in the previous ctor.
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.
This code is the reason why the other 2 constructors are marked RUC:
Lines 181 to 182 in 5ff417f
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "The ctors are marked with RequiresUnreferencedCode.")] | |
private string? GetDisplayName() |
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 updated the RUC messages here to be a little bit clearer about the fact that the issue is not providing a display name forces us down the path of resolving the type from the object instance
which is not trim-safe.
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.
Pretty wild that getting the display name was the pitfall here. Very cool to overcome that, @captainsafia!
I updated the emitter to generate the different invocations behind an #ifdef. Alternatively, we could try to query for the TFM in the source generator itself and change what is emitted there but this felt easier. Edit: bleh, need to hop on a devbox to update the netfx baselines |
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.
LGTM. Thanks @captainsafia!
Resolves #113134