Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,8 @@ private NamedTypeSymbol ConstructNamedType(

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);
Copy link
Member

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?

Copy link
Member Author

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)

}

type = (NamedTypeSymbol)TupleTypeSymbol.TransformToTupleIfCompatible(type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,27 @@ internal sealed class Conversions : ConversionsBase
private readonly Binder _binder;

public Conversions(Binder binder)
: this(binder, currentRecursionDepth: 0, includeNullability: false)
: this(binder, currentRecursionDepth: 0, includeNullability: false, otherNullabilityOpt: null)
{
}

private Conversions(Binder binder, int currentRecursionDepth, bool includeNullability)
: base(binder.Compilation.Assembly.CorLibrary, currentRecursionDepth, includeNullability)
private Conversions(Binder binder, int currentRecursionDepth, bool includeNullability, Conversions otherNullabilityOpt)
: base(binder.Compilation.Assembly.CorLibrary, currentRecursionDepth, includeNullability, otherNullabilityOpt)
{
_binder = binder;
}

protected override ConversionsBase CreateInstance(int currentRecursionDepth)
{
return new Conversions(_binder, currentRecursionDepth, IncludeNullability);
return new Conversions(_binder, currentRecursionDepth, IncludeNullability, otherNullabilityOpt: null);
}

private CSharpCompilation Compilation { get { return _binder.Compilation; } }

internal Conversions WithNullability(bool includeNullability)
protected override ConversionsBase WithNullabilityCore(bool includeNullability)
{
return (IncludeNullability == includeNullability) ? this : new Conversions(_binder, currentRecursionDepth, includeNullability);
Debug.Assert(IncludeNullability != includeNullability);
return new Conversions(_binder, currentRecursionDepth, includeNullability, this);
}

public override Conversion GetMethodGroupConversion(BoundMethodGroup source, TypeSymbol destination, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,45 @@ internal abstract partial class ConversionsBase

internal readonly bool IncludeNullability;

protected ConversionsBase(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability)
/// <summary>
/// An optional clone of this instance with distinct IncludeNullability.
/// Used to avoid unnecessary allocations when calling WithNullability() repeatedly.
/// </summary>
private ConversionsBase _lazyOtherNullability;

protected ConversionsBase(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability, ConversionsBase otherNullabilityOpt)
{
Debug.Assert((object)corLibrary != null);
Debug.Assert(otherNullabilityOpt == null || includeNullability != otherNullabilityOpt.IncludeNullability);
Debug.Assert(otherNullabilityOpt == null || currentRecursionDepth == otherNullabilityOpt.currentRecursionDepth);

this.corLibrary = corLibrary;
this.currentRecursionDepth = currentRecursionDepth;
IncludeNullability = includeNullability;
_lazyOtherNullability = otherNullabilityOpt;
}

/// <summary>
/// Returns this instance if includeNullability is correct, and returns a
/// cached clone of this instance with distinct includeNullability otherwise.
/// </summary>
internal ConversionsBase WithNullability(bool includeNullability)
{
if (IncludeNullability == includeNullability)
{
return this;
}
if (_lazyOtherNullability == null)
{
_lazyOtherNullability = WithNullabilityCore(includeNullability);
}
Debug.Assert(_lazyOtherNullability.IncludeNullability == includeNullability);
Debug.Assert(_lazyOtherNullability._lazyOtherNullability == this);
return _lazyOtherNullability;
}

protected abstract ConversionsBase WithNullabilityCore(bool includeNullability);

public abstract Conversion GetMethodGroupConversion(BoundMethodGroup source, TypeSymbol destination, ref HashSet<DiagnosticInfo> useSiteDiagnostics);

public abstract Conversion GetStackAllocConversion(BoundStackAllocArrayCreation sourceExpression, TypeSymbol destination, ref HashSet<DiagnosticInfo> useSiteDiagnostics);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,31 @@
using Roslyn.Utilities;
using System;
using System.Collections.Generic;
using System.Diagnostics;

namespace Microsoft.CodeAnalysis.CSharp
{
internal sealed class TypeConversions : ConversionsBase
{
public TypeConversions(AssemblySymbol corLibrary)
: this(corLibrary, currentRecursionDepth: 0, includeNullability: false)
: this(corLibrary, currentRecursionDepth: 0, includeNullability: false, otherNullabilityOpt: null)
{
}

private TypeConversions(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability)
: base(corLibrary, currentRecursionDepth, includeNullability)
private TypeConversions(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability, TypeConversions otherNullabilityOpt)
: base(corLibrary, currentRecursionDepth, includeNullability, otherNullabilityOpt)
{
}

protected override ConversionsBase CreateInstance(int currentRecursionDepth)
{
return new TypeConversions(this.corLibrary, currentRecursionDepth, IncludeNullability);
return new TypeConversions(this.corLibrary, currentRecursionDepth, IncludeNullability, otherNullabilityOpt: null);
}

protected override ConversionsBase WithNullabilityCore(bool includeNullability)
{
Debug.Assert(IncludeNullability != includeNullability);
return new TypeConversions(corLibrary, currentRecursionDepth, includeNullability, this);
}

public override Conversion GetMethodGroupConversion(BoundMethodGroup source, TypeSymbol destination, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ private bool FailsConstraintChecks(MethodSymbol method, out ArrayBuilder<TypePar
this.Conversions,
this.Compilation,
diagnosticsBuilder,
warningsBuilderOpt: null,
ref useSiteDiagnosticsBuilder);

if (!constraintsSatisfied)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public TypeSymbolWithAnnotations GetInferredReturnType(ref HashSet<DiagnosticInf
var diagnostics = DiagnosticBag.GetInstance();
var delegateType = Type.GetDelegateType();
var compilation = Binder.Compilation;
var conversions = Binder.Conversions.WithNullability(includeNullability: true);
var conversions = (Conversions)Binder.Conversions.WithNullability(includeNullability: true);
NullableWalker.Analyze(compilation, lambda: this, diagnostics, delegateInvokeMethod: delegateType?.DelegateInvokeMethod, returnTypes: returnTypes, initialState: nullableState);
diagnostics.Free();
var inferredReturnType = InferReturnType(returnTypes, compilation, conversions, delegateType, Symbol.IsAsync);
Expand Down
18 changes: 18 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -5363,6 +5363,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_AnnotationDisallowedInObjectCreation" xml:space="preserve">
<value>Cannot use a nullable reference type in object creation.</value>
</data>
<data name="WRN_NullabilityMismatchInTypeParameterConstraint" xml:space="preserve">
<value>The type '{3}' cannot be used as type parameter '{2}' in the generic type or method '{0}'. Nullability of type argument '{3}' doesn't match constraint type '{1}'.</value>
</data>
<data name="WRN_NullabilityMismatchInTypeParameterConstraint_Title" xml:space="preserve">
<value>The type cannot be used as type parameter in the generic type or method. Nullability of type argument doesn't match constraint type.</value>
</data>
<data name="ERR_ExplicitNullableAttribute" xml:space="preserve">
<value>Explicit application of 'System.Runtime.CompilerServices.NullableAttribute' is not allowed.</value>
</data>
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1615,6 +1615,7 @@ internal enum ErrorCode
ERR_AnnotationDisallowedInObjectCreation = 8628,
WRN_MissingNonNullTypesContext = 8629,
ERR_NonNullTypesNotAvailable = 8630,
WRN_NullabilityMismatchInTypeParameterConstraint = 8631,
}
// Note: you will need to re-generate compiler code after adding warnings (build\scripts\generate-compiler-code.cmd)
}
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_NullabilityMismatchInParameterTypeOfTargetDelegate:
case ErrorCode.WRN_NullAsNonNullable:
case ErrorCode.WRN_NoBestNullabilityConditionalExpression:
case ErrorCode.WRN_NullabilityMismatchInTypeParameterConstraint:
case ErrorCode.WRN_Experimental:
case ErrorCode.WRN_AttributesOnBackingFieldsNotAvailable:
case ErrorCode.WRN_TupleBinopLiteralNameMismatch:
Expand Down
38 changes: 33 additions & 5 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ private NullableWalker(
// If so, are we interested in an InMethodBinder specifically?
_binder = compilation.GetBinderFactory(node.SyntaxTree).GetBinder(node.Syntax);
Debug.Assert(!_binder.Conversions.IncludeNullability);
_conversions = _binder.Conversions.WithNullability(true);
_conversions = (Conversions)_binder.Conversions.WithNullability(true);
_useMethodSignatureReturnType = (object)methodSignatureOpt != null && useMethodSignatureReturnType;
_useMethodSignatureParameterTypes = (object)methodSignatureOpt != null && useMethodSignatureParameterTypes;
_methodSignatureOpt = methodSignatureOpt;
Expand Down Expand Up @@ -2047,10 +2047,18 @@ private MethodSymbol VisitArguments(
// We do a first pass to work through the arguments without making any assumptions
ImmutableArray<TypeSymbolWithAnnotations> results = VisitArgumentsEvaluate(arguments, refKindsOpt);

if ((object)method != null && method.IsGenericMethod && HasImplicitTypeArguments(node))
if ((object)method != null && method.IsGenericMethod)
{
method = InferMethodTypeArguments((BoundCall)node, method, GetArgumentsForMethodTypeInference(arguments, results));
parameters = method.Parameters;
if (HasImplicitTypeArguments(node))
{
method = InferMethodTypeArguments((BoundCall)node, method, GetArgumentsForMethodTypeInference(arguments, results));
parameters = method.Parameters;
}
if (!method.IsDefinition)
{
var syntax = node.Syntax;
CheckMethodConstraints((syntax as InvocationExpressionSyntax)?.Expression ?? syntax, method);
}
}

// PROTOTYPE(NullableReferenceTypes): Can we handle some error cases?
Expand Down Expand Up @@ -2489,7 +2497,6 @@ private MethodSymbol InferMethodTypeArguments(BoundCall node, MethodSymbol metho
ref useSiteDiagnostics);
if (result.Success)
{
// PROTOTYPE(NullableReferenceTypes): Check constraints
return definition.Construct(result.InferredTypeArguments);
}
return method;
Expand Down Expand Up @@ -2532,6 +2539,27 @@ BoundExpression getArgumentForMethodTypeInference(BoundExpression argument, Type
}
}

private void CheckMethodConstraints(SyntaxNode syntax, MethodSymbol method)
{
var diagnosticsBuilder = ArrayBuilder<TypeParameterDiagnosticInfo>.GetInstance();
var warningsBuilder = ArrayBuilder<TypeParameterDiagnosticInfo>.GetInstance();
ArrayBuilder<TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder = null;
ConstraintsHelper.CheckMethodConstraints(
method,
_conversions,
compilation,
diagnosticsBuilder,
warningsBuilder,
ref useSiteDiagnosticsBuilder);
foreach (var pair in warningsBuilder)
{
Diagnostics.Add(pair.DiagnosticInfo, syntax.Location);
}
useSiteDiagnosticsBuilder?.Free();
warningsBuilder.Free();
diagnosticsBuilder.Free();
}

private void ReplayReadsAndWrites(LocalFunctionSymbol localFunc,
SyntaxNode syntax,
bool writes)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ public static bool IsWarning(ErrorCode code)
case ErrorCode.WRN_NullAsNonNullable:
case ErrorCode.WRN_NoBestNullabilityConditionalExpression:
case ErrorCode.WRN_MissingNonNullTypesContext:
case ErrorCode.WRN_NullabilityMismatchInTypeParameterConstraint:
return true;
default:
return false;
Expand Down
Loading