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

Support nullable annotations on unconstrained type parameters #45993

Merged
merged 28 commits into from
Jul 23, 2020
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
1 change: 1 addition & 0 deletions docs/contributing/Compiler Test Plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ This document provides guidance for thinking about language interactions and tes
- Ref structs, Readonly structs
- Readonly members on structs (methods, property/indexer accessors, custom event accessors)
- SkipLocalsInit
- Method override or explicit implementation with `where T : { class, struct, default }`

# Code
- Operators (see Eric's list below)
Expand Down
38 changes: 32 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/Binder_Constraints.cs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai
{
if (!reportedOverrideWithConstraints)
{
diagnostics.Add(ErrorCode.ERR_TypeConstraintsMustBeUniqueAndFirst, syntax.GetFirstToken().GetLocation());
reportTypeConstraintsMustBeUniqueAndFirst(syntax, diagnostics);
}

if (isForOverride && (constraints & (TypeParameterConstraintKind.ValueType | TypeParameterConstraintKind.ReferenceType)) != 0)
Expand Down Expand Up @@ -180,7 +180,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai
{
if (!reportedOverrideWithConstraints)
{
diagnostics.Add(ErrorCode.ERR_TypeConstraintsMustBeUniqueAndFirst, syntax.GetFirstToken().GetLocation());
reportTypeConstraintsMustBeUniqueAndFirst(syntax, diagnostics);
}

if (isForOverride && (constraints & (TypeParameterConstraintKind.ValueType | TypeParameterConstraintKind.ReferenceType)) != 0)
Expand Down Expand Up @@ -214,6 +214,27 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai

constraints |= TypeParameterConstraintKind.Constructor;
continue;
case SyntaxKind.DefaultConstraint:
if (!isForOverride)
{
diagnostics.Add(ErrorCode.ERR_DefaultConstraintOverrideOnly, syntax.GetLocation());
}

if (i != 0)
{
if (!reportedOverrideWithConstraints)
{
reportTypeConstraintsMustBeUniqueAndFirst(syntax, diagnostics);
}

if (isForOverride && (constraints & (TypeParameterConstraintKind.ValueType | TypeParameterConstraintKind.ReferenceType)) != 0)
{
continue;
}
}

constraints |= TypeParameterConstraintKind.Default;
continue;
case SyntaxKind.TypeConstraint:
if (isForOverride)
{
Expand All @@ -239,7 +260,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai
case ConstraintContextualKeyword.Unmanaged:
if (i != 0)
{
diagnostics.Add(ErrorCode.ERR_TypeConstraintsMustBeUniqueAndFirst, typeSyntax.GetLocation());
reportTypeConstraintsMustBeUniqueAndFirst(typeSyntax, diagnostics);
continue;
}

Expand All @@ -253,7 +274,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai
case ConstraintContextualKeyword.NotNull:
if (i != 0)
{
diagnostics.Add(ErrorCode.ERR_TypeConstraintsMustBeUniqueAndFirst, typeSyntax.GetLocation());
reportTypeConstraintsMustBeUniqueAndFirst(typeSyntax, diagnostics);
}

constraints |= TypeParameterConstraintKind.NotNull;
Expand Down Expand Up @@ -292,6 +313,11 @@ static void reportOverrideWithConstraints(ref bool reportedOverrideWithConstrain
reportedOverrideWithConstraints = true;
}
}

static void reportTypeConstraintsMustBeUniqueAndFirst(CSharpSyntaxNode syntax, DiagnosticBag diagnostics)
{
diagnostics.Add(ErrorCode.ERR_TypeConstraintsMustBeUniqueAndFirst, syntax.GetLocation());
}
}

internal ImmutableArray<TypeParameterConstraintClause> GetDefaultTypeParameterConstraintClauses(TypeParameterListSyntax typeParameterList)
Expand Down Expand Up @@ -351,7 +377,7 @@ private static TypeParameterConstraintClause RemoveInvalidConstraints(
// 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))
if (IsValidConstraint(typeParameter.Name, syntax, constraintType, constraintClause.Constraints, constraintTypeBuilder, diagnostics))
{
CheckConstraintTypeVisibility(containingSymbol, syntax.Location, constraintType, diagnostics);
constraintTypeBuilder.Add(constraintType);
Expand Down Expand Up @@ -388,7 +414,7 @@ private static void CheckConstraintTypeVisibility(
/// Returns true if the constraint is valid. Otherwise
/// returns false and generates a diagnostic.
/// </summary>
internal static bool IsValidConstraint(
private static bool IsValidConstraint(
string typeParameterName,
TypeConstraintSyntax syntax,
TypeWithAnnotations type,
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1020,7 +1020,7 @@ private BoundPattern BindVarDesignation(
}
case SyntaxKind.SingleVariableDesignation:
{
var declType = TypeWithState.ForType(inputType).ToTypeWithAnnotations();
var declType = TypeWithState.ForType(inputType).ToTypeWithAnnotations(Compilation);
BindPatternDesignation(
designation: node, declType: declType, inputValEscape: inputValEscape, permitDesignations: permitDesignations,
typeSyntax: null, diagnostics: diagnostics, hasErrors: ref hasErrors,
Expand Down
21 changes: 18 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ NamespaceOrTypeOrAliasSymbolWithAnnotations bindNullable()

if (!ShouldCheckConstraints)
{
diagnostics.Add(new LazyUseSiteDiagnosticsInfoForNullableType(constructedType), syntax.GetLocation());
diagnostics.Add(new LazyUseSiteDiagnosticsInfoForNullableType(Compilation.LanguageVersion, constructedType), syntax.GetLocation());
}
else if (constructedType.IsNullableType())
{
Expand All @@ -521,9 +521,9 @@ NamespaceOrTypeOrAliasSymbolWithAnnotations bindNullable()
var location = syntax.Location;
type.CheckConstraints(new ConstraintsHelper.CheckConstraintsArgs(this.Compilation, this.Conversions, includeNullability: true, location, diagnostics));
}
else if (constructedType.Type.IsTypeParameterDisallowingAnnotation())
else if (GetNullableUnconstrainedTypeParameterDiagnosticIfNecessary(Compilation.LanguageVersion, constructedType) is { } diagnosticInfo)
{
diagnostics.Add(ErrorCode.ERR_NullableUnconstrainedTypeParameter, syntax.Location);
diagnostics.Add(diagnosticInfo, syntax.Location);
}

return constructedType;
Expand Down Expand Up @@ -571,6 +571,21 @@ NamespaceOrTypeOrAliasSymbolWithAnnotations createErrorType()
return TypeWithAnnotations.Create(CreateErrorType());
}
}

internal static CSDiagnosticInfo? GetNullableUnconstrainedTypeParameterDiagnosticIfNecessary(LanguageVersion languageVersion, in TypeWithAnnotations type)
{
if (type.Type.IsTypeParameterDisallowingAnnotationInCSharp8())
{
// Check IDS_FeatureDefaultTypeParameterConstraint feature since `T?` and `where ... : default`
// are treated as a single feature, even though the errors reported for the two cases are distinct.
var requiredVersion = MessageID.IDS_FeatureDefaultTypeParameterConstraint.RequiredVersion();
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 17, 2020

Choose a reason for hiding this comment

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

IDS_FeatureDefaultTypeParameterConstraint [](start = 48, length = 41)

This is somewhat confusing because default constraint isn't involved. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment.


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

if (requiredVersion > languageVersion)
{
return new CSDiagnosticInfo(ErrorCode.ERR_NullableUnconstrainedTypeParameter, new CSharpRequiredLanguageVersion(requiredVersion));
}
}
return null;
}
#nullable restore

private TypeWithAnnotations BindArrayType(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1493,12 +1493,14 @@ internal bool HasTopLevelNullabilityIdentityConversion(TypeWithAnnotations sourc
return true;
}

if (source.IsPossiblyNullableTypeTypeParameter() && !destination.IsPossiblyNullableTypeTypeParameter())
var sourceIsPossiblyNullableTypeParameter = IsPossiblyNullableTypeTypeParameter(source);
var destinationIsPossiblyNullableTypeParameter = IsPossiblyNullableTypeTypeParameter(destination);
if (sourceIsPossiblyNullableTypeParameter && !destinationIsPossiblyNullableTypeParameter)
{
return destination.NullableAnnotation.IsAnnotated();
}

if (destination.IsPossiblyNullableTypeTypeParameter() && !source.IsPossiblyNullableTypeTypeParameter())
if (destinationIsPossiblyNullableTypeParameter && !sourceIsPossiblyNullableTypeParameter)
{
return source.NullableAnnotation.IsAnnotated();
}
Expand All @@ -1523,14 +1525,21 @@ internal bool HasTopLevelNullabilityImplicitConversion(TypeWithAnnotations sourc
return true;
}

if (source.IsPossiblyNullableTypeTypeParameter() && !destination.IsPossiblyNullableTypeTypeParameter())
if (IsPossiblyNullableTypeTypeParameter(source) && !IsPossiblyNullableTypeTypeParameter(destination))
{
return false;
}

return !source.NullableAnnotation.IsAnnotated();
}

private static bool IsPossiblyNullableTypeTypeParameter(in TypeWithAnnotations typeWithAnnotations)
{
var type = typeWithAnnotations.Type;
return type is object &&
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 17, 2020

Choose a reason for hiding this comment

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

return type is object && [](start = 12, length = 24)

It looks like we dropped IsNotAnnotated check. Is this intentional? Could you please elaborate? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like we dropped IsNotAnnotated check. Is this intentional?

Yes. Callers above are explicitly checking NullableAnnotation, and requiring NullableAnnotation == NullableAnnotation.NotAnnotated here would mean T and T? would be considered identical in HasTopLevelNullabilityIdentityConversion(). For instance, that would allow assigning IEnumerable<T?> to IEnumerable<T> in UnconstrainedTypeParameter_21().


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

(type.IsPossiblyNullableReferenceTypeTypeParameter() || type.IsNullableTypeOrTypeParameter());
}

/// <summary>
/// Returns false if the source does not have an implicit conversion to the destination
/// because of either incompatible top level or nested nullability.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1541,9 +1541,9 @@ private bool ExactOrBoundsNullableInference(ExactOrBoundsKind kind, TypeWithAnno

return false;

// True if the type is nullable but not an unconstrained type parameter.
// True if the type is nullable.
bool isNullableOnly(TypeWithAnnotations type)
=> type.NullableAnnotation.IsAnnotated() && !type.Type.IsTypeParameterDisallowingAnnotation();
=> type.NullableAnnotation.IsAnnotated();
}

private bool ExactNullableInference(TypeWithAnnotations source, TypeWithAnnotations target, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
Expand Down
13 changes: 11 additions & 2 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -1363,7 +1363,7 @@
<value>The return type for ++ or -- operator must match the parameter type or be derived from the parameter type</value>
</data>
<data name="ERR_TypeConstraintsMustBeUniqueAndFirst" xml:space="preserve">
<value>The 'class', 'struct', 'unmanaged', and 'notnull' constraints cannot be combined or duplicated, and must be specified first in the constraints list.</value>
<value>The 'class', 'struct', 'unmanaged', 'notnull', and 'default' constraints cannot be combined or duplicated, and must be specified first in the constraints list.</value>
</data>
<data name="ERR_RefValBoundWithClass" xml:space="preserve">
<value>'{0}': cannot specify both a constraint class and the 'class' or 'struct' constraint</value>
Expand Down Expand Up @@ -5608,7 +5608,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>Explicit application of 'System.Runtime.CompilerServices.NullableAttribute' is not allowed.</value>
</data>
<data name="ERR_NullableUnconstrainedTypeParameter" xml:space="preserve">
<value>A nullable type parameter must be known to be a value type or non-nullable reference type. Consider adding a 'class', 'struct', or type constraint.</value>
<value>A nullable type parameter must be known to be a value type or non-nullable reference type unless language version '{0}' or greater is used. Consider changing the language version or adding a 'class', 'struct', or type constraint.</value>
</data>
<data name="ERR_NullableOptionNotAvailable" xml:space="preserve">
<value>Invalid '{0}' value: '{1}' for C# {2}. Please use language version '{3}' or greater.</value>
Expand Down Expand Up @@ -5736,6 +5736,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureNullPointerConstantPattern" xml:space="preserve">
<value>null pointer constant pattern</value>
</data>
<data name="IDS_FeatureDefaultTypeParameterConstraint" xml:space="preserve">
<value>default type parameter constraints</value>
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 17, 2020

Choose a reason for hiding this comment

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

default type parameter constraints [](start = 11, length = 34)

It feels like the name of the feature isn't very clear, especially that default is used in not very common cases. For example, a user typed T? and compiler reports that "default type parameter constraints" aren't supported by the language version. What constraints the message is talking about? This isn't a constraint. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we need another feature name for T?


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

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 message is only reported for uses of where T : default. For uses of T?, we continue to report ErrorCode.ERR_NullableUnconstrainedTypeParameter.


In reply to: 456603253 [](ancestors = 456603253,456602711)

</data>
<data name="ERR_WrongNumberOfSubpatterns" xml:space="preserve">
<value>Matching the tuple type '{0}' requires '{1}' subpatterns, but '{2}' subpatterns are present.</value>
</data>
Expand Down Expand Up @@ -6021,6 +6024,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_OverrideValConstraintNotSatisfied" xml:space="preserve">
<value>Method '{0}' specifies a 'struct' constraint for type parameter '{1}', but corresponding type parameter '{2}' of overridden or explicitly implemented method '{3}' is not a non-nullable value type.</value>
</data>
<data name="ERR_OverrideDefaultConstraintNotSatisfied" xml:space="preserve">
<value>Method '{0}' specifies a 'default' constraint for type parameter '{1}', but corresponding type parameter '{2}' of overridden or explicitly implemented method '{3}' is constrained to a reference type or a value type.</value>
</data>
<data name="ERR_DefaultConstraintOverrideOnly" xml:space="preserve">
<value>The 'default' constraint is valid on override and explicit interface implementation methods only.</value>
</data>
<data name="IDS_OverrideWithConstraints" xml:space="preserve">
<value>constraints for override and explicit interface implementation methods</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1831,6 +1831,8 @@ internal enum ErrorCode

ERR_StaticAnonymousFunctionCannotCaptureVariable = 8820,
ERR_StaticAnonymousFunctionCannotCaptureThis = 8821,
ERR_OverrideDefaultConstraintNotSatisfied = 8822,
ERR_DefaultConstraintOverrideOnly = 8823,

ERR_BadWarningVersion = 8848,
ERR_ExpressionTreeContainsWithExpression = 8849,
Expand Down
6 changes: 4 additions & 2 deletions src/Compilers/CSharp/Portable/Errors/LazyDiagnosticInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable enable

using System.Threading;

namespace Microsoft.CodeAnalysis.CSharp
{
internal abstract class LazyDiagnosticInfo : DiagnosticInfo
{
private DiagnosticInfo _lazyInfo;
private DiagnosticInfo? _lazyInfo;

protected LazyDiagnosticInfo()
: base(CSharp.MessageProvider.Instance, (int)ErrorCode.Unknown)
Expand All @@ -25,6 +27,6 @@ internal sealed override DiagnosticInfo GetResolvedInfo()
return _lazyInfo;
}

protected abstract DiagnosticInfo ResolveInfo();
protected abstract DiagnosticInfo? ResolveInfo();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,30 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable enable

using Microsoft.CodeAnalysis.CSharp.Symbols;

namespace Microsoft.CodeAnalysis.CSharp
{
internal sealed class LazyUseSiteDiagnosticsInfoForNullableType : LazyDiagnosticInfo
{
private readonly LanguageVersion _languageVersion;
private readonly TypeWithAnnotations _possiblyNullableTypeSymbol;

internal LazyUseSiteDiagnosticsInfoForNullableType(TypeWithAnnotations possiblyNullableTypeSymbol)
internal LazyUseSiteDiagnosticsInfoForNullableType(LanguageVersion languageVersion, TypeWithAnnotations possiblyNullableTypeSymbol)
{
_languageVersion = languageVersion;
_possiblyNullableTypeSymbol = possiblyNullableTypeSymbol;
}

protected override DiagnosticInfo ResolveInfo()
protected override DiagnosticInfo? ResolveInfo()
{
if (_possiblyNullableTypeSymbol.IsNullableType())
{
return _possiblyNullableTypeSymbol.Type.OriginalDefinition.GetUseSiteDiagnostic();
}
else if (_possiblyNullableTypeSymbol.Type.IsTypeParameterDisallowingAnnotation())
{
return new CSDiagnosticInfo(ErrorCode.ERR_NullableUnconstrainedTypeParameter);
}

return null;
return Binder.GetNullableUnconstrainedTypeParameterDiagnosticIfNecessary(_languageVersion, _possiblyNullableTypeSymbol);
}
}
}
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ internal enum MessageID

// available

IDS_FeatureDefaultTypeParameterConstraint = MessageBase + 12689,
IDS_FeatureNullPropagatingOperator = MessageBase + 12690,
IDS_FeatureExpressionBodiedMethod = MessageBase + 12691,
IDS_FeatureExpressionBodiedProperty = MessageBase + 12692,
Expand Down Expand Up @@ -208,6 +209,7 @@ internal enum MessageID
IDS_FeatureRecords = MessageBase + 12782,
IDS_FeatureNullPointerConstantPattern = MessageBase + 12783,
IDS_FeatureModuleInitializers = MessageBase + 12784,
IDS_FeatureTargetTypedConditional = MessageBase + 12785,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -334,6 +336,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureRecords:
case MessageID.IDS_FeatureStaticAnonymousFunction: // syntax check
case MessageID.IDS_FeatureModuleInitializers: // semantic check on method attribute
case MessageID.IDS_FeatureDefaultTypeParameterConstraint:
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 17, 2020

Choose a reason for hiding this comment

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

case MessageID.IDS_FeatureDefaultTypeParameterConstraint: [](start = 16, length = 57)

Consider adding a comment if this is not a parsing time check #Closed

return LanguageVersion.Preview;

// C# 8.0 features.
Expand Down
Loading