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

List-patterns: factor binding logic #57318

Merged
merged 33 commits into from
Nov 20, 2021
Merged
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
025f398
Address some PROTOTYPE markers
jcouv Oct 20, 2021
00e6ff2
Address feedback
jcouv Oct 26, 2021
b1112ce
Merge remote-tracking branch 'dotnet/features/list-patterns' into lis…
jcouv Oct 27, 2021
8d16d55
Rename
jcouv Nov 1, 2021
05f3611
More rename
jcouv Nov 1, 2021
1c3417a
fix incorrect rename
jcouv Nov 1, 2021
51be8eb
Revert most of the renaming
jcouv Nov 2, 2021
c3f1618
Use-site info
jcouv Nov 2, 2021
ddb617c
Merge remote-tracking branch 'dotnet/features/list-patterns' into lis…
jcouv Nov 8, 2021
d69d920
WIP on refactoring
jcouv Nov 2, 2021
39ea1f6
update tests
jcouv Nov 8, 2021
91a7ffa
CheckValue on placeholder
jcouv Nov 8, 2021
7f36c4f
Merge remote-tracking branch 'dotnet/features/list-patterns' into lis…
jcouv Nov 11, 2021
0f924db
Redo lowering change. Store receiver in IndexerAccess
jcouv Nov 11, 2021
d1288ed
Address feedback
jcouv Nov 12, 2021
adfee13
Address feedback (2)
jcouv Nov 13, 2021
dbdbe39
Address feedback (3)
jcouv Nov 15, 2021
a1a4243
Disable caching that is not necessary during a list pattern evaluation.
AlekseyTs Nov 17, 2021
c9f67d1
Merge remote-tracking branch 'features/list-patterns' into ImplicitIn…
AlekseyTs Nov 17, 2021
05edc54
Address feedback (4)
jcouv Nov 17, 2021
1912f37
Fix NullabilityRewriter
jcouv Nov 17, 2021
cd501b4
tweaks
jcouv Nov 17, 2021
aab5d74
compiler generated
jcouv Nov 17, 2021
8be23fa
or
jcouv Nov 17, 2021
6c132fa
Fix order of evaluation for nullability
jcouv Nov 17, 2021
a4ba54b
Clean up handling of an implicit indexer as a target of an assignment…
AlekseyTs Nov 18, 2021
dff82c5
Address feedback (5)
jcouv Nov 18, 2021
9def40b
Improve SemanticModel handling of receiver under BoundIndexOrRangePat…
AlekseyTs Nov 18, 2021
2965f6d
Check implicit copy directly
jcouv Nov 18, 2021
dea39c5
Address feedback (6)
jcouv Nov 19, 2021
174d3f2
Fix 2 test baselines with missing members
jcouv Nov 19, 2021
bcc3336
tweak
jcouv Nov 20, 2021
76d6d7b
Rename to ImplicitIndexer and IndexerOrSliceAccess
jcouv Nov 20, 2021
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
Prev Previous commit
Next Next commit
CheckValue on placeholder
jcouv committed Nov 9, 2021
commit 91a7ffa4e29b6c42e94e55818199e7972c6eb55e
39 changes: 38 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
@@ -597,7 +597,11 @@ internal bool CheckValueKind(SyntaxNode node, BoundExpression expr, BindValueKin
return CheckValueKind(node, implicitIndexerAccess.IndexerAccess, valueKind, checkingReceiver, diagnostics);

case BoundKind.IndexOrRangeIndexerPatternReceiverPlaceholder:
return true;
var receiverPlaceholder = (BoundIndexOrRangeIndexerPatternReceiverPlaceholder)expr;
return CheckValueKind(node, receiverPlaceholder.Expression, valueKind, checkingReceiver, diagnostics);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 10, 2021

Choose a reason for hiding this comment

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

receiverPlaceholder.Expression

return CheckValueKind(node, receiverPlaceholder.Expression, valueKind, checkingReceiver, diagnostics);


Note to self, please, ignore. #Closed


case BoundKind.DeconstructValuePlaceholder:
break;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 10, 2021

Choose a reason for hiding this comment

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

break;

Is this code path covered by tests? It is not obvious why returning false is the right thing to do for it. #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's covered by DeconstructRefExtensionMethod. The cases the only need to use the expression as an r-value are handled above (line 418, RequiresRValueOnly case).


case BoundKind.ConditionalOperator:
var conditional = (BoundConditionalOperator)expr;
@@ -622,13 +626,46 @@ internal bool CheckValueKind(SyntaxNode node, BoundExpression expr, BindValueKin
case BoundKind.AssignmentOperator:
var assignment = (BoundAssignmentOperator)expr;
return CheckSimpleAssignmentValueKind(node, assignment, valueKind, diagnostics);

default:
Debug.Assert(expr is not BoundValuePlaceholderBase, $"Placeholder kind {expr.Kind} should be explicitly handled");
break;
}

// At this point we should have covered all the possible cases for anything that is not a strict RValue.
Error(diagnostics, GetStandardLvalueError(valueKind), node);
return false;
}

private static BoundExpression UnwrapPlaceholdersIfNeeded(BoundExpression? e)
{
switch (e)
{
case null:
return null;

case BoundIndexOrRangeIndexerPatternReceiverPlaceholder placeholder:
return placeholder.Expression;

case BoundSlicePatternReceiverPlaceholder:
case BoundListPatternReceiverPlaceholder:
case BoundObjectOrCollectionValuePlaceholder:
case BoundAwaitableValuePlaceholder:
case BoundInterpolatedStringHandlerPlaceholder:
case BoundDisposableValuePlaceholder:
// TODO2
return e;

case BoundDeconstructValuePlaceholder:
// PROTOTYPE file issue
return e;

default:
Debug.Assert(e is not BoundValuePlaceholderBase, $"Placeholder kind {e.Kind} should be explicitly handled");
return e;
}
}

private static bool CheckNotNamespaceOrType(BoundExpression expr, BindingDiagnosticBag diagnostics)
{
switch (expr.Kind)
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
@@ -8056,7 +8056,7 @@ private bool TryBindIndexOrRangeImplicitIndexer(
_ => GetValEscape(receiverOpt, LocalScopeDepth)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 16, 2021

Choose a reason for hiding this comment

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

GetValEscape(receiverOpt, LocalScopeDepth)

Can we simplify code, always do that and get rid of the switch? #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.

Indeed. Thanks

};

var receiverPlaceholder = new BoundIndexOrRangeIndexerPatternReceiverPlaceholder(receiverOpt.Syntax, receiverValEscape, receiverOpt.Type) { WasCompilerGenerated = true };
var receiverPlaceholder = new BoundIndexOrRangeIndexerPatternReceiverPlaceholder(receiverOpt.Syntax, receiverValEscape, receiverOpt, receiverOpt.Type) { WasCompilerGenerated = true };
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 10, 2021

Choose a reason for hiding this comment

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

receiverOpt

var receiverPlaceholder = new BoundIndexOrRangeIndexerPatternReceiverPlaceholder(receiverOpt.Syntax, receiverValEscape, receiverOpt, receiverOpt.Type) { WasCompilerGenerated = true };


It looks like here we can create a placeholder on top of another placeholder. Feels strange. #Closed

if (!TryBindIndexOrRangeImplicitIndexer(syntax, receiverPlaceholder, receiverType, argIsIndex: argIsIndex,
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 10, 2021

Choose a reason for hiding this comment

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

receiverType

if (!TryBindIndexOrRangeImplicitIndexer(syntax, receiverPlaceholder, receiverType, argIsIndex: argIsIndex,


Why do we need to pass this explicitly? #Closed

out var lengthOrCountAccess, out var indexerOrSliceAccess, out var argumentPlaceholders, diagnostics))
Copy link
Member

@333fred 333fred Nov 18, 2021

Choose a reason for hiding this comment

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

Nit: consider indenting #Closed

{
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
Original file line number Diff line number Diff line change
@@ -1464,7 +1464,7 @@ internal bool CheckImplicitThisCopyInReadOnlyMember(BoundExpression receiver, Me
{
// For now we are warning only in implicit copy scenarios that are only possible with readonly members.
// Eventually we will warn on implicit value copies in more scenarios. See https://github.com/dotnet/roslyn/issues/33968.
if (receiver is BoundThisReference &&
if (UnwrapPlaceholdersIfNeeded(receiver) is BoundThisReference &&
Copy link
Contributor

Choose a reason for hiding this comment

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

UnwrapPlaceholdersIfNeeded

Note to self, please, ignore.

receiver.Type.IsValueType &&
ContainingMemberOrLambda is MethodSymbol containingMethod &&
containingMethod.IsEffectivelyReadOnly &&
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
@@ -143,6 +143,8 @@
<Node Name="BoundIndexOrRangeIndexerPatternReceiverPlaceholder" Base="BoundValuePlaceholderBase">
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>
<Field Name="ValEscape" Type="uint" Null="NotApplicable"/>
<!-- For CheckValue -->
<Field Name="Expression" Type="BoundExpression"/>
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 9, 2021

Choose a reason for hiding this comment

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


Is this a duplication of information? #Closed

</Node>

<!-- This node represents the receiver for a list pattern. It does not survive lowering -->
333fred marked this conversation as resolved.
Show resolved Hide resolved
48 changes: 27 additions & 21 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs

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
@@ -975,16 +975,20 @@ internal static bool CanBePassedByReference(BoundExpression expr)
case BoundKind.IndexOrRangePatternIndexerAccess:
return CanBePassedByReference(((BoundIndexOrRangePatternIndexerAccess)expr).IndexerAccess);

// TODO2 I'm not sure about this
case BoundKind.IndexOrRangeIndexerPatternReceiverPlaceholder:
case BoundKind.IndexOrRangeIndexerPatternValuePlaceholder:
case BoundKind.ListPatternReceiverPlaceholder:
case BoundKind.ListPatternUnloweredIndexPlaceholder:
case BoundKind.SlicePatternReceiverPlaceholder:
case BoundKind.SlicePatternUnloweredRangePlaceholder:
return true;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 10, 2021

Choose a reason for hiding this comment

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

return true;

return true;


It is not clear why is this the right value to return? Why do we even run into them? #Closed


case BoundKind.Conversion:
return expr is BoundConversion { Conversion: { IsInterpolatedStringHandler: true }, Type: { IsValueType: true } };
}

Debug.Assert(expr is not BoundValuePlaceholderBase, $"Placeholder kind {expr.Kind} must be handled explicitly");

return false;
}

Original file line number Diff line number Diff line change
@@ -588,7 +588,8 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
case BoundKind.IndexOrRangePatternIndexerAccess:
{
var implicitIndexerAccess = (BoundIndexOrRangePatternIndexerAccess)originalLHS;
if (implicitIndexerAccess.GetRefKind() == RefKind.None)
Debug.Assert(implicitIndexerAccess.Argument.Type!.Equals(_compilation.GetWellKnownType(WellKnownType.System_Index)));
if (implicitIndexerAccess.IndexerAccess.GetRefKind() == RefKind.None)
{
return TransformImplicitIndexerAccess(implicitIndexerAccess, stores, temps, isDynamicAssignment);
}
Original file line number Diff line number Diff line change
@@ -220,6 +220,7 @@ private BoundSequence VisitIndexOrRangePatternIndexerAccess(BoundIndexOrRangePat
private BoundSequence VisitIndexImplicitIndexerAccess(BoundIndexOrRangePatternIndexerAccess node, bool isLeftOfAssignment) // TODO2 we're not using isLeftOfAssignment
{
Debug.Assert(node.ArgumentPlaceholders.Length == 1);
Debug.Assert(node.IndexerAccess is BoundIndexerAccess);

Debug.Assert(TypeSymbol.Equals(
node.Argument.Type,
@@ -322,6 +323,7 @@ private BoundExpression MakePatternIndexOffsetExpression(
private BoundSequence VisitRangeImplicitIndexerAccess(BoundIndexOrRangePatternIndexerAccess node)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 10, 2021

Choose a reason for hiding this comment

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

VisitRangeImplicitIndexerAccess

private BoundSequence VisitRangeImplicitIndexerAccess(BoundIndexOrRangePatternIndexerAccess node)


Same comment about rename. #Closed

{
Debug.Assert(node.ArgumentPlaceholders.Length == 2);
Debug.Assert(node.IndexerAccess is BoundCall);

Debug.Assert(TypeSymbol.Equals(
node.Argument.Type,
18 changes: 12 additions & 6 deletions src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs
Original file line number Diff line number Diff line change
@@ -314,11 +314,10 @@ .locals init (int[] V_0, //array
public void PatternIndexCompoundOperator_InReadonlyMethod()
{
var src = @"
using System;
struct S
public struct S
{
private readonly int[] _array;
private int _counter;
public readonly int[] _array;
public int _counter;

public S(int[] a)
{
@@ -341,9 +340,16 @@ readonly void M()
}
}
";
// TODO2 missing "error CS1604: Cannot assign to 'this[^1]' because it is read-only"
// TODO2 why no CS8656 on the indexer?
var comp = CreateCompilationWithIndexAndRangeAndSpan(src);
comp.VerifyDiagnostics();
comp.VerifyDiagnostics(
// (24,9): warning CS8656: Call to non-readonly member 'S.Length.get' from a 'readonly' member results in an implicit copy of 'this'.
// this[^1] += 5;
Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "this").WithArguments("S.Length.get", "this").WithLocation(24, 9),
// (24,9): error CS1604: Cannot assign to 'this[^1]' because it is read-only
// this[^1] += 5;
Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "this[^1]").WithArguments("this[^1]").WithLocation(24, 9)
);
}

[Fact]
Original file line number Diff line number Diff line change
@@ -3016,7 +3016,43 @@ readonly void M(Index i, Range r)
_ = this is [2, ..var rest];
}
}";
// Note: no "warning CS8656: Call to non-readonly member ... from a 'readonly' member results in an implicit copy of 'this'"
var comp = CreateCompilationWithIndexAndRange(src);
comp.VerifyDiagnostics(
// (11,13): warning CS8656: Call to non-readonly member 'S.Length.get' from a 'readonly' member results in an implicit copy of 'this'.
// _ = this[i]; // 1, 2
Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "this").WithArguments("S.Length.get", "this").WithLocation(11, 13),
// (11,13): warning CS8656: Call to non-readonly member 'S.this[int].get' from a 'readonly' member results in an implicit copy of 'this'.
// _ = this[i]; // 1, 2
Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "this").WithArguments("S.this[int].get", "this").WithLocation(11, 13),
// (12,13): warning CS8656: Call to non-readonly member 'S.Slice(int, int)' from a 'readonly' member results in an implicit copy of 'this'.
// _ = this[r]; // 3, 4
Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "this").WithArguments("S.Slice(int, int)", "this").WithLocation(12, 13),
// (12,13): warning CS8656: Call to non-readonly member 'S.Length.get' from a 'readonly' member results in an implicit copy of 'this'.
// _ = this[r]; // 3, 4
Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "this").WithArguments("S.Length.get", "this").WithLocation(12, 13)
);
}

[Fact]
public void PatternIndexRangeReadOnly_02()
{
var src = @"
using System;
struct S
{
public readonly int this[int i] => 0;
public readonly int Length => 0;
public readonly int Slice(int x, int y) => 0;

readonly void M(Index i, Range r)
{
_ = this[i];
_ = this[r];

_ = this is [1];
_ = this is [2, ..var rest];
}
}";
var comp = CreateCompilationWithIndexAndRange(src);
comp.VerifyDiagnostics();
}
@@ -6297,7 +6333,7 @@ void Test(int[] a)
");
}

// PROTOTYPE this test takes 10 seconds to run...
// PROTOTYPE this test takes 7 seconds to run...
[Theory]
[CombinatorialData]
public void Subsumption_Slice_00(