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.

Sign up for GitHub

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

Jump to bottom

Support Object type as a generic type constraint. #29809

Merged
merged 7 commits into from
Sep 14, 2018
Merged

Support Object type as a generic type constraint. #29809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
25fd08a
Support Object type as a generic type constraint.
AlekseyTs Sep 12, 2018
426225b
PR feedback
AlekseyTs Sep 12, 2018
941531b
Move doc comment
AlekseyTs Sep 12, 2018
f1636e4
PR feedback
AlekseyTs Sep 12, 2018
ebeea03
Merge remote-tracking branch 'dotnet/features/NullableReferenceTypes'…
AlekseyTs Sep 13, 2018
1bb5763
Follow up on merge
AlekseyTs Sep 13, 2018
00f0b8a
PR feedback
AlekseyTs Sep 13, 2018
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
7 changes: 7 additions & 0 deletions docs/features/nullable-reference-types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,13 @@ A `class?` constraint is allowed, which, like class, requires the type argument
[Nullable strawman](https://github.com/dotnet/csharplang/issues/790)
[4/25/18](https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-04-25.md)

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?`.
[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.


A warning is reported for nullable type argument for type parameter with `class` constraint or non-nullable reference type or interface type constraint.
[4/25/18](https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-04-25.md)
```c#
Expand Down
23 changes: 19 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Constraints.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ private TypeParameterConstraintClause BindTypeParameterConstraints(
var constraintTypes = ArrayBuilder<TypeSymbolWithAnnotations>.GetInstance();
var syntaxBuilder = ArrayBuilder<TypeConstraintSyntax>.GetInstance();
SeparatedSyntaxList<TypeParameterConstraintSyntax> constraintsSyntax = constraintClauseSyntax.Constraints;
Debug.Assert(!InExecutableBinder); // Cannot eagerly report diagnostics handled by LazyMissingNonNullTypesContextDiagnosticInfo

for (int i = 0, n = constraintsSyntax.Count; i < n; i++)
{
Expand All @@ -115,9 +116,11 @@ private TypeParameterConstraintClause BindTypeParameterConstraints(
diagnostics.Add(ErrorCode.ERR_RefValBoundMustBeFirst, syntax.GetFirstToken().GetLocation());
}

if (((ClassOrStructConstraintSyntax)syntax).QuestionToken.IsKind(SyntaxKind.QuestionToken))
SyntaxToken questionToken = ((ClassOrStructConstraintSyntax)syntax).QuestionToken;
if (questionToken.IsKind(SyntaxKind.QuestionToken))
{
constraints |= TypeParameterConstraintKind.NullableReferenceType;
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, type: default), questionToken.GetLocation());
}
else
{
Expand Down Expand Up @@ -198,7 +201,7 @@ private TypeParameterConstraintClause BindTypeParameterConstraints(
}
}

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

/// <summary>
Expand All @@ -213,7 +216,7 @@ internal static bool IsValidConstraint(
ArrayBuilder<TypeSymbolWithAnnotations> constraintTypes,
DiagnosticBag diagnostics)
{
if (!IsValidConstraintType(syntax, type.TypeSymbol, diagnostics))
if (!IsValidConstraintType(syntax, type, diagnostics))
{
return false;
}
Expand Down Expand Up @@ -278,8 +281,10 @@ internal static bool IsValidConstraint(
/// Returns true if the type is a valid constraint type.
/// Otherwise returns false and generates a diagnostic.
/// </summary>
private static bool IsValidConstraintType(TypeConstraintSyntax syntax, TypeSymbol type, DiagnosticBag diagnostics)
private static bool IsValidConstraintType(TypeConstraintSyntax syntax, TypeSymbolWithAnnotations typeWithAnnotations, DiagnosticBag diagnostics)
{
TypeSymbol type = typeWithAnnotations.TypeSymbol;

switch (type.SpecialType)
{
case SpecialType.System_Enum:
Expand All @@ -292,6 +297,16 @@ private static bool IsValidConstraintType(TypeConstraintSyntax syntax, TypeSymbo
break;

case SpecialType.System_Object:
if (typeWithAnnotations.IsAnnotated)
{
// "Constraint cannot be special class '{0}'"
Error(diagnostics, ErrorCode.ERR_SpecialTypeAsBound, syntax, typeWithAnnotations);
return false;
}

CheckFeatureAvailability(syntax, MessageID.IDS_FeatureObjectGenericTypeConstraint, diagnostics);
break;

case SpecialType.System_ValueType:
case SpecialType.System_Array:
// "Constraint cannot be special class '{0}'"
Expand Down
39 changes: 25 additions & 14 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,20 +343,7 @@ internal NamespaceOrTypeOrAliasSymbolWithAnnotations BindNamespaceOrTypeOrAliasS
TypeSymbolWithAnnotations typeArgument = BindType(typeArgumentSyntax, diagnostics, basesBeingResolved);
TypeSymbolWithAnnotations constructedType = typeArgument.SetIsAnnotated(Compilation);

if (InExecutableBinder)
{
// Inside a method body or other executable code, we can pull on NonNullTypes symbol or question IsValueType without causing cycles.
// Types created outside executable context should be checked by the responsible symbol (the method symbol checks its return type, for instance).
// We still need to delay that check when binding in an attribute argument
if (!ShouldCheckConstraintsNullability)
{
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, typeArgument), nullableSyntax.QuestionToken.GetLocation());
}
else if (!typeArgument.IsValueType)
{
Symbol.ReportNullableReferenceTypesIfNeeded(Compilation, NonNullTypesContext, diagnostics, nullableSyntax.QuestionToken.GetLocation());
}
}
reportNullableReferenceTypesIfNeeded(nullableSyntax.QuestionToken, typeArgument);

if (!ShouldCheckConstraints)
{
Expand All @@ -375,6 +362,10 @@ internal NamespaceOrTypeOrAliasSymbolWithAnnotations BindNamespaceOrTypeOrAliasS
}
type.CheckConstraints(this.Compilation, conversions, location, diagnostics);
}
else if (constructedType.TypeSymbol.IsUnconstrainedTypeParameter())
{
diagnostics.Add(ErrorCode.ERR_NullableUnconstrainedTypeParameter, syntax.Location);
}

return constructedType;
}
Expand Down Expand Up @@ -451,6 +442,7 @@ internal NamespaceOrTypeOrAliasSymbolWithAnnotations BindNamespaceOrTypeOrAliasS
if (a.QuestionToken.IsKind(SyntaxKind.QuestionToken))
{
type = TypeSymbolWithAnnotations.Create(array, isNullableIfReferenceType: true);
reportNullableReferenceTypesIfNeeded(a.QuestionToken);
}
else
{
Expand Down Expand Up @@ -508,6 +500,25 @@ internal NamespaceOrTypeOrAliasSymbolWithAnnotations BindNamespaceOrTypeOrAliasS
default:
throw ExceptionUtilities.UnexpectedValue(syntax.Kind());
}

void reportNullableReferenceTypesIfNeeded(SyntaxToken questionToken, TypeSymbolWithAnnotations typeArgument = default)
{
// Inside a method body or other executable code, we can pull on NonNullTypes symbol or question IsValueType without causing cycles.
// We still need to delay that check when binding in an attribute argument
if (!InExecutableBinder || !ShouldCheckConstraintsNullability)
{
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, typeArgument), questionToken.GetLocation());
}
else
{
DiagnosticInfo info = LazyMissingNonNullTypesContextDiagnosticInfo.ReportNullableReferenceTypesIfNeeded(Compilation, NonNullTypesContext, typeArgument);

if (!(info is null))
{
diagnostics.Add(info, questionToken.GetLocation());
}
}
}
}

private TypeSymbol BindTupleType(TupleTypeSyntax syntax, DiagnosticBag diagnostics)
Expand Down
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,15 +480,13 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)

if (!returnType.IsNull)
{
returnType.ReportAnnotatedUnconstrainedTypeParameterIfAny(lambdaSymbol.DiagnosticLocation, diagnostics);
if (returnType.ContainsNullableReferenceTypes())
{
binder.Compilation.EnsureNullableAttributeExists(diagnostics, lambdaSymbol.DiagnosticLocation, modifyCompilation: false);
// Note: we don't need to warn on annotations used without NonNullTypes context for lambdas, as this is handled in binding already
}
}

ParameterHelpers.ReportAnnotatedUnconstrainedTypeParameters(lambdaParameters, diagnostics);
ParameterHelpers.EnsureNullableAttributeExists(lambdaParameters, diagnostics, modifyCompilation: false);
// Note: we don't need to warn on annotations used without NonNullTypes context for lambdas, as this is handled in binding already

Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs
View file
Open in desktop

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

3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5546,4 +5546,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_NullabilityMismatchInTypeParameterReferenceTypeConstraint_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 'class' constraint.</value>
</data>
<data name="IDS_FeatureObjectGenericTypeConstraint" xml:space="preserve">
<value>object generic type constraint</value>
</data>
</root>
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Emitter/Model/TypeParameterSymbolAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,8 @@ Cci.ITypeReference Cci.IGenericTypeParameterReference.DefiningType
switch (type.SpecialType)
{
case SpecialType.System_Object:
// Avoid emitting unnecessary object constraint.
continue;
Debug.Assert(!type.IsAnnotated);
break;
case SpecialType.System_ValueType:
seenValueType = true;
break;
Expand Down
31 changes: 30 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/LazyMissingNonNullTypesContextDiagnosticInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,36 @@ internal LazyMissingNonNullTypesContextDiagnosticInfo(CSharpCompilation compilat

protected override DiagnosticInfo ResolveInfo()
{
return _type.IsValueType ? null : Symbol.ReportNullableReferenceTypesIfNeeded(_compilation, _context);
return ReportNullableReferenceTypesIfNeeded(_compilation, _context, _type);
}

/// <summary>
/// A `?` annotation on a type that isn't a value type causes:
/// - an error before C# 8.0
/// - a warning outside of a NonNullTypes context
/// </summary>
public static DiagnosticInfo ReportNullableReferenceTypesIfNeeded(CSharpCompilation compilation, INonNullTypesContext context, TypeSymbolWithAnnotations type)
{
return !type.IsNull && (type.IsValueType || type.IsErrorType()) ? null : ReportNullableReferenceTypesIfNeeded(compilation, context);
}

private static DiagnosticInfo ReportNullableReferenceTypesIfNeeded(CSharpCompilation compilation, INonNullTypesContext nonNullTypesContext)
Copy link
Contributor Author

@AlekseyTs AlekseyTs Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

ReportNullableReferenceTypesIfNeeded [](start = 38, length = 36)

This method is moved as is from Symbol #ByDesign

{
var featureID = MessageID.IDS_FeatureStaticNullChecking;
if (!compilation.IsFeatureEnabled(featureID))
{
LanguageVersion availableVersion = compilation.LanguageVersion;
LanguageVersion requiredVersion = featureID.RequiredVersion();

return new CSDiagnosticInfo(availableVersion.GetErrorCode(), featureID.Localize(), new CSharpRequiredLanguageVersion(requiredVersion));
}
else if (nonNullTypesContext.NonNullTypes != true)
{
return new CSDiagnosticInfo(ErrorCode.WRN_MissingNonNullTypesContextForAnnotation);
}

return null;
}
}
}

4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/LazyUseSiteDiagnosticsInfoForNullableType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ protected override DiagnosticInfo ResolveInfo()
{
return _possiblyNullableTypeSymbol.TypeSymbol.OriginalDefinition.GetUseSiteDiagnostic();
}
else if (_possiblyNullableTypeSymbol.TypeSymbol.IsUnconstrainedTypeParameter())
{
return new CSDiagnosticInfo(ErrorCode.ERR_NullableUnconstrainedTypeParameter);
}

return null;
}
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ internal enum MessageID
IDS_FeatureIndexingMovableFixedBuffers = MessageBase + 12744,

IDS_InjectedDeclaration = MessageBase + 12745,
IDS_FeatureObjectGenericTypeConstraint = MessageBase + 12746,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -204,6 +205,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
{
// C# 8 features.
case MessageID.IDS_FeatureStaticNullChecking: // syntax and semantic check
case MessageID.IDS_FeatureObjectGenericTypeConstraint: // semantic check
Copy link
Member

@jcouv jcouv Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

IDS_FeatureObjectGenericTypeConstraint [](start = 31, length = 38)

Why use a separate feature? I expected this would be part of the nullable reference types feature. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why use a separate feature? I expected this would be part of the nullable reference types feature.

I think it is better to keep this as a separate feature, like other new constraints that syntactically aren't tied to nullable feature (don't use ?). For example, I believe an experience is better when one is able to use this constraint without any diagnostics regardless of NonNullTypes context. And having different error for the case when feature is not enabled feels better too.

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

return LanguageVersion.CSharp8;

// C# 7.3 features.
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6370,7 +6370,7 @@ private ArrayRankSpecifierSyntax ParseArrayRankSpecifier(bool isArrayCreation, b
SyntaxToken questionToken = null;
if (allowQuestionToken && this.CurrentToken.Kind == SyntaxKind.QuestionToken)
{
questionToken = CheckFeatureAvailability(this.EatToken(), MessageID.IDS_FeatureStaticNullChecking);
questionToken = this.EatToken();
}

return _syntaxFactory.ArrayRankSpecifier(open, list, close, questionToken);
Expand Down
29 changes: 1 addition & 28 deletions src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,25 +396,6 @@ private static TypeParameterConstraintClause MakeTypeParameterConstraintsLate(
{
Symbol containingSymbol = typeParameter.ContainingSymbol;

bool onLocalFunction = containingSymbol.Kind == SymbolKind.Method && ((MethodSymbol)containingSymbol).MethodKind == MethodKind.LocalFunction;

if ((constraintClause.Constraints & TypeParameterConstraintKind.NullableReferenceType) == TypeParameterConstraintKind.NullableReferenceType)
{
Location location = constraintClause.ClauseSyntax.Location;

foreach (TypeParameterConstraintSyntax constraintSyntax in constraintClause.ClauseSyntax.Constraints)
{
if (constraintSyntax.IsKind(SyntaxKind.ClassConstraint) &&
((ClassOrStructConstraintSyntax)constraintSyntax).QuestionToken.IsKind(SyntaxKind.QuestionToken))
{
location = ((ClassOrStructConstraintSyntax)constraintSyntax).QuestionToken.GetLocation();
break;
}
}

typeParameter.ReportNullableReferenceTypesIfNeeded(diagnostics, location);
}

var constraintTypeBuilder = ArrayBuilder<TypeSymbolWithAnnotations>.GetInstance();
var constraintTypes = constraintClause.ConstraintTypes;
int n = constraintTypes.Length;
Expand All @@ -431,14 +412,6 @@ private static TypeParameterConstraintClause MakeTypeParameterConstraintsLate(
containingSymbol.CheckConstraintTypeVisibility(syntax.Location, constraintType, diagnostics);
constraintTypeBuilder.Add(constraintType);
}

constraintType.ReportAnnotatedUnconstrainedTypeParameterIfAny(syntax.Location, diagnostics);

if (!onLocalFunction && constraintType.ContainsNullableReferenceTypes())
{
// Note: Misuse of ? annotation on declarations of local functions is reported when binding their types (since in executable context)
typeParameter.ReportNullableReferenceTypesIfNeeded(diagnostics, syntax.Location);
}
}

if (constraintTypeBuilder.Count < n)
Expand Down Expand Up @@ -966,7 +939,7 @@ private static bool CheckConstraints(
{
if (!SatisfiesConstraintType(conversions, typeArgument, constraintType, ref useSiteDiagnostics) ||
(typeArgument.IsNullable == true && !typeArgument.IsValueType &&
TypeParameterSymbol.IsNotNullableIfReferenceTypeFromConstraintType(constraintType, ConsList<TypeParameterSymbol>.Empty) == true))
TypeParameterSymbol.IsNotNullableIfReferenceTypeFromConstraintType(constraintType) == true))
{
var diagnostic = new CSDiagnosticInfo(ErrorCode.WRN_NullabilityMismatchInTypeParameterConstraint, containingSymbol.ConstructedFrom(), constraintType, typeParameter, typeArgument);
warningsBuilderOpt.Add(new TypeParameterDiagnosticInfo(typeParameter, diagnostic));
Expand Down
11 changes: 6 additions & 5 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,16 @@ private ImmutableArray<TypeSymbolWithAnnotations> GetDeclaredConstraintTypes()
}
}

// Drop 'System.Object' constraint type.
if (typeSymbol.SpecialType == SpecialType.System_Object)
// PROTOTYPE(NullableReferenceTypes): Test different [NonNullTypes] on method and containing type.
var type = TypeSymbolWithAnnotations.Create(this, typeSymbol);
type = NullableTypeDecoder.TransformType(type, constraintHandle, moduleSymbol);

// Drop 'System.Object?' constraint type.
if (type.SpecialType == SpecialType.System_Object && type.IsAnnotated)
{
continue;
}

// PROTOTYPE(NullableReferenceTypes): Test different [NonNullTypes] on method and containing type.
var type = TypeSymbolWithAnnotations.Create(this, typeSymbol);
type = NullableTypeDecoder.TransformType(type, constraintHandle, moduleSymbol);
type = TupleTypeDecoder.DecodeTupleTypesIfApplicable(type, constraintHandle, moduleSymbol);

symbolsBuilder.Add(type);
Expand Down
Loading