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

Extended property patterns: address PROTOTYPE comments #53594

Merged
2 changes: 1 addition & 1 deletion docs/contributing/Compiler Test Plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ This document provides guidance for thinking about language interactions and tes
- Ref return, ref readonly return, ref ternary, ref readonly local, ref local re-assignment, ref foreach
- `this = e;` in `struct` .ctor
- Stackalloc (including initializers)
- Patterns (constant, declaration, `var`, positional, property, discard, parenthesized, type, relational, `and`/`or`/`not`)
- Patterns (constant, declaration, `var`, positional, property and extended property, discard, parenthesized, type, relational, `and`/`or`/`not`)
- Switch expressions
- With expressions (on record classes and on value types)
- Nullability annotations (`?`, attributes) and analysis
Expand Down
2 changes: 1 addition & 1 deletion docs/features/patterns.work.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
- [ ] Test all of the ways that a name would not be valid to name a property in a property pattern
- [ ] Need to design the data representation for edit-and-continue for temps in patterns
- [ ] Need to precisely share temps per the edit-and-continue spec
- [ ] Need bullets here or github issues for PROTOTYPE(patterns2) comments.
- [ ] Need bullets here or github issues for prototype comments.

- [ ] Need to ensure good code quality, e.g. avoid redundant null checks preceding types tests
- [ ] Need to ensure a good tradeoff between decision tree size explosion and execution of redundant tests.
3 changes: 1 addition & 2 deletions eng/targets/Settings.props
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,7 @@
and hence suppress this warning until we get closer to release and a more
thorough documentation story
-->
<!-- PROTOTYPE(extended-property-patterns) PublicAPIs -->
<NoWarn>$(NoWarn);1573;1591;1701;RS0016</NoWarn>
<NoWarn>$(NoWarn);1573;1591;1701</NoWarn>
Copy link
Member

@alrz alrz May 21, 2021

Choose a reason for hiding this comment

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

Thanks a lot for sorting this out. Have we fixed the code action or this is done manually? (I have no idea how this could be done manually) #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done it semi-manually: copy sections of generated code into real files, then trigger publicAPI fixer on those, repeat... It's better than manually editting, by far :-)

</PropertyGroup>
<PropertyGroup>
<DefineConstants Condition="'$(InitialDefineConstants)' != ''">$(DefineConstants);$(InitialDefineConstants)</DefineConstants>
Expand Down
35 changes: 32 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,12 @@ internal BoundPattern BindConstantPatternWithFallbackToTypePattern(
bool hasErrors,
BindingDiagnosticBag diagnostics)
{
if (hasSuppression(expression))
{
diagnostics.Add(ErrorCode.ERR_IllegalSuppression, expression.Location);
hasErrors = true;
}

ExpressionSyntax innerExpression = SkipParensAndNullSuppressions(expression);
if (innerExpression.Kind() == SyntaxKind.DefaultLiteralExpression)
{
Expand All @@ -222,6 +228,23 @@ internal BoundPattern BindConstantPatternWithFallbackToTypePattern(
bool isExplicitNotNullTest = boundType.Type.SpecialType == SpecialType.System_Object;
return new BoundTypePattern(node, boundType, isExplicitNotNullTest, inputType, boundType.Type, hasErrors);
}

static bool hasSuppression(ExpressionSyntax e)
{
while (true)
{
switch (e)
{
case ParenthesizedExpressionSyntax p:
e = p.Expression;
break;
case PostfixUnaryExpressionSyntax { RawKind: (int)SyntaxKind.SuppressNullableWarningExpression }:
return true;
default:
return false;
}
}
}
}

private ExpressionSyntax SkipParensAndNullSuppressions(ExpressionSyntax e)
Expand Down Expand Up @@ -777,10 +800,12 @@ deconstructMethod is null &&
for (int i = 0; i < node.Subpatterns.Count; i++)
{
var subPattern = node.Subpatterns[i];
// If the expression before the colon isn't just a name, we'll have reported a parsing error.
Debug.Assert(subPattern.NameColon is not null || subPattern.ExpressionColon is null || subPattern.ExpressionColon.HasErrors);

bool isError = hasErrors || outPlaceholders.IsDefaultOrEmpty || i >= outPlaceholders.Length;
TypeSymbol elementType = isError ? CreateErrorType() : outPlaceholders[i].Type;
ParameterSymbol? parameter = null;
// PROTOTYPE(extended-property-patterns) ExpressionColon
if (subPattern.NameColon != null && !isError)
{
// Check that the given name is the same as the corresponding parameter of the method.
Expand Down Expand Up @@ -819,7 +844,9 @@ private void BindITupleSubpatterns(
var objectType = Compilation.GetSpecialType(SpecialType.System_Object);
foreach (var subpatternSyntax in node.Subpatterns)
{
// PROTOTYPE(extended-property-patterns) ExpressionColon
// If the expression before the colon isn't just a name, we'll have reported a parsing error.
Debug.Assert(subpatternSyntax.NameColon is not null || subpatternSyntax.ExpressionColon is null || subpatternSyntax.ExpressionColon.HasErrors);

if (subpatternSyntax.NameColon != null)
{
// error: name not permitted in ITuple deconstruction
Expand Down Expand Up @@ -873,10 +900,12 @@ private void BindValueTupleSubpatterns(
for (int i = 0; i < node.Subpatterns.Count; i++)
{
var subpatternSyntax = node.Subpatterns[i];
// If the expression before the colon isn't just a name, we'll have reported a parsing error.
Debug.Assert(subpatternSyntax.NameColon is not null || subpatternSyntax.ExpressionColon is null || subpatternSyntax.ExpressionColon.HasErrors);
Copy link
Member

@alrz alrz May 21, 2021

Choose a reason for hiding this comment

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

When subpatternSyntax.ExpressionColon is null NameColon is also null. #Resolved


bool isError = i >= elementTypesWithAnnotations.Length;
TypeSymbol elementType = isError ? CreateErrorType() : elementTypesWithAnnotations[i].Type;
FieldSymbol? foundField = null;
// PROTOTYPE(extended-property-patterns) ExpressionColon
if (subpatternSyntax.NameColon != null && !isError)
{
string name = subpatternSyntax.NameColon.Name.Identifier.ValueText;
Expand Down
4 changes: 1 addition & 3 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1932,15 +1932,13 @@ internal enum ErrorCode

#region diagnostics introduced for C# 10.0

// PROTOTYPE(extended-property-patterns) Temp values
ERR_InvalidNameInSubpattern = 9000,

ERR_InheritingFromRecordWithSealedToString = 8912,
ERR_HiddenPositionalMember = 8913,
ERR_GlobalUsingInNamespace = 8914,
ERR_GlobalUsingOutOfOrder = 8915,
ERR_AttributesRequireParenthesizedLambdaExpression = 8916,
ERR_CannotInferDelegateType = 8917,
ERR_InvalidNameInSubpattern = 8918,

#endregion

Expand Down
5 changes: 1 addition & 4 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureGlobalUsing:
case MessageID.IDS_FeatureInferredDelegateType: // semantic check
case MessageID.IDS_FeatureLambdaAttributes: // semantic check
case MessageID.IDS_FeatureExtendedPropertyPatterns:
return LanguageVersion.Preview;

// C# 9.0 features.
Expand Down Expand Up @@ -499,10 +500,6 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureSwitchOnBool: // Checked in the binder.
return LanguageVersion.CSharp2;

// PROTOTYPE(extended-property-patterns) Move
case MessageID.IDS_FeatureExtendedPropertyPatterns:
return LanguageVersion.Preview;

// Special C# 2 feature: only a warning in C# 1.
case MessageID.IDS_FeatureModuleAttrLoc:
return LanguageVersion.CSharp1;
Expand Down
138 changes: 72 additions & 66 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1580,87 +1580,93 @@ public override void VisitPattern(BoundPattern pattern)
base.VisitPattern(pattern);
var whenFail = StateWhenFalse;
SetState(StateWhenTrue);
AssignPatternVariables(pattern);
assignPatternVariablesAndMarkReadProperties(pattern);
SetConditionalState(this.State, whenFail);
}

/// <summary>
/// Find the pattern variables of the pattern, and make them definitely assigned if <paramref name="definitely"/>.
/// That would be false under "not" and "or" patterns.
/// </summary>
private void AssignPatternVariables(BoundPattern pattern, bool definitely = true)
{
switch (pattern.Kind)
// Find the pattern variables of the pattern, and make them definitely assigned if <paramref name="definitely"/>.
// That would be false under "not" and "or" patterns.
void assignPatternVariablesAndMarkReadProperties(BoundPattern pattern, bool definitely = true)
{
case BoundKind.DeclarationPattern:
{
var pat = (BoundDeclarationPattern)pattern;
if (definitely)
Assign(pat, value: null, isRef: false, read: false);
break;
}
case BoundKind.DiscardPattern:
break;
case BoundKind.ConstantPattern:
{
var pat = (BoundConstantPattern)pattern;
this.VisitRvalue(pat.Value);
switch (pattern.Kind)
{
case BoundKind.DeclarationPattern:
{
var pat = (BoundDeclarationPattern)pattern;
if (definitely)
Assign(pat, value: null, isRef: false, read: false);
break;
}
case BoundKind.DiscardPattern:
break;
}
case BoundKind.RecursivePattern:
{
var pat = (BoundRecursivePattern)pattern;
if (!pat.Deconstruction.IsDefaultOrEmpty)
case BoundKind.ConstantPattern:
{
var pat = (BoundConstantPattern)pattern;
this.VisitRvalue(pat.Value);
break;
}
case BoundKind.RecursivePattern:
{
foreach (var subpat in pat.Deconstruction)
var pat = (BoundRecursivePattern)pattern;
if (!pat.Deconstruction.IsDefaultOrEmpty)
{
AssignPatternVariables(subpat.Pattern, definitely);
foreach (var subpat in pat.Deconstruction)
{
assignPatternVariablesAndMarkReadProperties(subpat.Pattern, definitely);
}
}
if (!pat.Properties.IsDefaultOrEmpty)
{
foreach (BoundSubpattern sub in pat.Properties)
{
if (sub is BoundPropertySubpattern { Member: var member } )
{
while (member is not null && !member.HasErrors)
Copy link
Member

@cston cston May 24, 2021

Choose a reason for hiding this comment

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

!member.HasErrors

Is this check necessary? #Closed

{
NoteRead(member.Symbol);
member = member.Receiver;
}
}
assignPatternVariablesAndMarkReadProperties(sub.Pattern, definitely);
}
}
if (definitely)
Assign(pat, null, false, false);
break;
}
if (!pat.Properties.IsDefaultOrEmpty)
case BoundKind.ITuplePattern:
{
foreach (BoundSubpattern sub in pat.Properties)
var pat = (BoundITuplePattern)pattern;
foreach (var subpat in pat.Subpatterns)
{
AssignPatternVariables(sub.Pattern, definitely);
assignPatternVariablesAndMarkReadProperties(subpat.Pattern, definitely);
}
break;
}
if (definitely)
Assign(pat, null, false, false);
case BoundKind.TypePattern:
break;
}
case BoundKind.ITuplePattern:
{
var pat = (BoundITuplePattern)pattern;
foreach (var subpat in pat.Subpatterns)
case BoundKind.RelationalPattern:
{
AssignPatternVariables(subpat.Pattern, definitely);
var pat = (BoundRelationalPattern)pattern;
this.VisitRvalue(pat.Value);
break;
}
break;
}
case BoundKind.TypePattern:
break;
case BoundKind.RelationalPattern:
{
var pat = (BoundRelationalPattern)pattern;
this.VisitRvalue(pat.Value);
break;
}
case BoundKind.NegatedPattern:
{
var pat = (BoundNegatedPattern)pattern;
AssignPatternVariables(pat.Negated, definitely: false);
break;
}
case BoundKind.BinaryPattern:
{
var pat = (BoundBinaryPattern)pattern;
bool def = definitely && !pat.Disjunction;
AssignPatternVariables(pat.Left, def);
AssignPatternVariables(pat.Right, def);
break;
}
default:
throw ExceptionUtilities.UnexpectedValue(pattern.Kind);
case BoundKind.NegatedPattern:
{
var pat = (BoundNegatedPattern)pattern;
assignPatternVariablesAndMarkReadProperties(pat.Negated, definitely: false);
break;
}
case BoundKind.BinaryPattern:
{
var pat = (BoundBinaryPattern)pattern;
bool def = definitely && !pat.Disjunction;
assignPatternVariablesAndMarkReadProperties(pat.Left, def);
assignPatternVariablesAndMarkReadProperties(pat.Right, def);
break;
}
default:
throw ExceptionUtilities.UnexpectedValue(pattern.Kind);
}
}
}

Expand Down
Loading