-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Include nullability in CheckConstraints #28959
Include nullability in CheckConstraints #28959
Conversation
…s' into check-constraints
@dotnet/roslyn-compiler please review. |
@cston test failure appears legitimate |
@@ -14,6 +14,18 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests | |||
{ | |||
public class AttributeTests_Nullable : CSharpTestBase | |||
{ | |||
private const string NonNullTypesAttributesDefinition = @" |
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.
NonNullTypesAttributesDefinition [](start = 29, length = 32)
We should consider making this available on CSharpTestBase, it's likely that many more tests will need the attribute.
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.
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "F(x, y)").WithLocation(7, 9)); | ||
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "F(x, y)").WithLocation(7, 9), | ||
// (8,9): warning CS8631: The type 'C?' cannot be used as type parameter 'U' in the generic type or method 'C.F<T, U>(T, U)'. Nullability of type argument 'C?' doesn't match constraint type 'C'. | ||
// F(y, x).ToString(); |
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.
Consider adding some comments to the test to help read the errors. #Resolved
{ | ||
Debug.Assert((object)corLibrary != null); | ||
Debug.Assert(otherNullabilityOpt == null || includeNullability == !otherNullabilityOpt.IncludeNullability); |
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.
Why not just !=
? #Closed
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.
Done review pass (commit 4)
@jcouv, @dotnet/roslyn-compiler, please review. |
…s' into check-constraints
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 (commit 7)
@@ -1166,7 +1166,8 @@ private NamedTypeSymbol ConstructNamedTypeUnlessTypeArgumentOmitted(SyntaxNode t | |||
|
|||
if (ShouldCheckConstraints) | |||
{ | |||
type.CheckConstraintsForNonTuple(this.Conversions, typeSyntax, typeArgumentsSyntax, this.Compilation, basesBeingResolved, diagnostics); | |||
bool includeNullability = Compilation.IsFeatureEnabled(MessageID.IDS_FeatureStaticNullChecking); | |||
type.CheckConstraintsForNonTuple(this.Conversions.WithNullability(includeNullability), typeSyntax, typeArgumentsSyntax, this.Compilation, basesBeingResolved, diagnostics); |
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.
CheckConstraintsForNonTuple [](start = 21, length = 27)
Why are tuples vs. non-tuples relevant here?
Do we need to test a ValueTuple type with constraints?
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 is not specific to nullable - this call is in master. And it looks like the transform to TupleTypeSymbol
occurs below, after constraints are checked.
In reply to: 207374410 [](ancestors = 207374410)
protected ConversionsBase(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability) | ||
private ConversionsBase _lazyOtherNullability; | ||
|
||
protected ConversionsBase(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability, ConversionsBase otherNullabilityOpt) |
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.
ConversionsBase [](start = 18, length = 15)
xml doc would be useful
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.
} | ||
|
||
internal ConversionsBase WithNullability(bool includeNullability) |
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.
WithNullability [](start = 33, length = 15)
doc on this would be useful too, since there is some trickery to minimize allocations.
Maybe also on the _lazyOtherNullability
field. #Resolved
@@ -797,6 +800,7 @@ private static bool HasDuplicateInterfaces(NamedTypeSymbol type, ConsList<Symbol | |||
/// <param name="typeArguments">Containing symbol type arguments.</param> | |||
/// <param name="currentCompilation">Improves error message detail.</param> | |||
/// <param name="diagnosticsBuilder">Diagnostics.</param> | |||
/// <param name="warningsBuilderOpt">Nullability warnings.</param> |
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.
warningsBuilderOpt [](start = 25, length = 18)
Why do we need to keep warnings separate from the rest of diagnostics?
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.
To allow NullableWalker
to drop all but the nullability diagnostics.
In reply to: 207376397 [](ancestors = 207376397)
@@ -4177,12 +4196,11 @@ public class Class<T> : Base<T> where T : class | |||
comp.VerifyDiagnostics(); | |||
} | |||
|
|||
[Fact(Skip = "PROTOTYPE(NullableReferenceTypes): hits an assertion in AsObliviousReferenceType")] |
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.
Skip [](start = 14, length = 4)
Thanks! It looks like I'd forgotten to unskip a bunch of NNT tests.
Do we have tests where the nullability warning occurs within the constraints declaration itself? class C<TBase, TDerived> where TDerived : TBase { }
class D<U> where U : C<U, U?> { } // warn Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:33961 in 1206b86. [](commit_id = 1206b86, deletion_comment = False) |
Debug.Assert(conversions.IncludeNullability); | ||
if (!SatisfiesConstraintType(conversions, typeArgument, constraintType, ref useSiteDiagnostics)) | ||
{ | ||
warningsBuilderOpt.Add(new TypeParameterDiagnosticInfo(typeParameter, new CSDiagnosticInfo(ErrorCode.WRN_NullabilityMismatchInTypeParameterConstraint, containingSymbol.ConstructedFrom(), constraintType, typeParameter, typeArgument))); |
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.
warningsBuilderOpt [](start = 28, length = 18)
nit: Consider breaking long line #Resolved
@@ -1003,7 +1018,8 @@ private static bool HasDuplicateInterfaces(NamedTypeSymbol type, ConsList<Symbol | |||
|
|||
// "An identity conversion (6.1.1). | |||
// An implicit reference conversion (6.1.6). ..." | |||
if (conversions.HasIdentityOrImplicitReferenceConversion(typeArgument.TypeSymbol, constraintType.TypeSymbol, ref useSiteDiagnostics)) | |||
if ((!conversions.IncludeNullability || ConversionsBase.HasTopLevelNullabilityImplicitConversion(typeArgument, constraintType)) && |
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.
if [](start = 12, length = 2)
Could you add a comment? #Resolved
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.
Done with review pass (iteration 7). Mostly some documentation requests. Thanks
… into unconstrained-type-parameter-nullability * dotnet/features/NullableReferenceTypes: Include nullability in CheckConstraints (dotnet#28959) Use struct for TypeSymbolWithAnnotations (dotnet#28943) Publish Linux Test Results Force retest Migrate Linux tests to VSTS Port determinism fixes Make sure TLS 1.2 is used to fetch from https://dot.net Move to language version 8 Make mutex creation more robust Disable icacls use on Helix Disable leak detection tests on x64 VSTS YAML file
No description provided.