Reflection annotate more of CoreLib #37418
Conversation
| @@ -47,6 +50,8 @@ public override bool Equals(object? obj) | |||
| return true; | |||
| } | |||
|
|
|||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern", | |||
| Justification = "Unused fields don't make a difference for hashcode quality")] | |||
There was a problem hiding this comment.
I assume we're OK with the hascode value itself changing as a result of trimming, right?
There was a problem hiding this comment.
I would think so. We can make this invariant if we keep all fields of attributes in linker, but I don't think we care so much.
Whoever cares about these hashcodes is probably already overriding this implementation (this returns the hashcode of the first non-null field and if that field also happens to be unused - that's a pretty bad hashcode).
| @@ -149,6 +149,8 @@ public virtual Type[] GetGenericParameterConstraints() | |||
| protected abstract ConstructorInfo? GetConstructorImpl(BindingFlags bindingAttr, Binder? binder, CallingConventions callConvention, Type[] types, ParameterModifier[]? modifiers); | |||
|
|
|||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] | |||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern", | |||
| Justification = "Linker doesn't recongnize GetConstructors(BindingFlags.Public) but this is safe")] | |||
There was a problem hiding this comment.
Please don't use "safe"... it resembles security, which this is not about (I know we use it a lot, but we should try to stay away from it)
| @@ -278,6 +279,8 @@ public void SetThreadPrincipal(IPrincipal principal) | |||
| } | |||
| } | |||
|
|
|||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern", | |||
There was a problem hiding this comment.
Same question on RequiresUnreferencedCode.
There was a problem hiding this comment.
Linker intrinsically recognizes all these weird overloads. This was ported from @marek-safar's original prototype. I don't think we would add them based on API usage telemetry since nobody uses these APIs.
Yes, my preference would be to make these string,string overloads RequiresUnreferencedCode, delete the linker handling, and be done with it.
Having to mark these UnconditionalSuppressMessage is unfortunate because it puts us into a situation where forgetting to intrinsically recognize this API (e.g. once we build a Roslyn analyzer) will result in potentially broken apps.
With the other annotations in this pull request, if we forget to intrinsically recognize a rare overload, it will result in more warnings, not less warnings.
There was a problem hiding this comment.
You could start with more aggressive mode and block less APIs based on previews feedback.
ghost
locked as resolved and limited conversation to collaborators
No description provided.