Separate top-level and nested nullability checks in OHI #42491
Conversation
jcouv
added
Area-Compilers
New Language Feature - Nullable Reference Types
|
|
||
| Debug.Assert(conversions.IncludeNullability); | ||
| HashSet<DiagnosticInfo> discardedDiagnostics = null; | ||
| return conversions.ClassifyImplicitConversionFromType(sourceType, targetType, ref discardedDiagnostics).Kind != ConversionKind.NoConversion; |
There was a problem hiding this comment.
📝 This is the heart of this change.
HasAnyNullabilityImplicitConversion checks HasTopLevelNullabilityImplicitConversion and ClassifyImplicitConversionFromType, but we no longer need to do the top-level check.
The top-level check is done by NullableWalker.AreParameterAnnotationsCompatible, which accounts for attributes in addition to annotations from types.
|
@agocke @dotnet/roslyn-compiler for review. Thanks |
| <data name="WRN_NullabilityMismatchInReturnTypeOnExplicitImplementationBecauseOfAttributes" xml:space="preserve"> | ||
| <value>Nullability of reference types in return type doesn't match implemented member '{0}' because of nullability attributes.</value> | ||
| <data name="WRN_TopLevelNullabilityMismatchInReturnTypeOnExplicitImplementation" xml:space="preserve"> | ||
| <value>Top-level nullability of reference types in return type doesn't match implemented member '{0}' (possibly because of nullability attributes).</value> |
There was a problem hiding this comment.
Top-level nullability [](start = 11, length = 21)
Consider removing the "top-level" qualifier from these warning messages.
"Top-level nullability" is probably not a term users are familiar with, and in most cases top-level nullability is the only nullability. Even in cases where there is top-level and nested nullability, the warning messages are simply indicating a nullability mismatch so it should be sufficient to say "nullability".
ghost
modified the milestones:
Fixes #42470
Relates to PR #41336 (Report bad tightening/loosening of attributes in OHI)
I'll update the failing CI test later in the morning, but I think this PR can already be looked at. Thanks