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

Treat unconstrained type parameters declared in #nullable disable context as having an unknown nullability in case they are substituted with a reference type. #34797

Merged
merged 12 commits into from
Apr 16, 2019
7 changes: 6 additions & 1 deletion docs/features/nullable-reference-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ Trivial/optimized cases:

NullableAttribute(1) should be placed on a type parameter definition that has a `class!` constraint.
NullableAttribute(2) should be placed on a type parameter definition that has a `class?` constraint.
NullableAttribute(2) should be placed on a type parameter definition that has no type constraints, `class`, `struct` and `unmanaged` constraints and
is declared in a context where nullable type annotations are allowed, that is eqivalent to having an `object?` constraint.
Other forms of NullableAttribute are not emitted on type parameter definitions and are not specially recognized on them.

The `NullableAttribute` type declaration is synthesized by the compiler if it is not included in the compilation, but is needed to produce the output.
Expand Down Expand Up @@ -267,7 +269,10 @@ A `class?` constraint is allowed, which, like class, requires the type argument

An explicit `object` (or `System.Object`) constraint is allowed, which requires the type to be non-nullable when it is a reference type.
However, an explicit `object?` constraint is not allowed.
An unconstrained type parameter is essentially equivalent to one constrained by `object?`.
An unconstrained (here it means - no type constraints, and no `class`, `struct`, or `unmanaged` constraints) type parameter is essentially
equivalent to one constrained by `object?` when it is declared in a context where nullable annotations are enabled. If annotations are disabled,
the type parameter is essentially equivalent to one constrained by `object~`. The context is determined at the identifier that declares the type
parameter within a type parameter list.
[4/25/18](https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-04-25.md)
Note, the `object`/`System.Object` constraint is represented in metadata as any other type constraint, the type is System.Object.

Expand Down
30 changes: 27 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Constraints.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ internal partial class Binder
internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstraintClauses(
Symbol containingSymbol,
ImmutableArray<TypeParameterSymbol> typeParameters,
TypeParameterListSyntax typeParameterList,
SyntaxList<TypeParameterConstraintClauseSyntax> clauses,
DiagnosticBag diagnostics)
{
Expand Down Expand Up @@ -58,7 +59,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai
Debug.Assert(ordinal >= 0);
Debug.Assert(ordinal < n);

var constraintClause = this.BindTypeParameterConstraints(name, clause, diagnostics);
var constraintClause = this.BindTypeParameterConstraints(typeParameterList.Parameters[ordinal], clause, diagnostics);
if (results[ordinal] == null)
{
results[ordinal] = constraintClause;
Expand Down Expand Up @@ -96,21 +97,23 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai
/// <summary>
/// Bind and return a single type parameter constraint clause.
/// </summary>
private TypeParameterConstraintClause BindTypeParameterConstraints(
string name, TypeParameterConstraintClauseSyntax constraintClauseSyntax, DiagnosticBag diagnostics)
private TypeParameterConstraintClause BindTypeParameterConstraints(TypeParameterSyntax typeParameterSyntax, TypeParameterConstraintClauseSyntax constraintClauseSyntax, DiagnosticBag diagnostics)
{
var constraints = TypeParameterConstraintKind.None;
var constraintTypes = ArrayBuilder<TypeWithAnnotations>.GetInstance();
var syntaxBuilder = ArrayBuilder<TypeConstraintSyntax>.GetInstance();
SeparatedSyntaxList<TypeParameterConstraintSyntax> constraintsSyntax = constraintClauseSyntax.Constraints;
Debug.Assert(!InExecutableBinder); // Cannot eagerly report diagnostics handled by LazyMissingNonNullTypesContextDiagnosticInfo
bool hasTypeLikeConstraint = false;

for (int i = 0, n = constraintsSyntax.Count; i < n; i++)
{
var syntax = constraintsSyntax[i];
switch (syntax.Kind())
{
case SyntaxKind.ClassConstraint:
hasTypeLikeConstraint = true;

if (i != 0)
{
diagnostics.Add(ErrorCode.ERR_RefValBoundMustBeFirst, syntax.GetFirstToken().GetLocation());
Expand All @@ -135,6 +138,8 @@ private TypeParameterConstraintClause BindTypeParameterConstraints(

continue;
case SyntaxKind.StructConstraint:
hasTypeLikeConstraint = true;

if (i != 0)
{
diagnostics.Add(ErrorCode.ERR_RefValBoundMustBeFirst, syntax.GetFirstToken().GetLocation());
Expand All @@ -161,6 +166,8 @@ private TypeParameterConstraintClause BindTypeParameterConstraints(
continue;
case SyntaxKind.TypeConstraint:
{
hasTypeLikeConstraint = true;

var typeConstraintSyntax = (TypeConstraintSyntax)syntax;
var typeSyntax = typeConstraintSyntax.Type;
var typeSyntaxKind = typeSyntax.Kind();
Expand Down Expand Up @@ -207,9 +214,26 @@ private TypeParameterConstraintClause BindTypeParameterConstraints(
}
}

if (!hasTypeLikeConstraint && !IsNullableEnabled(typeParameterSyntax.Identifier))
{
Copy link
Member

@gafter gafter Apr 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to have some comments somewhere about the strategy here: we add constraints to represent the default oblivious constraint where T: object~, which causes us to suppress the IL emission of the default nullable constraint to imitate the behavior of pre-nullable compilers. When reading an assembly, the absence of a constraint causes us to create the default oblivious constraint. where T: object~, whereas reading the default nullable constraint where T: object? causes us to drop it on the floor, as that is the new default. Adjusted to correct any misunderstanding I may have. #Resolved

constraints |= TypeParameterConstraintKind.ObliviousNullabilityIfReferenceType;
}

return TypeParameterConstraintClause.Create(constraints, constraintTypes.ToImmutableAndFree(), syntaxBuilder.ToImmutableAndFree());
}

internal ImmutableArray<TypeParameterConstraintClause> GetDefaultTypeParameterConstraintClauses(TypeParameterListSyntax typeParameterList)
{
var builder = ArrayBuilder<TypeParameterConstraintClause>.GetInstance(typeParameterList.Parameters.Count);

foreach (TypeParameterSyntax typeParameterSyntax in typeParameterList.Parameters)
{
builder.Add(IsNullableEnabled(typeParameterSyntax.Identifier) ? TypeParameterConstraintClause.Empty : TypeParameterConstraintClause.ObliviousNullabilityIfReferenceType);
}

return builder.ToImmutableAndFree();
}

/// <summary>
/// Returns true if the constraint is valid. Otherwise
/// returns false and generates a diagnostic.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ internal override bool? ReferenceTypeConstraintIsNullable
get { return false; }
}

internal override bool? IsNotNullableIfReferenceType => null;

public override bool HasValueTypeConstraint
{
get { return false; }
Expand Down
57 changes: 35 additions & 22 deletions src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -352,21 +352,29 @@ internal static ImmutableArray<TypeParameterConstraintClause> MakeTypeParameterC
this Symbol containingSymbol,
Binder binder,
ImmutableArray<TypeParameterSymbol> typeParameters,
TypeParameterListSyntax typeParameterList,
SyntaxList<TypeParameterConstraintClauseSyntax> constraintClauses,
Location location,
DiagnosticBag diagnostics)
{
if (typeParameters.Length == 0 || constraintClauses.Count == 0)
if (typeParameters.Length == 0)
{
return ImmutableArray<TypeParameterConstraintClause>.Empty;
}

if (constraintClauses.Count == 0)
{
ImmutableArray<TypeParameterConstraintClause> defaultClauses = binder.GetDefaultTypeParameterConstraintClauses(typeParameterList);

return defaultClauses.ContainsOnlyEmptyConstraintClauses() ? ImmutableArray<TypeParameterConstraintClause>.Empty : defaultClauses;
Copy link
Member

@cston cston Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaultClauses.ContainsOnlyEmptyConstraintClauses() [](start = 23, length = 51)

Should Binder.GetDefaultTypeParameterConstraintClauses() simply return ImmutableArray<TypeParameterConstraintClause>.Empty if all are empty? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Binder.GetDefaultTypeParameterConstraintClauses() simply return ImmutableArray.Empty if all are empty?

Some consumers do not want this optimization.


In reply to: 273625867 [](ancestors = 273625867)

}

// Wrap binder from factory in a generic constraints specific binder
// to avoid checking constraints when binding type names.
Debug.Assert(!binder.Flags.Includes(BinderFlags.GenericConstraintsClause));
binder = binder.WithAdditionalFlags(BinderFlags.GenericConstraintsClause | BinderFlags.SuppressConstraintChecks);

return binder.BindTypeParameterConstraintClauses(containingSymbol, typeParameters, constraintClauses, diagnostics);
return binder.BindTypeParameterConstraintClauses(containingSymbol, typeParameters, typeParameterList, constraintClauses, diagnostics);
}

/// <summary>
Expand Down Expand Up @@ -394,32 +402,37 @@ private static TypeParameterConstraintClause MakeTypeParameterConstraintsLate(
TypeParameterConstraintClause constraintClause,
DiagnosticBag diagnostics)
{
Symbol containingSymbol = typeParameter.ContainingSymbol;

var constraintTypeBuilder = ArrayBuilder<TypeWithAnnotations>.GetInstance();
var constraintTypes = constraintClause.ConstraintTypes;
int n = constraintTypes.Length;
for (int i = 0; i < n; i++)

if (!constraintClause.TypeConstraintsSyntax.IsDefault)
{
var constraintType = constraintTypes[i];
var syntax = constraintClause.TypeConstraintsSyntax[i];
// Only valid constraint types are included in ConstraintTypes
// since, in general, it may be difficult to support all invalid types.
// In the future, we may want to include some invalid types
// though so the public binding API has the most information.
if (Binder.IsValidConstraint(typeParameter.Name, syntax, constraintType, constraintClause.Constraints, constraintTypeBuilder, diagnostics))
Symbol containingSymbol = typeParameter.ContainingSymbol;

var constraintTypeBuilder = ArrayBuilder<TypeWithAnnotations>.GetInstance();
int n = constraintTypes.Length;

for (int i = 0; i < n; i++)
{
containingSymbol.CheckConstraintTypeVisibility(syntax.Location, constraintType, diagnostics);
constraintTypeBuilder.Add(constraintType);
var constraintType = constraintTypes[i];
var syntax = constraintClause.TypeConstraintsSyntax[i];
// Only valid constraint types are included in ConstraintTypes
// since, in general, it may be difficult to support all invalid types.
// In the future, we may want to include some invalid types
// though so the public binding API has the most information.
if (Binder.IsValidConstraint(typeParameter.Name, syntax, constraintType, constraintClause.Constraints, constraintTypeBuilder, diagnostics))
{
containingSymbol.CheckConstraintTypeVisibility(syntax.Location, constraintType, diagnostics);
constraintTypeBuilder.Add(constraintType);
}
}
}

if (constraintTypeBuilder.Count < n)
{
constraintTypes = constraintTypeBuilder.ToImmutable();
}
if (constraintTypeBuilder.Count < n)
{
constraintTypes = constraintTypeBuilder.ToImmutable();
}

constraintTypeBuilder.Free();
constraintTypeBuilder.Free();
}

// Verify constraints on any other partial declarations.
foreach (var otherClause in constraintClause.OtherPartialDeclarations)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ internal override bool? ReferenceTypeConstraintIsNullable
}
}

internal override bool? IsNotNullableIfReferenceType => null;

public override bool HasValueTypeConstraint
{
get
Expand Down
16 changes: 10 additions & 6 deletions src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -609,14 +609,15 @@ private static bool HaveSameConstraints(Symbol member1, TypeMap typeMap1, Symbol
return HaveSameConstraints(typeParameters1, typeMap1, typeParameters2, typeMap2);
}

public static bool HaveSameConstraints(ImmutableArray<TypeParameterSymbol> typeParameters1, TypeMap typeMap1, ImmutableArray<TypeParameterSymbol> typeParameters2, TypeMap typeMap2)
public static bool HaveSameConstraints(ImmutableArray<TypeParameterSymbol> typeParameters1, TypeMap typeMap1, ImmutableArray<TypeParameterSymbol> typeParameters2, TypeMap typeMap2, bool includingNullability = false)
{
Debug.Assert(typeParameters1.Length == typeParameters2.Length);

int arity = typeParameters1.Length;
for (int i = 0; i < arity; i++)
{
if (!HaveSameConstraints(typeParameters1[i], typeMap1, typeParameters2[i], typeMap2))
if (!HaveSameConstraints(typeParameters1[i], typeMap1, typeParameters2[i], typeMap2) ||
(includingNullability && !HaveSameNullabilityInConstraints(typeParameters1[i], typeMap1, typeParameters2[i], typeMap2, unknownNullabilityMatchesAny: false)))
{
return false;
}
Expand Down Expand Up @@ -668,20 +669,23 @@ private static bool HaveSameTypeConstraints(TypeParameterSymbol typeParameter1,
AreConstraintTypesSubset(substitutedTypes2, substitutedTypes1, typeParameter1);
}

public static bool HaveSameNullabilityInConstraints(TypeParameterSymbol typeParameter1, TypeMap typeMap1, TypeParameterSymbol typeParameter2, TypeMap typeMap2)
public static bool HaveSameNullabilityInConstraints(TypeParameterSymbol typeParameter1, TypeMap typeMap1, TypeParameterSymbol typeParameter2, TypeMap typeMap2, bool unknownNullabilityMatchesAny = true)
{
if (!typeParameter1.IsValueType)
{
bool? isNotNullableIfReferenceType1 = typeParameter1.IsNotNullableIfReferenceType;
bool? isNotNullableIfReferenceType2 = typeParameter2.IsNotNullableIfReferenceType;
if (isNotNullableIfReferenceType1.HasValue && isNotNullableIfReferenceType2.HasValue &&
isNotNullableIfReferenceType1.GetValueOrDefault() != isNotNullableIfReferenceType2.GetValueOrDefault())
if (isNotNullableIfReferenceType1 != isNotNullableIfReferenceType2 &&
!(unknownNullabilityMatchesAny && (!isNotNullableIfReferenceType1.HasValue || !isNotNullableIfReferenceType2.HasValue)))
{
return false;
}
}

return HaveSameTypeConstraints(typeParameter1, typeMap1, typeParameter2, typeMap2, TypeSymbol.EqualsAllIgnoreOptionsPlusNullableWithUnknownMatchesAnyComparer);
return HaveSameTypeConstraints(typeParameter1, typeMap1, typeParameter2, typeMap2,
unknownNullabilityMatchesAny ?
TypeSymbol.EqualsAllIgnoreOptionsPlusNullableWithUnknownMatchesAnyComparer :
TypeSymbol.EqualsAllIgnoreOptionsPlusNullableComparer);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,18 +157,8 @@ private ImmutableArray<TypeWithAnnotations> GetDeclaredConstraintTypes()
}

var moduleSymbol = containingType.ContainingPEModule;
var metadataReader = moduleSymbol.Module.MetadataReader;
GenericParameterConstraintHandleCollection constraints;

try
{
constraints = metadataReader.GetGenericParameter(_handle).GetConstraints();
}
catch (BadImageFormatException)
{
constraints = default(GenericParameterConstraintHandleCollection);
Interlocked.CompareExchange(ref _lazyConstraintsUseSiteErrorInfo, new CSDiagnosticInfo(ErrorCode.ERR_BindToBogus, this), CSDiagnosticInfo.EmptyErrorInfo);
}
PEModule peModule = moduleSymbol.Module;
GenericParameterConstraintHandleCollection constraints = GetConstraintHandleCollection(peModule);

bool hasUnmanagedModreqPattern = false;

Expand All @@ -186,6 +176,7 @@ private ImmutableArray<TypeWithAnnotations> GetDeclaredConstraintTypes()
tokenDecoder = new MetadataDecoder(moduleSymbol, containingType);
}

var metadataReader = peModule.MetadataReader;
foreach (var constraintHandle in constraints)
{
var constraint = metadataReader.GetGenericParameterConstraint(constraintHandle);
Expand Down Expand Up @@ -230,7 +221,7 @@ private ImmutableArray<TypeWithAnnotations> GetDeclaredConstraintTypes()
// - presence of unmanaged pattern has to be matched with `valuetype`
// - IsUnmanagedAttribute is allowed iif there is an unmanaged pattern
if (hasUnmanagedModreqPattern && (_flags & GenericParameterAttributes.NotNullableValueTypeConstraint) == 0 ||
hasUnmanagedModreqPattern != moduleSymbol.Module.HasIsUnmanagedAttribute(_handle))
hasUnmanagedModreqPattern != peModule.HasIsUnmanagedAttribute(_handle))
{
// we do not recognize these combinations as "unmanaged"
hasUnmanagedModreqPattern = false;
Expand All @@ -244,6 +235,23 @@ private ImmutableArray<TypeWithAnnotations> GetDeclaredConstraintTypes()
return _lazyDeclaredConstraintTypes;
}

private GenericParameterConstraintHandleCollection GetConstraintHandleCollection(PEModule module)
{
GenericParameterConstraintHandleCollection constraints;

try
{
constraints = module.MetadataReader.GetGenericParameter(_handle).GetConstraints();
}
catch (BadImageFormatException)
{
constraints = default(GenericParameterConstraintHandleCollection);
Interlocked.CompareExchange(ref _lazyConstraintsUseSiteErrorInfo, new CSDiagnosticInfo(ErrorCode.ERR_BindToBogus, this), CSDiagnosticInfo.EmptyErrorInfo);
}

return constraints;
}

public override ImmutableArray<Location> Locations
{
get
Expand Down Expand Up @@ -301,6 +309,30 @@ internal override bool? ReferenceTypeConstraintIsNullable
}
}

internal override bool? IsNotNullableIfReferenceType
{
get
{
if ((_flags & (GenericParameterAttributes.NotNullableValueTypeConstraint | GenericParameterAttributes.ReferenceTypeConstraint)) == 0)
{
PEModule module = ((PEModuleSymbol)this.ContainingModule).Module;
GenericParameterConstraintHandleCollection constraints = GetConstraintHandleCollection(module);

if (constraints.Count == 0)
{
if (module.HasNullableAttribute(_handle, out byte transformFlag, out _) && (NullableAnnotation)transformFlag == NullableAnnotation.Annotated)
{
return false;
}

return null;
}
}

return CalculateIsNotNullableIfReferenceType();
}
}

public override bool HasValueTypeConstraint
{
get
Expand Down
Loading