From bd2d261bc6a82ae2bece034d4aba2cbc4e14c178 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Mon, 29 Aug 2022 03:27:53 +0000 Subject: [PATCH] Shared type analysis: add support for `when` clauses and fix label support. Support for `when` clauses requires flow analysis integration, so that `when` clauses can promote variables, e.g.: f(int x, String? y) { switch (x) { case 0 when y != null: // y is known to be non-null here } } Support for labels in switch statements had a small flaw: we weren't reporting an error in the case where a label shared a case body with a pattern that tried to bind a variable, e.g.: f(int x) { switch (x) { L: // Error: does not mind the variable `y` case var y: ... } } Change-Id: I0b2bb4721a6b3a8f7898df682b24b75ddb6e44ae Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256605 Commit-Queue: Paul Berry Reviewed-by: Konstantin Shcheglov --- .../lib/src/flow_analysis/flow_analysis.dart | 109 ++++++++++++- .../lib/src/type_inference/type_analyzer.dart | 89 ++++++++--- .../flow_analysis/flow_analysis_test.dart | 141 +++++++++++------ pkg/_fe_analyzer_shared/test/mini_ast.dart | 119 ++++++++++----- .../type_inference/type_inference_test.dart | 143 +++++++++++++----- .../test/spell_checking_list_code.txt | 1 + 6 files changed, 453 insertions(+), 149 deletions(-) diff --git a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart index f7d88b79d14..e315ad98426 100644 --- a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart +++ b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart @@ -530,6 +530,17 @@ abstract class FlowAnalysis? ssaNodeForTesting(Variable variable); + /// Call this method just after visiting a `when` part of a case clause. See + /// [switchStatement_expressionEnd] for details. + /// + /// [when] should be the expression following the `when` keyword. + void switchStatement_afterWhen(Expression when); + + /// Call this method just before visiting a sequence of two or more `case` or + /// `default` clauses that share a body. See [switchStatement_expressionEnd] + /// for details.` + void switchStatement_beginAlternatives(); + /// Call this method just before visiting one of the cases in the body of a /// switch statement. See [switchStatement_expressionEnd] for details. /// @@ -547,6 +558,16 @@ abstract class FlowAnalysis _wrapped.switchStatement_afterWhen(when)); + } + + @override + void switchStatement_beginAlternatives() { + _wrap('switchStatement_beginAlternatives()', + () => _wrapped.switchStatement_beginAlternatives()); + } + @override void switchStatement_beginCase(bool hasLabel, Statement? node) { _wrap('switchStatement_beginCase($hasLabel, $node)', @@ -1184,6 +1226,18 @@ class FlowAnalysisDebug _wrapped.switchStatement_end(isExhaustive)); } + @override + void switchStatement_endAlternative() { + _wrap('switchStatement_endAlternative()', + () => _wrapped.switchStatement_endAlternative()); + } + + @override + void switchStatement_endAlternatives() { + _wrap('switchStatement_endAlternatives()', + () => _wrapped.switchStatement_endAlternatives()); + } + @override void switchStatement_expressionEnd(Statement? switchStatement) { _wrap('switchStatement_expressionEnd($switchStatement)', @@ -3685,6 +3739,22 @@ class _FlowAnalysisImpl? ssaNodeForTesting(Variable variable) => _current .variableInfo[promotionKeyStore.keyForVariable(variable)]?.ssaNode; + @override + void switchStatement_afterWhen(Expression when) { + ExpressionInfo? expressionInfo = _getExpressionInfo(when); + if (expressionInfo != null) { + _current = expressionInfo.ifTrue; + } + } + + @override + void switchStatement_beginAlternatives() { + _current = _current.split(); + _SwitchAlternativesContext context = + new _SwitchAlternativesContext(_current); + _stack.add(context); + } + @override void switchStatement_beginCase(bool hasLabel, Statement? node) { _SimpleStatementContext context = @@ -3714,6 +3784,21 @@ class _FlowAnalysisImpl context = + _stack.last as _SwitchAlternativesContext; + context._combinedModel = _join(context._combinedModel, _current); + _current = context._previous; + } + + @override + void switchStatement_endAlternatives() { + _SwitchAlternativesContext context = + _stack.removeLast() as _SwitchAlternativesContext; + _current = context._combinedModel!.unsplit(); + } + @override void switchStatement_expressionEnd(Statement? switchStatement) { _current = _current.split(); @@ -4514,12 +4599,24 @@ class _LegacyTypePromotion 'checkpoint: $_checkpoint)'; } +class _SwitchAlternativesContext extends _FlowContext { + final FlowModel _previous; + + FlowModel? _combinedModel; + + _SwitchAlternativesContext(this._previous); +} + /// Specialization of [ExpressionInfo] for the case where the information we /// have about the expression is trivial (meaning we know by construction that /// the expression's [after], [ifTrue], and [ifFalse] models are all the same). diff --git a/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart b/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart index cdec2a3aad7..a84e2dc19a0 100644 --- a/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart +++ b/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart @@ -14,29 +14,36 @@ class ExpressionCaseInfo { /// For a `case` clause, the case pattern. For a `default` clause, `null`. final Node? pattern; + /// For a `case` clause that has a `when` part, the expression following + /// `when`. Otherwise `null`. + final Expression? when; + /// The body of the `case` or `default` clause. final Expression body; - ExpressionCaseInfo(this.pattern, this.body); + ExpressionCaseInfo({required this.pattern, this.when, required this.body}); } /// Information supplied by the client to [TypeAnalyzer.analyzeSwitchStatement] /// about an individual `case` or `default` clause. /// /// The client is free to `implement` or `extend` this class. -class StatementCaseInfo { +class StatementCaseInfo { /// The AST node for this `case` or `default` clause. This is used for error /// reporting, in case errors arise from mismatch among the variables bound by /// various cases that share a body. final Node node; - /// Indicates whether this `case` or `default` clause is preceded by one or - /// more `goto` labels. - final bool hasLabel; + /// The labels preceding this `case` or `default` clause, if any. + final List labels; /// For a `case` clause, the case pattern. For a `default` clause, `null`. final Node? pattern; + /// For a `case` clause that has a `when` part, the expression following + /// `when`. Otherwise `null`. + final Expression? when; + /// The statements following this `case` or `default` clause. If this list is /// empty, and this is not the last `case` or `default` clause, this clause /// will be considered to share a body with the `case` or `default` clause @@ -45,8 +52,9 @@ class StatementCaseInfo { StatementCaseInfo( {required this.node, - required this.hasLabel, + this.labels = const [], required this.pattern, + this.when, required this.body}); } @@ -70,6 +78,9 @@ class StatementCaseInfo { mixin TypeAnalyzer implements VariableBindingCallbacks { + /// Returns the type `bool`. + Type get boolType; + /// Returns the type `double`. Type get doubleType; @@ -175,6 +186,8 @@ mixin TypeAnalyzer> cases) { + List> cases) { Type expressionType = analyzeExpression(scrutinee, unknownType); flow?.switchStatement_expressionEnd(node); - bool hasLabel = false; - List>? casesInThisExecutionPath; + List labels = []; + List>? + casesInThisExecutionPath; int numExecutionPaths = 0; for (int i = 0; i < cases.length; i++) { - StatementCaseInfo caseInfo = cases[i]; - hasLabel = hasLabel || caseInfo.hasLabel; + StatementCaseInfo caseInfo = cases[i]; + labels.addAll(caseInfo.labels); (casesInThisExecutionPath ??= []).add(caseInfo); if (i == cases.length - 1 || caseInfo.body.isNotEmpty) { numExecutionPaths++; - flow?.switchStatement_beginCase(hasLabel, node); + flow?.switchStatement_beginCase(labels.isNotEmpty, node); VariableBindings bindings = new VariableBindings(this); bindings.startAlternatives(); - for (int i = 0; i < casesInThisExecutionPath.length; i++) { - StatementCaseInfo caseInfo = + // Labels count as empty patterns for the purposes of bindings. + for (Node label in labels) { + bindings.startAlternative(label); + bindings.finishAlternative(); + } + int numCasesInThisExecutionPath = casesInThisExecutionPath.length; + if (numCasesInThisExecutionPath > 1) { + flow?.switchStatement_beginAlternatives(); + } + for (int i = 0; i < numCasesInThisExecutionPath; i++) { + StatementCaseInfo caseInfo = casesInThisExecutionPath[i]; bindings.startAlternative(caseInfo.node); Node? pattern = caseInfo.pattern; if (pattern != null) { dispatchPattern(pattern) .match(expressionType, bindings, isFinal: true, isLate: false); + Expression? when = caseInfo.when; + bool hasWhen = when != null; + if (hasWhen) { + analyzeExpression(when, boolType); + flow?.switchStatement_afterWhen(when); + } + handleCaseHead(hasWhen: hasWhen); } else { handleDefault(); } bindings.finishAlternative(); + if (numCasesInThisExecutionPath > 1) { + flow?.switchStatement_endAlternative(); + } } bindings.finishAlternatives(); - handleCase_afterCaseHeads(casesInThisExecutionPath.length); + if (numCasesInThisExecutionPath > 1) { + flow?.switchStatement_endAlternatives(); + } + handleCase_afterCaseHeads(labels, numCasesInThisExecutionPath); for (Statement statement in caseInfo.body) { dispatchStatement(statement); } finishStatementCase(node, i, caseInfo.body.length); - hasLabel = false; + labels.clear(); casesInThisExecutionPath = null; } } @@ -334,7 +380,10 @@ mixin TypeAnalyzer labels, int numHeads); + + /// See [analyzeSwitchStatement] and [analyzeSwitchExpression]. + void handleCaseHead({required bool hasWhen}); /// See [analyzeConstOrLiteralPattern]. void handleConstOrLiteralPattern(); diff --git a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart index 35d1050aa5a..7a806c425f1 100644 --- a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart +++ b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart @@ -1628,10 +1628,11 @@ main() { test('labeledBlock without break', () { var x = Var('x'); + var l = Label('l'); h.run([ declare(x, type: 'int?', initializer: expr('int?')), if_(x.expr.isNot('int'), [ - labeled((_) => return_()), + l.thenStmt(return_()), ]), checkPromoted(x, 'int'), ]); @@ -1639,15 +1640,16 @@ main() { test('labeledBlock with break joins', () { var x = Var('x'); + var l = Label('l'); h.run([ declare(x, type: 'int?', initializer: expr('int?')), if_(x.expr.isNot('int'), [ - labeled((t) => block([ - if_(expr('bool'), [ - break_(t), - ]), - return_(), - ])), + l.thenStmt(block([ + if_(expr('bool'), [ + break_(l), + ]), + return_(), + ])), ]), checkNotPromoted(x), ]); @@ -1914,8 +1916,8 @@ main() { h.run([ switchExpr(throw_(expr('C')), [ caseExpr(intLiteral(0).pattern, - checkReachable(false).thenExpr(intLiteral(1))), - defaultExpr(checkReachable(false).thenExpr(intLiteral(2))), + body: checkReachable(false).thenExpr(intLiteral(1))), + defaultExpr(body: checkReachable(false).thenExpr(intLiteral(2))), ]).stmt, checkReachable(false), ]); @@ -1924,8 +1926,8 @@ main() { test('switchExpression throw in case body has isolated effect', () { h.run([ switchExpr(expr('int'), [ - caseExpr(intLiteral(0).pattern, throw_(expr('C'))), - defaultExpr(checkReachable(true).thenExpr(intLiteral(2))), + caseExpr(intLiteral(0).pattern, body: throw_(expr('C'))), + defaultExpr(body: checkReachable(true).thenExpr(intLiteral(2))), ]).stmt, checkReachable(true), ]); @@ -1934,8 +1936,8 @@ main() { test('switchExpression throw in all case bodies affects flow after', () { h.run([ switchExpr(expr('int'), [ - caseExpr(intLiteral(0).pattern, throw_(expr('C'))), - defaultExpr(throw_(expr('C'))), + caseExpr(intLiteral(0).pattern, body: throw_(expr('C'))), + defaultExpr(body: throw_(expr('C'))), ]).stmt, checkReachable(false), ]); @@ -1952,7 +1954,7 @@ main() { h.run([ switchExpr(expr('int'), [ caseExpr(x.pattern(type: 'int?'), - checkNotPromoted(x).thenExpr(nullLiteral)), + body: checkNotPromoted(x).thenExpr(nullLiteral)), ]).stmt, ]); }); @@ -1962,10 +1964,10 @@ main() { switch_( throw_(expr('C')), [ - case_(intLiteral(0).pattern, [ + case_(intLiteral(0).pattern, body: [ checkReachable(false), ]), - case_(intLiteral(1).pattern, [ + case_(intLiteral(1).pattern, body: [ checkReachable(false), ]), ], @@ -1985,7 +1987,7 @@ main() { switch_( expr('int'), [ - case_(x.pattern(type: 'int?'), [ + case_(x.pattern(type: 'int?'), body: [ checkNotPromoted(x), ]), ], @@ -1993,6 +1995,31 @@ main() { ]); }); + test('switchStatement_afterWhen() promotes', () { + var x = Var('x'); + h.run([ + switch_( + expr('num'), + [ + case_(x.pattern(), when: x.expr.is_('int'), body: [ + checkPromoted(x, 'int'), + ]), + ], + isExhaustive: true), + ]); + }); + + test('switchStatement_afterWhen() called for switch expressions', () { + var x = Var('x'); + h.run([ + switchExpr(expr('num'), [ + caseExpr(x.pattern(), + when: x.expr.is_('int'), + body: checkPromoted(x, 'int').thenExpr(expr('String'))), + ]).stmt, + ]); + }); + test('switchStatement_beginCase(false) restores previous promotions', () { var x = Var('x'); h.run([ @@ -2001,12 +2028,12 @@ main() { switch_( expr('int'), [ - case_(intLiteral(0).pattern, [ + case_(intLiteral(0).pattern, body: [ checkPromoted(x, 'int'), x.write(expr('int?')).stmt, checkNotPromoted(x), ]), - case_(intLiteral(1).pattern, [ + case_(intLiteral(1).pattern, body: [ checkPromoted(x, 'int'), x.write(expr('int?')).stmt, checkNotPromoted(x), @@ -2024,7 +2051,7 @@ main() { switch_( expr('int'), [ - case_(intLiteral(0).pattern, [ + case_(intLiteral(0).pattern, body: [ checkPromoted(x, 'int'), x.write(expr('int?')).stmt, checkNotPromoted(x), @@ -2043,7 +2070,7 @@ main() { switch_( expr('int'), [ - case_(intLiteral(0).pattern, [ + case_(intLiteral(0).pattern, body: [ checkPromoted(x, 'int'), localFunction([ x.write(expr('int?')).stmt, @@ -2057,6 +2084,7 @@ main() { test('switchStatement_beginCase(true) un-promotes', () { var x = Var('x'); + var l = Label('l'); late SsaNode ssaBeforeSwitch; h.run([ declare(x, type: 'int?', initializer: expr('int?')), @@ -2067,16 +2095,13 @@ main() { getSsaNodes((nodes) => ssaBeforeSwitch = nodes[x]!), ])), [ - case_( - intLiteral(0).pattern, - [ - checkNotPromoted(x), - getSsaNodes( - (nodes) => expect(nodes[x], isNot(ssaBeforeSwitch))), - x.write(expr('int?')).stmt, - checkNotPromoted(x), - ], - hasLabel: true), + l.thenCase(case_(intLiteral(0).pattern, body: [ + checkNotPromoted(x), + getSsaNodes( + (nodes) => expect(nodes[x], isNot(ssaBeforeSwitch))), + x.write(expr('int?')).stmt, + checkNotPromoted(x), + ])), ], isExhaustive: false), ]); @@ -2084,23 +2109,21 @@ main() { test('switchStatement_beginCase(true) handles write captures in cases', () { var x = Var('x'); + var l = Label('l'); h.run([ declare(x, type: 'int?', initializer: expr('int?')), x.expr.as_('int').stmt, switch_( expr('int'), [ - case_( - intLiteral(0).pattern, - [ - x.expr.as_('int').stmt, - checkNotPromoted(x), - localFunction([ - x.write(expr('int?')).stmt, - ]), - checkNotPromoted(x), - ], - hasLabel: true), + l.thenCase(case_(intLiteral(0).pattern, body: [ + x.expr.as_('int').stmt, + checkNotPromoted(x), + localFunction([ + x.write(expr('int?')).stmt, + ]), + checkNotPromoted(x), + ])), ], isExhaustive: false), ]); @@ -2119,7 +2142,7 @@ main() { switch_( expr('int'), [ - case_(intLiteral(0).pattern, [ + case_(intLiteral(0).pattern, body: [ x.expr.as_('int').stmt, y.write(expr('int?')).stmt, break_(), @@ -2148,13 +2171,13 @@ main() { switch_( expr('int'), [ - case_(intLiteral(0).pattern, [ + case_(intLiteral(0).pattern, body: [ w.expr.as_('int').stmt, y.expr.as_('int').stmt, x.write(expr('int?')).stmt, break_(), ]), - default_([ + default_(body: [ w.expr.as_('int').stmt, x.expr.as_('int').stmt, y.write(expr('int?')).stmt, @@ -2176,17 +2199,41 @@ main() { switch_( expr('int'), [ - case_(intLiteral(0).pattern, [ + case_(intLiteral(0).pattern, body: [ x.expr.as_('int').stmt, break_(), ]), - default_([]), + default_(body: []), ], isExhaustive: true), checkNotPromoted(x), ]); }); + test('switchStatement_endAlternative() joins branches', () { + var x = Var('x'); + var y = Var('y'); + var z = Var('z'); + h.run([ + declare(y, type: 'num'), + declare(z, type: 'num'), + switch_( + expr('num'), + [ + case_(x.pattern(), + when: x.expr.is_('int').and(y.expr.is_('int')), body: []), + case_(x.pattern(), + when: y.expr.is_('int').and(z.expr.is_('int')), + body: [ + checkNotPromoted(x), + checkPromoted(y, 'int'), + checkNotPromoted(z), + ]), + ], + isExhaustive: true), + ]); + }); + test('tryCatchStatement_bodyEnd() restores pre-try state', () { var x = Var('x'); var y = Var('y'); diff --git a/pkg/_fe_analyzer_shared/test/mini_ast.dart b/pkg/_fe_analyzer_shared/test/mini_ast.dart index 5fe88dd5318..5cbd24302c7 100644 --- a/pkg/_fe_analyzer_shared/test/mini_ast.dart +++ b/pkg/_fe_analyzer_shared/test/mini_ast.dart @@ -28,14 +28,15 @@ Statement block(List statements) => new _Block(statements); Expression booleanLiteral(bool value) => _BooleanLiteral(value); -Statement break_([LabeledStatement? target]) => new _Break(target); +Statement break_([Label? target]) => new _Break(target); -StatementCase case_(Pattern pattern, List body, - {bool hasLabel = false}) => - StatementCase._(hasLabel, pattern, _Block(body)); +StatementCase case_(Pattern pattern, + {Expression? when, required List body}) => + StatementCase._(pattern, when, _Block(body)); -ExpressionCase caseExpr(Pattern pattern, Expression expression) => - ExpressionCase._(pattern, expression); +ExpressionCase caseExpr(Pattern pattern, + {Expression? when, required Expression body}) => + ExpressionCase._(pattern, when, body); /// Creates a pseudo-statement whose function is to verify that flow analysis /// considers [variable]'s assigned state to be [expectedAssignedState]. @@ -77,11 +78,11 @@ Statement declare(Var variable, isLate: isLate, isFinal: isFinal); -StatementCase default_(List body, {bool hasLabel = false}) => - StatementCase._(hasLabel, null, _Block(body)); +StatementCase default_({required List body}) => + StatementCase._(null, null, _Block(body)); -ExpressionCase defaultExpr(Expression expression) => - ExpressionCase._(null, expression); +ExpressionCase defaultExpr({required Expression body}) => + ExpressionCase._(null, null, body); Statement do_(List body, Expression condition) => _Do(block(body), condition); @@ -146,12 +147,6 @@ Statement if_(Expression condition, List ifTrue, Literal intLiteral(int value, {bool? expectConversionToDouble}) => new _IntLiteral(value, expectConversionToDouble: expectConversionToDouble); -Statement labeled(Statement Function(LabeledStatement) callback) { - var labeledStatement = LabeledStatement._(); - labeledStatement._body = callback(labeledStatement); - return labeledStatement; -} - Statement localFunction(List body) => _LocalFunction(block(body)); Statement match(Pattern pattern, Expression initializer, @@ -279,13 +274,19 @@ class ExpressionCase extends Node @override final Pattern? pattern; + @override + final Expression? when; + @override final Expression body; - ExpressionCase._(this.pattern, this.body) : super._(); + ExpressionCase._(this.pattern, this.when, this.body) : super._(); - String toString() => - [pattern == null ? 'default:' : 'case $pattern:', '$body'].join(' '); + String toString() => [ + pattern == null ? 'default' : 'case $pattern', + if (when != null) ' when $when', + ': $body' + ].join(''); void _preVisit(AssignedVariables assignedVariables) { pattern?.preVisit(assignedVariables); @@ -642,23 +643,29 @@ class Harness } } -class LabeledStatement extends Statement { - late final Statement _body; +class Label extends Node { + final String _name; - LabeledStatement._(); + late final Node _binding; - @override - void preVisit(AssignedVariables assignedVariables) { - _body.preVisit(assignedVariables); + Label(this._name) : super._(); + + StatementCase thenCase(StatementCase case_) { + case_.labels.insert(0, this); + return case_; + } + + Statement thenStmt(Statement statement) { + if (statement is! _LabeledStatement) { + statement = _LabeledStatement(statement); + } + statement._labels.insert(0, this); + _binding = statement; + return statement; } @override - String toString() => 'labeled: $_body'; - - @override - void visit(Harness h) { - h.typeAnalyzer.analyzeLabeledStatement(this, _body); - } + String toString() => _name; } abstract class Literal extends Expression { @@ -765,16 +772,20 @@ abstract class Statement extends Node { /// Representation of a single case clause in a switch statement. Use [case_] /// to create instances of this class. -class StatementCase extends Node implements StatementCaseInfo { +class StatementCase extends Node + implements StatementCaseInfo { @override - final bool hasLabel; + final List