-
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
Support Object type as a generic type constraint. #29809
Changes from 3 commits
25fd08a
426225b
941531b
f1636e4
ebeea03
1bb5763
00f0b8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
{ | ||
|
@@ -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; | ||
} | ||
|
@@ -451,6 +442,7 @@ internal NamespaceOrTypeOrAliasSymbolWithAnnotations BindNamespaceOrTypeOrAliasS | |
if (a.QuestionToken.IsKind(SyntaxKind.QuestionToken)) | ||
{ | ||
type = TypeSymbolWithAnnotations.Create(array, isNullableIfReferenceType: true); | ||
reportNullableReferenceTypesIfNeeded(a.QuestionToken); | ||
} | ||
else | ||
{ | ||
|
@@ -508,6 +500,32 @@ internal NamespaceOrTypeOrAliasSymbolWithAnnotations BindNamespaceOrTypeOrAliasS | |
default: | ||
throw ExceptionUtilities.UnexpectedValue(syntax.Kind()); | ||
} | ||
|
||
void reportNullableReferenceTypesIfNeeded(SyntaxToken questionToken, TypeSymbolWithAnnotations typeArgument = default) | ||
{ | ||
if (InExecutableBinder) | ||
{ | ||
// 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 (!ShouldCheckConstraintsNullability) | ||
{ | ||
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, typeArgument), questionToken.GetLocation()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please combine these checks to reduce the number of code paths:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested refactoring will require duplicating condition checks In reply to: 217495231 [](ancestors = 217495231) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I will follow up on this in the next PR In reply to: 217506307 [](ancestors = 217506307,217495231) |
||
else | ||
{ | ||
DiagnosticInfo info = LazyMissingNonNullTypesContextDiagnosticInfo.ReportNullableReferenceTypesIfNeeded(Compilation, NonNullTypesContext, typeArgument); | ||
|
||
if (!(info is null)) | ||
{ | ||
diagnostics.Add(info, questionToken.GetLocation()); | ||
} | ||
} | ||
} | ||
else | ||
{ | ||
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, typeArgument), questionToken.GetLocation()); | ||
} | ||
} | ||
} | ||
|
||
private TypeSymbol BindTupleType(TupleTypeSyntax syntax, DiagnosticBag diagnostics) | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider moving the xml doc along:
|
||
{ | ||
return !type.IsNull && (type.IsValueType || type.IsErrorType()) ? null : ReportNullableReferenceTypesIfNeeded(compilation, context); | ||
} | ||
|
||
private static DiagnosticInfo ReportNullableReferenceTypesIfNeeded(CSharpCompilation compilation, INonNullTypesContext nonNullTypesContext) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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; | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 In reply to: 217125534 [](ancestors = 217125534) |
||
return LanguageVersion.CSharp8; | ||
|
||
// C# 7.3 features. | ||
|
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.
Although I understand
object
is emitted just likeSystem.Object
constraint, or any other type constraint, it may be good to call out for our F#/interop friends. #Resolved