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

Using declaration lowering rewrite #31674

Merged
merged 22 commits into from Dec 21, 2018

Conversation

@chsienki
Copy link
Contributor

chsienki commented Dec 10, 2018

Refactor LocalUsingVarRewriter:

  • Look for using declarations as we lower a block: if we find them, skip them and lower them afterwards in a second stage
  • Remove existing LocalUsingVarRewriter pass

Disallow goto's across using declarations as per LDM decision

Fix data flow pass to not report using declarations as unused

Fixes #28707

@chsienki chsienki requested a review from dotnet/roslyn-compiler as a code owner Dec 10, 2018

@chsienki

This comment has been minimized.

Copy link
Contributor

chsienki commented Dec 10, 2018

@dotnet/roslyn-compiler for review please

immutableBoundStatements);
}

private static void CheckForInvalidGotos(ImmutableArray<BoundStatement> immutableBoundStatements, DiagnosticBag diagnostics)

This comment has been minimized.

@alrz

alrz Dec 11, 2018

Contributor

Could this be done in ControlFlowPass? just like goto in finally.

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

This seems reasonable

chsienki added some commits Dec 6, 2018

Fix data flow pass to not report using declarations as unused:
 - Report using declaration variables as read
- Add test
Disallow goto's across using declarations as per LDM decision
- Check during statement list binding that goto's don't cross the declaration
- Add a new error and report it
- Add tests
Refactor LocalUsingVarRewriter:
- Delete LocalUsingVarRewriter
- Look for using declarations as we lower a block: if we find them, skip them and lower them afterwards in a second stage
- Split UsingStatement lowering into visit + make methods
- Split LabelStatement lowering into visit + make methods
- Correctly lower using declarations with instrumentation
- Add tests

@chsienki chsienki force-pushed the chsienki:using_declaration_rewrite branch from 03decd5 to 255370e Dec 11, 2018

@agocke agocke self-assigned this Dec 12, 2018

immutableBoundStatements);
}

private static void CheckForInvalidGotos(ImmutableArray<BoundStatement> immutableBoundStatements, DiagnosticBag diagnostics)

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

This seems reasonable


private static void CheckForInvalidGotos(ImmutableArray<BoundStatement> immutableBoundStatements, DiagnosticBag diagnostics)
{
// PROTOTYPE: Can we do this more efficiently than n^3?

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

You could walk through the method first, keeping a list of labels and using statements ordered by location. Then, walk through again checking each goto by binary-searching the using statements list for a location in-between.

@@ -65,5 +65,29 @@ public static bool IsConstructorInitializer(this BoundCall call)
node.WasCompilerGenerated = true;
return node;
}

public static bool ContainsUsingDeclarationStatement(this BoundStatement statement)

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

Is this overload really worth it?

// every variable declared in a using local declaration is implicity read when it goes out of scope and it is disposed
foreach (var decl in node.LocalDeclarations)
{
NoteRead(decl.LocalSymbol);

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

This read is happening at the wrong time, because it's happening where the using is declared, not where it's disposed.

{
statement = (BoundStatement)Visit(statement);
}
if (statement != null) builder.Add(statement);

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

Single-line if's are prohibited in our style rules.

BoundStatement statement = node.Statements[i];
if (statement.ContainsUsingDeclarationStatement())
{
usingDeclarationIndicies.Add(i);

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

This is more complicated than I expected. Could we do this all in one pass by exploiting recursion to visit every statement range starting after the using?


private void LowerPendingUsingDeclarations(ArrayBuilder<BoundStatement> statements, ImmutableArray<int> unloweredDeclarationIndicies)
{
// work backwards and lower the using declarations

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

Working backwards originally seemed like a good idea to me, but I think doing as much natural recursion as possible would be even simpler. The advantage of doing it this way is avoiding blowing the stack, but I'm not sure how much it's worth it.

ImmutableArray<BoundLocalDeclaration> declarations = node.DeclarationsOpt.LocalDeclarations;

BoundBlock result = tryBlock;
return MakeDeclarationUsingStatement(node.Syntax, tryBlock, node.Locals, node.DeclarationsOpt, node.IDisposableConversion, node.DisposeMethodOpt, node.AwaitOpt, awaitKeyword);

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

Time to split this across multiple lines, I think

{
result = RewriteDeclarationUsingStatement(usingSyntax, declarations[i], result, idisposableConversion, awaitKeyword, node.AwaitOpt, node.DisposeMethodOpt);
}
public BoundNode MakeDeclarationUsingStatement(SyntaxNode syntax, BoundBlock body, ImmutableArray<LocalSymbol> locals, BoundMultipleLocalDeclarations declarations, Conversion iDisposableConversion, MethodSymbol disposeMethodOpt, AwaitableInfo awaitOpt, SyntaxToken awaitKeyword)

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

Same here

chsienki added some commits Dec 13, 2018

PR Feedback:
- Remove extra overload
- Formatting in local rewriter
Recursively lower using declarations:
Visit a 'sublist' of statements in a block:
- For each statement in the sublist, visit with the set of statements after it:
    - If the statement is a label, recursively visit the body with the same set of following statements
    - If the statement is a using declaration, recursively visit the following statements and use those as the body of the using
    - Other statements are visited as normal
- If we saw a using during the statement visit, we've finished lowering. If not, continue down the list

Add test to handle multi-label scenarios
Rewrite the invalid got detection logic:
- Only run if we see gotos
- Iterate only once, recording the locations of gotos,labels and usings
- Check if the detected locations are invalid and error
ArrayBuilder<BoundStatement> boundStatements = ArrayBuilder<BoundStatement>.GetInstance(nStatements);

for (int i = 0; i < nStatements; i++)
{
var boundStatement = BindStatement(syntaxStatements[i], diagnostics);
boundStatements.Add(boundStatement);

sawGotos |= boundStatement.Kind == BoundKind.GotoStatement;

This comment has been minimized.

@cston

cston Dec 14, 2018

Member

boundStatement.Kind [](start = 28, length = 19)

Are we handling nested gotos?

{
    goto L1;
}
using expr;
L1: ;

This comment has been minimized.

@chsienki

chsienki Dec 18, 2018

Contributor

We weren't. I've re-written this as part of control flow as it makes the logic much simpler

{
return labelDictionary.ContainsKey(symbol)
? labelDictionary[symbol]
: (-1, -1);

This comment has been minimized.

@cston

cston Dec 14, 2018

Member

Please use labelDictionary.TryGetValue() to avoid two lookups.

foreach((int gotoIndex, int labelIndex) in labelDictionary.Values)
{
// negative indicates we didn't see a goto or label for this LabelSymbol
if(gotoIndex == -1 || labelIndex == -1)

This comment has been minimized.

@cston

cston Dec 14, 2018

Member

if(gotoIndex == -1 || labelIndex == -1) [](start = 16, length = 39)

Is this ever true?

immutableBoundStatements);
}

private static void CheckForInvalidGotos(ImmutableArray<BoundStatement> immutableBoundStatements, DiagnosticBag diagnostics)

This comment has been minimized.

@cston

cston Dec 14, 2018

Member

immutableBoundStatements [](start = 80, length = 24)

Perhaps just statements. #Resolved

int start = Math.Min(gotoIndex, labelIndex);
int end = Math.Max(gotoIndex, labelIndex);

//include start statement, as it could be a labled using, exclude the end because a labeled using is valid

This comment has been minimized.

@cston

cston Dec 14, 2018

Member

labled [](start = 60, length = 6)

Typo

//include start statement, as it could be a labled using, exclude the end because a labeled using is valid
for(int i = start; i < end; i++)
{
if (usingStatementIndicies.Contains(i))

This comment has been minimized.

@cston

cston Dec 14, 2018

Member

Is the foreach O(n*m) where n is number of gotos and m is the number of labels?

@chsienki chsienki force-pushed the chsienki:using_declaration_rewrite branch from 48f5901 to f3d9fe6 Dec 18, 2018

Rewrite goto across declaration as part of control flow pass:
- Note the using variables in a block, and currently in scope
- When visiting a goto, check the location of the branch and error if it crosses illegally
- Add an extra error to distinguish between forward and backward goto errors
- Add tests

@chsienki chsienki force-pushed the chsienki:using_declaration_rewrite branch from f3d9fe6 to 9d30b42 Dec 18, 2018

@chsienki

This comment has been minimized.

Copy link
Contributor

chsienki commented Dec 18, 2018

@dotnet/roslyn-compiler For second review please. I've re-written the goto-over-using declaration analysis as part of the control flow, and it seems much simpler / more robust.

{
if (true)
{
goto label1;

This comment has been minimized.

@cston

cston Dec 18, 2018

Member

goto label1; [](start = 16, length = 12)

Why is this allowed? If the using is crossed once, are two instances disposed or one? Please add a test that executes Dispose in such a scenario.

This comment has been minimized.

@chsienki

chsienki Dec 18, 2018

Contributor

Yes. We cross twice and dispose twice. It was explicitly allowed by LDM. Will add a test to validate multi-disposal.

This comment has been minimized.

@chsienki

This comment has been minimized.

@cston

cston Dec 18, 2018

Member

From the LDM notes, it seems the above example should be an error:

When gotoing a label in the same statement list as a using var, the goto must not have a using var syntactically between the goto and the label.

This comment has been minimized.

@chsienki

chsienki Dec 18, 2018

Contributor

We should check with LDM, but my understanding from the meeting was that 'statement list' meant it was on the same level in the block. We discussed that jumping back from a lower block should be allowed.

This comment has been minimized.

@chsienki

chsienki Dec 18, 2018

Contributor

Discussed offline and I was wrong here. I've updated the algorithm to handle it correctly: backwards jumps are now disallowed when the label and the using declaration are part of the same block.

}

[Fact]
public void VariableCannotBeReAssignedAsUsingVariable()

This comment has been minimized.

@cston

cston Dec 18, 2018

Member

VariableCannotBeReAssignedAsUsingVariable [](start = 20, length = 41)

Is the test name correct? And perhaps remove the test unless it is testing a distinct scenario.

// goto label1;
Diagnostic(ErrorCode.ERR_GoToBackwardJumpOverUsingVar, "goto label1;").WithLocation(10, 9)
);
}

This comment has been minimized.

@cston

cston Dec 18, 2018

Member

} [](start = 8, length = 1)

Consider testing goto case:

switch (value)
{
    case A:
        using (GetDisposable());
        goto case A;
}
``` #Resolved

This comment has been minimized.

@chsienki

chsienki Dec 18, 2018

Contributor

That isn't valid, as we don't allow using declarations in raw switch blocks. We have a test for this already in UsingStatementTests.UsingVarInSwitchCase

I'll move it over to the declarations class to make it clearer though.

This comment has been minimized.

@cston

cston Dec 18, 2018

Member

In that case, consider testing:

switch (value)
{
case A:
    {
        using (GetDisposable());
        goto case A;
    }
}

In reply to: 242682342 [](ancestors = 242682342)

}
else if (statement is BoundLabeledStatement label)
{
if (label.Body is BoundUsingLocalDeclarations localLabelDeclaration)

This comment has been minimized.

@cston

cston Dec 18, 2018

Member

label.Body is [](start = 20, length = 13)

Does this handle nested labels?

L1:
L2:
    using (var d =  expr);
``` #Resolved

This comment has been minimized.

@chsienki

chsienki Dec 18, 2018

Contributor

(Removed as unused)

public static bool ContainsUsingDeclarationStatement(this BoundStatement statement, out BoundUsingLocalDeclarations declaration)
{
declaration = null;
if (statement is BoundUsingLocalDeclarations localDeclaration)

This comment has been minimized.

@cston

cston Dec 18, 2018

Member

is [](start = 26, length = 2)

Consider using switch (statement.Kind) rather than is. #Resolved

This comment has been minimized.

@chsienki

chsienki Dec 18, 2018

Contributor

(Removed as unused)

Debug.Assert(_labelsDefined.ContainsKey(node.Label));

// Error if label and using are part of the same block
if (_labelsDefined[node.Label].Locals.Contains(usingDecl))

This comment has been minimized.

@cston

cston Dec 19, 2018

Member

_labelsDefined[node.Label].Locals.Contains(usingDecl) [](start = 24, length = 53)

Perhaps use PooledDictionary<LocalSymbol, BoundBlock> _usingDeclarations so this check can be a simple comparison: _labelsDefined[node.Label] == usingDecl.Value. #Resolved

var sourceStart = node.Syntax.Location.SourceSpan.Start;
var targetStart = node.Label.Locations[0].SourceSpan.Start;

foreach (var usingDecl in _usingDeclarations)

This comment has been minimized.

@cston

cston Dec 19, 2018

Member

The approach is O(n^m) with n gotos and m using declarations. Would it make sense to store the using declarations in an ArrayBuilder in order, and then search for the goto and target locations using a binary search of that list?

This comment has been minimized.

@chsienki

chsienki Dec 19, 2018

Contributor

I started prototyping this, but it got super complicated. We need for the using declaration to be in order, but we add them all as we visit the block. That means we'd need to insert and remove sub-block entries as we descend, and I think that actually ends up being more work. We also can't just consider the ones in the current block for forward, because we might be jumping out into a parent block, but past usings we haven't visited yet.

If you're still concerned we could create a bug and come back and think about the perf later on?

chsienki added some commits Dec 19, 2018

PR Feedback:
- Store block along with using for constant time same block check
- Change usingdeclaration to be arraybuilder rather than hashset
@@ -295,7 +298,7 @@ protected override void VisitFinallyBlock(BoundStatement finallyBlock, ref Local

protected override void VisitLabel(BoundLabeledStatement node)
{
_labelsDefined.Add(node.Label);
_labelsDefined[node.Label] = _currentBlock;

This comment has been minimized.

@cston

cston Dec 19, 2018

Member

_labelsDefined[node.Label] = _currentBlock [](start = 12, length = 42)

Minor point: Consider using _labelsDefined.Add(node.Label, _currentBlock) so an exception is thrown in the unexpected case of a duplicated label.

This comment has been minimized.

@chsienki

chsienki Dec 20, 2018

Contributor

So, I did this, but the bootstrap compiler was crashing due to a duplicate label :/

which I confess I didn't actually understand how...

var stmt = (BoundStatement)Visit(node.Statements[i]);
if (stmt != null) builder.Add(stmt);
}
builder.AddRange(VisitStatementSubList(node.Statements));

This comment has been minimized.

@cston

cston Dec 19, 2018

Member

VisitStatementSubList [](start = 29, length = 21)

Consider passing builder in as an argument.

var builder = ArrayBuilder<BoundStatement>.GetInstance();
for (int i = startIndex; i < statements.Length; i++)
{
BoundStatement statement = (BoundStatement)VisitPossibleUsingDeclaration(statements[i], statements, i, out var replacedUsingDeclarations);

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

VisitPossibleUsingDeclaration [](start = 59, length = 29)

Consider declaring VisitPossibleUsingDeclaration as returning a BoundStatement. #Resolved

This comment has been minimized.

@chsienki

chsienki Dec 20, 2018

Contributor

Done

for (int i = startIndex; i < statements.Length; i++)
{
BoundStatement statement = (BoundStatement)VisitPossibleUsingDeclaration(statements[i], statements, i, out var replacedUsingDeclarations);
if (statement != null)

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

statement != null [](start = 20, length = 17)

Is the result ever null? #Resolved

This comment has been minimized.

@chsienki

chsienki Dec 20, 2018

Contributor

The previous code called Visit(node) and checked for null. We eventually call down this path for non-labeled or using statements, so I believe it's possible.

/// Visits a node that is possibly a <see cref="BoundUsingLocalDeclarations"/>
/// </summary>
/// <param name="node">The node to visit</param>
/// <param name="statements">A list of statements that follow this node</param>

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

statements that follow this node [](start = 47, length = 32)

Perhaps all statements in the block containing this node #Resolved

@@ -35,40 +35,75 @@ public override BoundNode VisitUsingStatement(BoundUsingStatement node)
? (BoundBlock)rewrittenBody
: BoundBlock.SynthesizedNoLocals(node.Syntax, rewrittenBody);

SyntaxToken awaitKeyword = node.Syntax.Kind() == SyntaxKind.UsingStatement ? ((UsingStatementSyntax)node.Syntax).AwaitKeyword : default;

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

awaitKeyword [](start = 24, length = 12)

Declaration can be moved into else.

private BoundStatement MakeLocalUsingDeclarationStatement(BoundUsingLocalDeclarations usingDeclarations, ImmutableArray<BoundStatement> statements)
{
SyntaxNode syntax = usingDeclarations.Syntax;
BoundBlock body = new BoundBlock(syntax, ImmutableArray<LocalSymbol>.Empty, statements);

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

BoundBlock [](start = 34, length = 10)

Is there a BoundBlock that contains the locals from the declaration?

This comment has been minimized.

@chsienki

chsienki Dec 20, 2018

Contributor

Yes, the original parent block it was declared in. All the synthesized blocks generated as part of lowering the using declarations have no locals, and they all belong to the original block.

using var x = (IDisposable)null;
goto label1;
}

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

} [](start = 4, length = 1)

Please test nested label:

label1:
label2:
    using var x = GetDisposable();
    goto label2;
``` #Resolved
}

[Fact]
public void DissallowGoToAccrossUsingDeclarationsComplexTest()

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

Dissallow [](start = 20, length = 9)

Typo #Resolved

}

[Fact]
public void DissallowGoToAccrossUsingDeclarationsComplexTest()

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

Accross [](start = 33, length = 7)

Typo #Resolved

/// </summary>
/// <param name="node">The node to visit</param>
/// <param name="statements">A list of statements that follow this node</param>
/// <param name="startIndex">The startIndex of statements</param>

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

statements [](start = 55, length = 10)

Consider clarifying that startIndex represents the current statement rather than the following statement. #Resolved

chsienki added some commits Dec 20, 2018

{
public static void Main()
{
using var _ = new C1();

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

var _ [](start = 14, length = 5)

Is this is a discard or a local named "_"?

This comment has been minimized.

@chsienki

chsienki Dec 21, 2018

Contributor

Removed. This was a sleepy me being stupid last night and just testing a variable named discard.

static async Task Main()
{
using IDisposable x = await GetDisposable();
}

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

} [](start = 4, length = 1)

Are we testing execution of such a method, to ensure Dispose is called if and only if the await completes successfully? #Resolved

var source = @"
using System;
class C1 : IDisposable
{

This comment has been minimized.

@cston

cston Dec 20, 2018

Member
[](start = 0, length = 4)

The indenting is off. #Resolved

}";
CreateCompilation(source).VerifyDiagnostics(
// (11,15): error CS0819: Implicitly-typed variables cannot have multiple declarators
// using (var c1 = new C1(), c2 = new C2())

This comment has been minimized.

@cston

cston Dec 20, 2018

Member

using (var c1 = new C1(), c2 = new C2()) [](start = 25, length = 40)

Comment is stale. #Resolved

@cston

cston approved these changes Dec 20, 2018

Copy link
Member

cston left a comment

Minor comments only.

@jaredpar
Copy link
Member

jaredpar left a comment

:shipit:

chsienki added some commits Dec 21, 2018

@chsienki chsienki merged commit a22a1e0 into dotnet:features/enhanced-using Dec 21, 2018

2 checks passed

license/cla All CLA requirements met.
Details
roslyn-CI #20181220.52 succeeded
Details
@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Dec 21, 2018

The approach is O(n^m) with n gotos and m using declarations.

@cston @chsienki I took a stab at improving things there. If this looks good I'll open a PR

alrz@9ee8b3e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment