-
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
Support Object type as a generic type constraint. #29809
Conversation
Please update the speclet as well. Thanks #Closed |
@@ -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 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
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.
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)
@@ -904,6 +904,11 @@ Done: | |||
''' </summary> | |||
Private Shared Function AreConstraintTypesSubset(constraintTypes1 As ArrayBuilder(Of TypeSymbol), constraintTypes2 As ArrayBuilder(Of TypeSymbol)) As Boolean |
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.
AreConstraintTypesSubset [](start = 32, length = 24)
What happens if a C# 7 or VB 15 compiler encounters the object
constraint in metadata? #Closed
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.
What happens if a C# 7 or VB 15 compiler encounters the object constraint in metadata?
C# 7 simply ignores such constraint in metadata (see related changes in PETypeParameterSymbol.cs)
However, VB doesn't do this, which (without the change below) prevented overriding and implementing methods with such constraints because:
- VB disallows explicit Object constraint
- Constraints must be spelled out explicitly and should match overridden/implemented method.
I think the impact on VB should be investigated more, I am planning to open an issue for that.
In reply to: 217126236 [](ancestors = 217126236)
{ | ||
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, type: default), a.QuestionToken.GetLocation()); | ||
} | ||
else |
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.
else [](start = 36, length = 4)
Can or should this also check for IsValueType and IsErrorType as above?
If so, this block can probably be factored out to a helper method. #Closed
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.
Can or should this also check for IsValueType and IsErrorType as above?
Here we are dealing with array type, which is never a value type, or an error type.
In reply to: 217129906 [](ancestors = 217129906)
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.
Right, so the code above seems like it would work (although the extra checks give a known result).
I think the cost of a few wasted instructions is worth the benefit of factored code.
In reply to: 217139199 [](ancestors = 217139199,217129906)
{ | ||
Symbol.ReportNullableReferenceTypesIfNeeded(Compilation, NonNullTypesContext, diagnostics, nullableSyntax.QuestionToken.GetLocation()); | ||
} | ||
} | ||
else | ||
{ | ||
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, typeArgument), nullableSyntax.QuestionToken.GetLocation()); |
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.
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, typeArgument), nullableSyntax.QuestionToken.GetLocation()); [](start = 28, length = 158)
I need some help to understand this.
Previously, the check for misused ?
was checked by ReportNullableReferenceTypesIfNeeded
in the various symbols. That minimized the use of lazy diagnostics.
What's the scenario that makes using a lot more lazy diagnostics necessary?
Also, the comment above should be updated, since reflects previous design. #Closed
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.
Previously, the check for misused ? was checked by ReportNullableReferenceTypesIfNeeded in the various symbols. That minimized the use of lazy diagnostics.
Diagnostics should be reported when a type is bound. Old approach had obvious holes (some types were not checked, can be observed in test) and was fragile - every one implementing a new symbol was supposed to worry about not missing the checks and figure out the right location, etc. In addition, an attempt to report this diagnostics eagerly in ConstraintHelper.cs was causing a cycle in some scenarios.
I will remove the "// Types created outside executable context should be checked by the responsible symbol (the method symbol checks its return type, for instance)." comment.
In reply to: 217135590 [](ancestors = 217135590)
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.
Regarding the question of fragility:
We used to pair calls to ReportNullableReferenceTypesIfNeeded
and ReportAnnotatedUnconstrainedTypeParameters
with calls to EnsureNullableAttributeExists
.
If we were missing some cases of the former, then we're likely missing some cases of the latter too. Should we also move the call to EnsureNullableAttributeExists
here? (file an issue?)
In reply to: 217141886 [](ancestors = 217141886,217135590)
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.
Should we also move the call to EnsureNullableAttributeExists here? (file an issue?)
No. That check is in the right place. Who synthesizes the attribute is responsible for ensuring it exists.
In reply to: 217159561 [](ancestors = 217159561,217141886,217135590)
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.
Also note, that not every annotated type results in an attribute synthesized somewhere
In reply to: 217160425 [](ancestors = 217160425,217159561,217141886,217135590)
@@ -22,7 +22,8 @@ internal LazyMissingNonNullTypesContextDiagnosticInfo(CSharpCompilation compilat | |||
|
|||
protected override DiagnosticInfo ResolveInfo() | |||
{ | |||
return _type.IsValueType ? null : Symbol.ReportNullableReferenceTypesIfNeeded(_compilation, _context); | |||
return !_type.IsNull && (_type.IsValueType || _type.IsErrorType()) ? null : Symbol.ReportNullableReferenceTypesIfNeeded(_compilation, _context); |
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.
_type.IsNull [](start = 20, length = 12)
Since the _type.IsNull
check doesn't need to be delayed, can we do it up-front, and avoid queuing a lazy diagnostic in such cases?
Related to this, I didn't understand why we create some LazyMissingNonNullTypesContextDiagnosticInfo
instances with type: default
(which guarantee that no diagnostic is produced). #Closed
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.
I didn't understand why we create some LazyMissingNonNullTypesContextDiagnosticInfo instances with type: default (which guarantee that no diagnostic is produced).
Because there is no such guarantee.
In reply to: 217156726 [](ancestors = 217156726)
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 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
} | ||
} | ||
|
||
internal static DiagnosticInfo ReportNullableReferenceTypesIfNeeded(CSharpCompilation compilation, INonNullTypesContext nonNullTypesContext) |
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.
ReportNullableReferenceTypesIfNeeded [](start = 39, length = 36)
This method is moved as is to LazyMissingNonNullTypesContextDiagnosticInfo #ByDesign
return ReportNullableReferenceTypesIfNeeded(_compilation, _context, _type); | ||
} | ||
|
||
public static DiagnosticInfo ReportNullableReferenceTypesIfNeeded(CSharpCompilation compilation, INonNullTypesContext context, TypeSymbolWithAnnotations type) |
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.
ReportNullableReferenceTypesIfNeeded [](start = 37, length = 36)
Consider moving the xml doc along:
/// <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>
``` #Closed
@@ -175,6 +175,11 @@ 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` constraint is allowed, which requires the type to be non-nullable when it is a reference type. |
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.
object
[](start = 12, length = 8)
Although I understand object
is emitted just like System.Object
constraint, or any other type constraint, it may be good to call out for our F#/interop friends. #Resolved
@@ -3889,12 +3889,6 @@ End Class | |||
</compilation> | |||
Dim compilation = CreateCompilationWithCustomILSource(vbSource, ilSource) | |||
compilation.AssertTheseDiagnostics(<errors><![CDATA[ |
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.
AssertTheseDiagnostics [](start = 24, length = 22)
It feels like this change of behavior in VB (between new compiler and old compiler) may set up compat issues for customers, even though they are compiling with VB LangVersion 15.
Something to consider as part of the follow-up issue. #Resolved
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.
Something to consider as part of the follow-up issue.
I will capture that too.
In reply to: 217167443 [](ancestors = 217167443)
If you say so. This fact is not obvious to me. In reply to: 420777588 [](ancestors = 420777588) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:532 in 426225b. [](commit_id = 426225b, deletion_comment = False) |
Sorry the comment wasn't very explicit. Thanks :-) In reply to: 420778294 [](ancestors = 420778294,420777588) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:532 in 426225b. [](commit_id = 426225b, deletion_comment = False) |
It looks like both Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:42148 in 426225b. [](commit_id = 426225b, deletion_comment = False) |
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.
LGTM Thanks (iteration 3)
I cannot tell. In reply to: 420781793 [](ancestors = 420781793) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:45237 in 426225b. [](commit_id = 426225b, deletion_comment = False) |
This error is not specific to object and is reported this way for any class. I also do not find it confusing. It clearly states a class type constraint must be the first in the list. It does not suggest that changing the order is guaranteed to eliminate all errors. In reply to: 420780614 [](ancestors = 420780614) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:42148 in 426225b. [](commit_id = 426225b, deletion_comment = False) |
@cston Please review #Closed |
1 similar comment
@cston Please review #Closed |
if (!ShouldCheckConstraintsNullability) | ||
{ | ||
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(Compilation, NonNullTypesContext, typeArgument), questionToken.GetLocation()); | ||
} |
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.
Please combine these checks to reduce the number of code paths:
if (!InExecutable || !ShouldCheckConstraintsNullability)
{
diagnostics.Add(new LazyMissingNonNullTypesContextDiagnosticInfo(...));
}
else
{
...
}
``` #Resolved
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.
Please combine these checks to reduce the number of code paths:
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Please combine these checks to reduce the number of code paths:
I will follow up on this in the next PR
In reply to: 217506307 [](ancestors = 217506307,217495231)
@@ -118,6 +118,6 @@ internal override TypeSymbol GetDeducedBaseType(ConsList<TypeParameterSymbol> in | |||
return _map.SubstituteType(_underlyingTypeParameter.GetDeducedBaseType(inProgress)).AsTypeSymbolOnly(); | |||
} | |||
|
|||
private static readonly Func<TypeSymbolWithAnnotations, bool> s_isNotObjectFunc = type => type.SpecialType != SpecialType.System_Object; | |||
private static readonly Func<TypeSymbolWithAnnotations, bool> s_isNotObjectFunc = type => type.SpecialType != SpecialType.System_Object || !type.IsAnnotated; |
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.
s_isNotObjectFunc [](start = 70, length = 17)
The name seems misleading now since it looks like the lambda represents "not object?
". Consider renaming, or just inline the lambda in the one use above. #Resolved
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.
The name seems misleading now since it looks like the lambda represents "not object?". Consider renaming, or just inline the lambda in the one use above.
Will follow up on this in the next PR
In reply to: 217497755 [](ancestors = 217497755)
@@ -444,15 +444,14 @@ internal bool IsReferenceTypeFromConstraintTypes(ImmutableArray<TypeSymbolWithAn | |||
return AnyConstraintTypes(constraintTypes, inProgress, (type, arg) => ConstraintImpliesReferenceType(type.TypeSymbol, arg)); | |||
} | |||
|
|||
internal bool? IsNotNullableIfReferenceTypeFromConstraintTypes(ImmutableArray<TypeSymbolWithAnnotations> constraintTypes, ConsList<TypeParameterSymbol> inProgress) | |||
internal bool? IsNotNullableIfReferenceTypeFromConstraintTypes(ImmutableArray<TypeSymbolWithAnnotations> constraintTypes) |
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.
internal [](start = 8, length = 8)
static
#Closed
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.
Isn't Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:44102 in f1636e4. [](commit_id = f1636e4, deletion_comment = False) |
No it is not. In reply to: 421123846 [](ancestors = 421123846) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:44102 in f1636e4. [](commit_id = f1636e4, deletion_comment = False) |
It is a In reply to: 421126506 [](ancestors = 421126506,421123846) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:44102 in f1636e4. [](commit_id = f1636e4, deletion_comment = False) |
Please remove the In reply to: 421130652 [](ancestors = 421130652,421123369) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:44070 in f1636e4. [](commit_id = f1636e4, deletion_comment = False) |
TB2 is unconstraint (not known to be non-nullable), given the current implementation of TypeSymbolWithAnnotations all references to TB2 are considered nullable. I agree, it would make sense to treat them as In reply to: 421137362 [](ancestors = 421137362,421126506,421123846) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:44102 in f1636e4. [](commit_id = f1636e4, deletion_comment = False) |
I'll open an issue to take another look at that special handling In reply to: 421140386 [](ancestors = 421140386,421137362,421126506,421123846) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:44102 in f1636e4. [](commit_id = f1636e4, deletion_comment = False) |
These are markers for interesting lines. I will leave them as is. In reply to: 421137671 [](ancestors = 421137671,421130652,421123369) Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:44070 in f1636e4. [](commit_id = f1636e4, deletion_comment = False) |
… into ObjectGenericTypeConstraint
6f6d91e
to
00f0b8a
Compare
No description provided.