-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use lightweight type constraint binding for IsValueType #48270
Changes from 1 commit
4c83255
3df16ba
5c026fb
0f2136a
7c55588
a5ae914
7042f3b
36cd621
2dbbbe2
f24bc69
6d5e7a5
e65f6b1
928ebeb
9005dbc
74a0852
e5666fc
ca300e0
896ef00
adf1db1
618defa
2273303
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 |
---|---|---|
|
@@ -25,7 +25,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai | |
ImmutableArray<TypeParameterSymbol> typeParameters, | ||
TypeParameterListSyntax typeParameterList, | ||
SyntaxList<TypeParameterConstraintClauseSyntax> clauses, | ||
bool useLightweightTypeConstraintBinding, | ||
bool canUseLightweightTypeConstraintBinding, | ||
ref IReadOnlyDictionary<TypeParameterSymbol, bool> isValueTypeOverride, | ||
DiagnosticBag diagnostics, | ||
bool isForOverride = false) | ||
|
@@ -67,7 +67,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai | |
Debug.Assert(ordinal < n); | ||
|
||
(TypeParameterConstraintClause constraintClause, ArrayBuilder<TypeConstraintSyntax>? typeConstraintNodes) = | ||
this.BindTypeParameterConstraints(typeParameterList.Parameters[ordinal], clause, isForOverride, useLightweightTypeConstraintBinding: useLightweightTypeConstraintBinding, diagnostics); | ||
this.BindTypeParameterConstraints(typeParameterList.Parameters[ordinal], clause, isForOverride, canUseLightweightTypeConstraintBinding, diagnostics); | ||
if (results[ordinal] == null) | ||
{ | ||
results[ordinal] = constraintClause; | ||
|
@@ -119,7 +119,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai | |
/// Bind and return a single type parameter constraint clause along with syntax nodes corresponding to type constraints. | ||
/// </summary> | ||
private (TypeParameterConstraintClause, ArrayBuilder<TypeConstraintSyntax>?) BindTypeParameterConstraints( | ||
TypeParameterSyntax typeParameterSyntax, TypeParameterConstraintClauseSyntax constraintClauseSyntax, bool isForOverride, bool useLightweightTypeConstraintBinding, DiagnosticBag diagnostics) | ||
TypeParameterSyntax typeParameterSyntax, TypeParameterConstraintClauseSyntax constraintClauseSyntax, bool isForOverride, bool canUseLightweightTypeConstraintBinding, DiagnosticBag diagnostics) | ||
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.
Argument is not used. #Closed |
||
{ | ||
var constraints = TypeParameterConstraintKind.None; | ||
ArrayBuilder<TypeWithAnnotations>? constraintTypes = null; | ||
|
@@ -309,7 +309,7 @@ internal ImmutableArray<TypeParameterConstraintClause> BindTypeParameterConstrai | |
Debug.Assert(!isForOverride || | ||
(constraints & (TypeParameterConstraintKind.ReferenceType | TypeParameterConstraintKind.ValueType)) != (TypeParameterConstraintKind.ReferenceType | TypeParameterConstraintKind.ValueType)); | ||
|
||
return (TypeParameterConstraintClause.Create(constraints, constraintTypes?.ToImmutableAndFree() ?? ImmutableArray<TypeWithAnnotations>.Empty, useLightweightTypeConstraintBinding), syntaxBuilder); | ||
return (TypeParameterConstraintClause.Create(constraints, constraintTypes?.ToImmutableAndFree() ?? ImmutableArray<TypeWithAnnotations>.Empty, canUseLightweightTypeConstraintBinding), syntaxBuilder); | ||
|
||
static void reportOverrideWithConstraints(ref bool reportedOverrideWithConstraints, TypeParameterConstraintSyntax syntax, DiagnosticBag diagnostics) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -255,13 +255,13 @@ private ImmutableArray<TypeParameterSymbol> MakeTypeParameters(DiagnosticBag dia | |
/// <summary> | ||
/// Returns the constraint clause for the given type parameter. | ||
/// </summary> | ||
internal TypeParameterConstraintClause GetTypeParameterConstraintClause(bool useLightweightTypeConstraintBinding, int ordinal) | ||
internal TypeParameterConstraintClause GetTypeParameterConstraintClause(bool canUseLightweightTypeConstraintBinding, int ordinal) | ||
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 nullable enabling this code. #Resolved |
||
{ | ||
var clauses = _lazyTypeParameterConstraints; | ||
if (!clauses.HasValue(useLightweightTypeConstraintBinding)) | ||
if (!clauses.HasValue(canUseLightweightTypeConstraintBinding)) | ||
{ | ||
var diagnostics = DiagnosticBag.GetInstance(); | ||
var typeParameterConstraints = new TypeParameterConstraintClauses(MakeTypeParameterConstraints(useLightweightTypeConstraintBinding, diagnostics), useLightweightTypeConstraintBinding); | ||
var typeParameterConstraints = new TypeParameterConstraintClauses(MakeTypeParameterConstraints(canUseLightweightTypeConstraintBinding, diagnostics), canUseLightweightTypeConstraintBinding); | ||
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 add a |
||
|
||
if (TypeParameterConstraintClausesExtensions.InterlockedUpdate(ref _lazyTypeParameterConstraints, typeParameterConstraints) && | ||
_lazyTypeParameterConstraints.HasValue(usedLightweightTypeConstraintBinding: false)) | ||
|
@@ -275,7 +275,7 @@ internal TypeParameterConstraintClause GetTypeParameterConstraintClause(bool use | |
return (clauses.TypeParameterConstraints.Length > 0) ? clauses.TypeParameterConstraints[ordinal] : TypeParameterConstraintClause.Empty; | ||
} | ||
|
||
private ImmutableArray<TypeParameterConstraintClause> MakeTypeParameterConstraints(bool useLightweightTypeConstraintBinding, DiagnosticBag diagnostics) | ||
private ImmutableArray<TypeParameterConstraintClause> MakeTypeParameterConstraints(bool canUseLightweightTypeConstraintBinding, DiagnosticBag diagnostics) | ||
{ | ||
var typeParameters = this.TypeParameters; | ||
var results = ImmutableArray<TypeParameterConstraintClause>.Empty; | ||
|
@@ -325,9 +325,9 @@ private ImmutableArray<TypeParameterConstraintClause> MakeTypeParameterConstrain | |
// to avoid checking constraints when binding type names. | ||
Debug.Assert(!binder.Flags.Includes(BinderFlags.GenericConstraintsClause)); | ||
binder = binder.WithContainingMemberOrLambda(this).WithAdditionalFlags( | ||
BinderFlags.GenericConstraintsClause | BinderFlags.SuppressConstraintChecks | (useLightweightTypeConstraintBinding ? BinderFlags.LightweightTypeConstraintBinding : 0)); | ||
BinderFlags.GenericConstraintsClause | BinderFlags.SuppressConstraintChecks | (canUseLightweightTypeConstraintBinding ? BinderFlags.LightweightTypeConstraintBinding : 0)); | ||
|
||
constraints = binder.BindTypeParameterConstraintClauses(this, typeParameters, typeParameterList, constraintClauses, useLightweightTypeConstraintBinding, ref isValueTypeOverride, diagnostics); | ||
constraints = binder.BindTypeParameterConstraintClauses(this, typeParameters, typeParameterList, constraintClauses, canUseLightweightTypeConstraintBinding, ref isValueTypeOverride, diagnostics); | ||
} | ||
|
||
Debug.Assert(constraints.Length == arity); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,7 @@ protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymb | |
IReadOnlyDictionary<TypeParameterSymbol, bool> isValueTypeOverride = null; | ||
declaredConstraints = signatureBinder.WithAdditionalFlags(BinderFlags.GenericConstraintsClause | BinderFlags.SuppressConstraintChecks). | ||
BindTypeParameterConstraintClauses(this, TypeParameters, syntax.TypeParameterList, syntax.ConstraintClauses, | ||
useLightweightTypeConstraintBinding: false, | ||
canUseLightweightTypeConstraintBinding: false, | ||
ref isValueTypeOverride, | ||
diagnostics, isForOverride: true); | ||
} | ||
|
@@ -283,10 +283,10 @@ protected override void CompleteAsyncMethodChecksBetweenStartAndFinish() | |
} | ||
} | ||
|
||
public override ImmutableArray<TypeParameterConstraintClause> GetTypeParameterConstraintClauses(bool useLightweightTypeConstraintBinding) | ||
public override ImmutableArray<TypeParameterConstraintClause> GetTypeParameterConstraintClauses(bool canUseLightweightTypeConstraintBinding) | ||
{ | ||
var clauses = _lazyTypeParameterConstraints; | ||
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 consider inlining this local to avoid risk of using the (stale) local later in the method. #Closed |
||
if (!clauses.HasValue(useLightweightTypeConstraintBinding)) | ||
if (!clauses.HasValue(canUseLightweightTypeConstraintBinding)) | ||
{ | ||
var diagnostics = DiagnosticBag.GetInstance(); | ||
var syntax = GetSyntax(); | ||
|
@@ -301,9 +301,9 @@ public override ImmutableArray<TypeParameterConstraintClause> GetTypeParameterCo | |
TypeParameters, | ||
syntax.TypeParameterList, | ||
syntax.ConstraintClauses, | ||
useLightweightTypeConstraintBinding, | ||
canUseLightweightTypeConstraintBinding, | ||
diagnostics), | ||
useLightweightTypeConstraintBinding); | ||
canUseLightweightTypeConstraintBinding); | ||
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.
If It feels like 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 don't think so. There is still a difference between empty from lightweight vs. full binding. Diagnostics should be produced from the latter, when constraints are removed to produce an empty set. In reply to: 501252724 [](ancestors = 501252724) 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.
There shouldn't be a difference if In reply to: 501281392 [](ancestors = 501281392,501252724) 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 see. That works. Thanks #Resolved |
||
|
||
if (TypeParameterConstraintClausesExtensions.InterlockedUpdate(ref _lazyTypeParameterConstraints, constraints) && | ||
_lazyTypeParameterConstraints.HasValue(usedLightweightTypeConstraintBinding: false)) | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,10 +96,10 @@ public override string Name | |
} | ||
} | ||
|
||
internal override ImmutableArray<TypeWithAnnotations> GetConstraintTypes(ConsList<TypeParameterSymbol> inProgress, bool useLightweightTypeConstraintBinding) | ||
internal override ImmutableArray<TypeWithAnnotations> GetConstraintTypes(ConsList<TypeParameterSymbol> inProgress, bool canUseLightweightTypeConstraintBinding) | ||
{ | ||
var constraintTypes = ArrayBuilder<TypeWithAnnotations>.GetInstance(); | ||
_map.SubstituteConstraintTypesDistinctWithoutModifiers(_underlyingTypeParameter, _underlyingTypeParameter.GetConstraintTypes(inProgress, useLightweightTypeConstraintBinding), constraintTypes, null); | ||
_map.SubstituteConstraintTypesDistinctWithoutModifiers(_underlyingTypeParameter, _underlyingTypeParameter.GetConstraintTypes(inProgress, canUseLightweightTypeConstraintBinding), constraintTypes, null); | ||
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 feel like 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. That scenarios that request full binding may not get lightweight results, but other scenarios can use lightweight binding. So we have two scenarios: (1) require full binding, (2) would accept ("can use") lightweight binding. In reply to: 502585995 [](ancestors = 502585995) |
||
|
||
TypeWithAnnotations bestObjectConstraint = default; | ||
|
||
|
@@ -116,7 +116,7 @@ internal override ImmutableArray<TypeWithAnnotations> GetConstraintTypes(ConsLis | |
if (bestObjectConstraint.HasType) | ||
{ | ||
// See if we need to put Object! or Object~ back in order to preserve nullability information for the type parameter. | ||
if (!useLightweightTypeConstraintBinding && ConstraintsHelper.IsObjectConstraintSignificant(CalculateIsNotNullableFromNonTypeConstraints(), bestObjectConstraint)) | ||
if (!canUseLightweightTypeConstraintBinding && ConstraintsHelper.IsObjectConstraintSignificant(CalculateIsNotNullableFromNonTypeConstraints(), bestObjectConstraint)) | ||
{ | ||
Debug.Assert(!HasNotNullConstraint && !HasValueTypeConstraint); | ||
if (constraintTypes.Count == 0) | ||
|
@@ -159,7 +159,7 @@ internal override bool? IsNotNullable | |
else if (!HasNotNullConstraint && !HasValueTypeConstraint && !HasReferenceTypeConstraint) | ||
{ | ||
var constraintTypes = ArrayBuilder<TypeWithAnnotations>.GetInstance(); | ||
_map.SubstituteConstraintTypesDistinctWithoutModifiers(_underlyingTypeParameter, _underlyingTypeParameter.GetConstraintTypes(ConsList<TypeParameterSymbol>.Empty, useLightweightTypeConstraintBinding: false), constraintTypes, null); | ||
_map.SubstituteConstraintTypesDistinctWithoutModifiers(_underlyingTypeParameter, _underlyingTypeParameter.GetConstraintTypes(ConsList<TypeParameterSymbol>.Empty, canUseLightweightTypeConstraintBinding: false), constraintTypes, null); | ||
return IsNotNullableFromConstraintTypes(constraintTypes.ToImmutableAndFree()); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,7 +92,7 @@ private TypeParameterBounds(bool usedLightweightTypeConstraintBinding) | |
|
||
internal static class TypeParameterBoundsExtensions | ||
{ | ||
internal static bool HasValue(this TypeParameterBounds? boundsOpt, bool useLightweightTypeConstraintBinding) | ||
internal static bool HasValue(this TypeParameterBounds? boundsOpt, bool canUseLightweightTypeConstraintBinding) | ||
{ | ||
if (boundsOpt == TypeParameterBounds.Unset) | ||
{ | ||
|
@@ -102,18 +102,18 @@ internal static bool HasValue(this TypeParameterBounds? boundsOpt, bool useLight | |
{ | ||
return true; | ||
} | ||
return useLightweightTypeConstraintBinding || !boundsOpt.UsedLightweightTypeConstraintBinding; | ||
return canUseLightweightTypeConstraintBinding || !boundsOpt.UsedLightweightTypeConstraintBinding; | ||
} | ||
|
||
/// <summary> | ||
/// Returns true if bounds was updated with value. | ||
/// Returns false if bounds already had a value with sufficient 'useLightweightTypeConstraintBinding' | ||
/// or was updated to a value with sufficient 'useLightweightTypeConstraintBinding' on another thread. | ||
/// Returns false if bounds already had a value with sufficient 'canUseLightweightTypeConstraintBinding' | ||
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.
What does 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. If someone asks for fully-bound info, we cannot return light-bound info, it is not sufficient. In reply to: 502588978 [](ancestors = 502588978) |
||
/// or was updated to a value with sufficient 'canUseLightweightTypeConstraintBinding' on another thread. | ||
/// </summary> | ||
internal static bool InterlockedUpdate(ref TypeParameterBounds? bounds, TypeParameterBounds? value) | ||
{ | ||
Debug.Assert(value != TypeParameterBounds.Unset); | ||
bool valueUsedLightweightTypeConstraintBinding = !value.HasValue(useLightweightTypeConstraintBinding: false); | ||
bool valueUsedLightweightTypeConstraintBinding = !value.HasValue(canUseLightweightTypeConstraintBinding: false); | ||
|
||
while (true) | ||
{ | ||
|
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.
Consider keeping the named argument since there are two
bool
parameters in a row. #Closed