From 4775fa3857c9982b4c62e05718c886f0170c2cbc Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 11 Jan 2022 15:42:04 +0000 Subject: [PATCH] Flow analysis: ensure that not-yet-declared variables aren't marked as captured. In certain error recovery situations, it's possible for code to refer to a variable whose declaration has been lost by error recovery mechanisms. To prevent flow analysis from crashing when this happened, it assumed that any reference to a variable whose declaration had not yet been seen was valid, and implicitly added that variable to the flow analysis state. This created a subtle problem: if a function contained a closure that declared (and assigned to) a local variable, at the time the closure was entered, flow analysis would get confused and temporarily put the variable in the "write captured" state (because it hadn't yet seen the declaration of the variable, so it didn't realize it was local to the closure). Then, a boolean variable might capture that incorrect state. Later, upon seeing the declaration of the variable, it would fix the incorrect state, however it was possible that a later reference to the boolean variable would re-vivify the old incorrect state. This is precisely what happened in issue #47991. This CL fixes the problem by giving flow analysis the ability to detect, at the time the FlowAnalysis object is constructed, all variables that are referred to but not explicitly declared, and add them to the flow analysis state. This allows it to safely assume that any variables that are not yet in the flow analysis state haven't been declared yet (and hence can be ignored), so no variable is every erroneously placed into the "write captured" state. Fixes #47991. Bug: https://github.com/dart-lang/sdk/issues/47991 Change-Id: I8d84fab96fad063f1d3ade3b8b9a6e9af88c3737 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/227361 Reviewed-by: Konstantin Shcheglov Reviewed-by: Johnni Winther Commit-Queue: Paul Berry --- .../lib/src/flow_analysis/flow_analysis.dart | 25 +++++++++++-------- .../assigned_variables/data/constructor.dart | 7 ++++++ .../data/topLevelVariable.dart | 5 +++- .../flow_analysis/flow_analysis_test.dart | 19 ++++++++++++++ .../final/initialize_inside_closure_test.dart | 20 +++++++++++++++ 5 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 tests/language/final/initialize_inside_closure_test.dart 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 54370490442..9a0d773fb46 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 @@ -71,6 +71,7 @@ class AssignedVariables { void declare(Variable variable) { assert(!_isFinished); _stack.last._declared.add(variable); + _anywhere._declared.add(variable); } /// This method may be called during pre-traversal, to mark the end of a @@ -1737,7 +1738,8 @@ class FlowModel { Map>? newVariableInfo; for (Variable variable in writtenVariables) { - VariableModel info = infoFor(variable); + VariableModel? info = variableInfo[variable]; + if (info == null) continue; VariableModel newInfo = info.discardPromotionsAndMarkNotUnassigned(); if (!identical(info, newInfo)) { @@ -1749,16 +1751,8 @@ class FlowModel { for (Variable variable in capturedVariables) { VariableModel? info = variableInfo[variable]; - if (info == null) { - (newVariableInfo ??= - new Map>.of( - variableInfo))[variable] = new VariableModel( - promotedTypes: null, - tested: const [], - assigned: false, - unassigned: false, - ssaNode: null); - } else if (!info.writeCaptured) { + if (info == null) continue; + if (!info.writeCaptured) { (newVariableInfo ??= new Map>.of( variableInfo))[variable] = info.writeCapture(); @@ -3487,6 +3481,15 @@ class _FlowAnalysisImpl anywhere = _assignedVariables._anywhere; + Set implicitlyDeclaredVars = { + ...anywhere._read, + ...anywhere._written + }; + implicitlyDeclaredVars.removeAll(anywhere._declared); + for (Variable variable in implicitlyDeclaredVars) { + declare(variable, true); + } } @override diff --git a/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/constructor.dart b/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/constructor.dart index 20220737ab1..1b7e73194cf 100644 --- a/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/constructor.dart +++ b/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/constructor.dart @@ -12,3 +12,10 @@ class C { class D { const D(bool b) : assert(b); } + +class E { + final String a; + final String? b; + + const E(this.a, {this.b}); +} diff --git a/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/topLevelVariable.dart b/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/topLevelVariable.dart index b2b8e7df5aa..40b86c3b16e 100644 --- a/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/topLevelVariable.dart +++ b/pkg/_fe_analyzer_shared/test/flow_analysis/assigned_variables/data/topLevelVariable.dart @@ -2,6 +2,9 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -dynamic /*member: x:assigned={a}*/ x = /*declared={a, b}*/ (int a, int b) { +dynamic /*member: x1:assigned={a}*/ x1 = /*declared={a, b}*/ (int a, int b) { a = 0; }; + +/*member: x2:none*/ +final x2 = new List.filled(0, null); 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 e85803ae204..a3700cdecea 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 @@ -3197,6 +3197,25 @@ main() { x.expr.as_('int').stmt, checkNotPromoted(x), ]); }); + + test('issue 47991', () { + var h = Harness(); + var b = Var('b', 'bool'); + var i = Var('i', 'int', isFinal: true); + h.run([ + localFunction([ + declareInitialized(b, expr('bool').or(expr('bool'))), + declare(i, initialized: false), + if_(b.expr, [ + checkUnassigned(i, true), + i.write(expr('int')).stmt, + ], [ + checkUnassigned(i, true), + i.write(expr('int')).stmt, + ]), + ]), + ]); + }); }); group('Reachability', () { diff --git a/tests/language/final/initialize_inside_closure_test.dart b/tests/language/final/initialize_inside_closure_test.dart new file mode 100644 index 00000000000..f8fbcc86917 --- /dev/null +++ b/tests/language/final/initialize_inside_closure_test.dart @@ -0,0 +1,20 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Regression test - see https://github.com/dart-lang/sdk/issues/47991 + +import 'dart:math'; + +void main() { + () { + final should = Random().nextBool() || Random().nextBool(); + final int a; + + if (should) { + a = 1; + } else { + a = 2; + } + }; +}