diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f02905b704..1ab41b89b09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,30 @@ This brings the implementation behavior in line with the spec. +- Initializer expressions on implicitly typed condition variables can now + contribute to type promotion. For example, this program no longer produces a + compile-time error: + + ```dart + f(int? i) { + var iIsNull = i == null; + if (!iIsNull) { + print(i + 1); // OK, because `i` is known to be non-null + } + } + ``` + + Previously, the above program had a compile-time error because type promotion + due to a bug ([#1785][]) which prevented the initializer expression (`i == + null`) from being accounted for when the variable in question (`iIsNull`) + lacked an explicit type. + + To avoid causing problems for packages that are intended to work with older + versions of Dart, the fix only takes effect when the "constructor tearoffs" + language feature is enabled. + +[#1785]: https://github.com/dart-lang/language/issues/1785 + ### Tools #### Dart command line 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 3475d6d717a..55b94a9b0be 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 @@ -368,8 +368,11 @@ class ExpressionInfo { abstract class FlowAnalysis { factory FlowAnalysis(TypeOperations typeOperations, - AssignedVariables assignedVariables) { - return new _FlowAnalysisImpl(typeOperations, assignedVariables); + AssignedVariables assignedVariables, + {required bool respectImplicitlyTypedVarInitializers}) { + return new _FlowAnalysisImpl(typeOperations, assignedVariables, + respectImplicitlyTypedVarInitializers: + respectImplicitlyTypedVarInitializers); } factory FlowAnalysis.legacy(TypeOperations typeOperations, @@ -983,10 +986,13 @@ class FlowAnalysisDebug typeOperations, - AssignedVariables assignedVariables) { + AssignedVariables assignedVariables, + {required bool respectImplicitlyTypedVarInitializers}) { print('FlowAnalysisDebug()'); - return new FlowAnalysisDebug._( - new _FlowAnalysisImpl(typeOperations, assignedVariables)); + return new FlowAnalysisDebug._(new _FlowAnalysisImpl( + typeOperations, assignedVariables, + respectImplicitlyTypedVarInitializers: + respectImplicitlyTypedVarInitializers)); } factory FlowAnalysisDebug.legacy( @@ -3454,7 +3460,15 @@ class _FlowAnalysisImpl _assignedVariables; - _FlowAnalysisImpl(this.typeOperations, this._assignedVariables); + /// Indicates whether initializers of implicitly typed variables should be + /// accounted for by SSA analysis. (In an ideal world, they always would be, + /// but due to https://github.com/dart-lang/language/issues/1785, they weren't + /// always, and we need to be able to replicate the old behavior when + /// analyzing old language versions). + final bool respectImplicitlyTypedVarInitializers; + + _FlowAnalysisImpl(this.typeOperations, this._assignedVariables, + {required this.respectImplicitlyTypedVarInitializers}); @override bool get isReachable => _current.reachable.overallReachable; @@ -3810,10 +3824,11 @@ class _FlowAnalysisImpl( - h, AssignedVariables()); + h, AssignedVariables(), + respectImplicitlyTypedVarInitializers: true); flow.ifStatement_conditionBegin(); flow.ifStatement_thenBegin(e, s); expect(() => flow.finish(), _asserts); @@ -1407,9 +1408,10 @@ main() { ]); }); - test('initialize() does not store expressionInfo for implicitly typed vars', - () { - var h = Harness(); + test( + 'initialize() does not store expressionInfo for implicitly typed ' + 'vars, pre-bug fix', () { + var h = Harness(respectImplicitlyTypedVarInitializers: false); var x = Var('x', 'Object', isImplicitlyTyped: true); var y = Var('y', 'int?'); h.run([ @@ -1420,6 +1422,34 @@ main() { ]); }); + test( + 'initialize() stores expressionInfo for implicitly typed ' + 'vars, post-bug fix', () { + var h = Harness(respectImplicitlyTypedVarInitializers: true); + var x = Var('x', 'Object', isImplicitlyTyped: true); + var y = Var('y', 'int?'); + h.run([ + declareInitialized(x, y.expr.eq(nullLiteral)), + getSsaNodes((nodes) { + expect(nodes[x]!.expressionInfo, isNotNull); + }), + ]); + }); + + test( + 'initialize() stores expressionInfo for explicitly typed ' + 'vars, pre-bug fix', () { + var h = Harness(respectImplicitlyTypedVarInitializers: false); + var x = Var('x', 'Object'); + var y = Var('y', 'int?'); + h.run([ + declareInitialized(x, y.expr.eq(nullLiteral)), + getSsaNodes((nodes) { + expect(nodes[x]!.expressionInfo, isNotNull); + }), + ]); + }); + test('initialize() does not store expressionInfo for trivial expressions', () { var h = Harness(); diff --git a/pkg/_fe_analyzer_shared/test/flow_analysis/nullability/data/local_boolean.dart b/pkg/_fe_analyzer_shared/test/flow_analysis/nullability/data/local_boolean.dart index 7fd0b638674..32dd3c655d3 100644 --- a/pkg/_fe_analyzer_shared/test/flow_analysis/nullability/data/local_boolean.dart +++ b/pkg/_fe_analyzer_shared/test/flow_analysis/nullability/data/local_boolean.dart @@ -2,6 +2,8 @@ // 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. +// @dart=2.13 + finalLocalBool(int? x) { final bool b = x == null; if (!b) { diff --git a/pkg/_fe_analyzer_shared/test/mini_ast.dart b/pkg/_fe_analyzer_shared/test/mini_ast.dart index 10ed21b28ef..57ce172df11 100644 --- a/pkg/_fe_analyzer_shared/test/mini_ast.dart +++ b/pkg/_fe_analyzer_shared/test/mini_ast.dart @@ -388,7 +388,17 @@ class Harness extends TypeOperations { late final _typeAnalyzer = _MiniAstTypeAnalyzer(this); - Harness({this.legacy = false, String? thisType}) + /// Indicates whether initializers of implicitly typed variables should be + /// accounted for by SSA analysis. (In an ideal world, they always would be, + /// but due to https://github.com/dart-lang/language/issues/1785, they weren't + /// always, and we need to be able to replicate the old behavior when + /// analyzing old language versions). + final bool respectImplicitlyTypedVarInitializers; + + Harness( + {this.legacy = false, + String? thisType, + this.respectImplicitlyTypedVarInitializers = true}) : thisType = thisType == null ? null : Type(thisType); MiniIrBuilder get _irBuilder => _typeAnalyzer._irBuilder; @@ -483,7 +493,9 @@ class Harness extends TypeOperations { ? FlowAnalysis.legacy( this, assignedVariables) : FlowAnalysis( - this, assignedVariables); + this, assignedVariables, + respectImplicitlyTypedVarInitializers: + respectImplicitlyTypedVarInitializers); _typeAnalyzer.dispatchStatement(b); _typeAnalyzer.finish(); } diff --git a/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart b/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart index 61c0603e32e..5b6a4fbfb73 100644 --- a/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart +++ b/pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart @@ -13,6 +13,7 @@ import 'package:analyzer/src/dart/ast/extensions.dart'; import 'package:analyzer/src/dart/element/type.dart'; import 'package:analyzer/src/dart/element/type_system.dart' show TypeSystemImpl; import 'package:analyzer/src/generated/migration.dart'; +import 'package:analyzer/src/generated/resolver.dart'; import 'package:analyzer/src/generated/variable_type_provider.dart'; /// Data gathered by flow analysis, retained for testing purposes. @@ -222,7 +223,8 @@ class FlowAnalysisHelper { flow!.labeledStatement_end(); } - void topLevelDeclaration_enter(AstNode node, FormalParameterList? parameters, + void topLevelDeclaration_enter( + ResolverVisitor resolver, AstNode node, FormalParameterList? parameters, {void Function(AstVisitor visitor)? visit}) { assert(flow == null); assignedVariables = computeAssignedVariables(node, parameters, @@ -233,7 +235,9 @@ class FlowAnalysisHelper { } flow = isNonNullableByDefault ? FlowAnalysis(_typeOperations, assignedVariables!) + DartType>(_typeOperations, assignedVariables!, + respectImplicitlyTypedVarInitializers: + resolver.isConstructorTearoffsEnabled) : FlowAnalysis.legacy(_typeOperations, assignedVariables!); } @@ -349,9 +353,10 @@ class FlowAnalysisHelperForMigration extends FlowAnalysisHelper { : super(typeSystem, false, isNonNullableByDefault); @override - void topLevelDeclaration_enter(AstNode node, FormalParameterList? parameters, + void topLevelDeclaration_enter( + ResolverVisitor resolver, AstNode node, FormalParameterList? parameters, {void Function(AstVisitor visitor)? visit}) { - super.topLevelDeclaration_enter(node, parameters, visit: visit); + super.topLevelDeclaration_enter(resolver, node, parameters, visit: visit); migrationResolutionHooks.setFlowAnalysis(flow); } diff --git a/pkg/analyzer/lib/src/dart/resolver/variable_declaration_resolver.dart b/pkg/analyzer/lib/src/dart/resolver/variable_declaration_resolver.dart index 1e417598bab..36542692701 100644 --- a/pkg/analyzer/lib/src/dart/resolver/variable_declaration_resolver.dart +++ b/pkg/analyzer/lib/src/dart/resolver/variable_declaration_resolver.dart @@ -45,7 +45,7 @@ class VariableDeclarationResolver { InferenceContext.setTypeFromNode(initializer, node); if (isTopLevel) { - _resolver.flowAnalysis.topLevelDeclaration_enter(node, null); + _resolver.flowAnalysis.topLevelDeclaration_enter(_resolver, node, null); } else if (element.isLate) { _resolver.flowAnalysis.flow?.lateInitializer_begin(node); } diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart index 81790d559c4..4fc7f040d79 100644 --- a/pkg/analyzer/lib/src/generated/resolver.dart +++ b/pkg/analyzer/lib/src/generated/resolver.dart @@ -1185,7 +1185,7 @@ class ResolverVisitor extends ResolverBase with ErrorDetectionHelpers { @override void visitConstructorDeclaration(ConstructorDeclaration node) { - flowAnalysis.topLevelDeclaration_enter(node, node.parameters); + flowAnalysis.topLevelDeclaration_enter(this, node, node.parameters); flowAnalysis.executableDeclaration_enter(node, node.parameters, false); var returnType = node.declaredElement!.type.returnType; @@ -1409,7 +1409,7 @@ class ResolverVisitor extends ResolverBase with ErrorDetectionHelpers { flowAnalysis.flow!.functionExpression_begin(node); } else { flowAnalysis.topLevelDeclaration_enter( - node, node.functionExpression.parameters); + this, node, node.functionExpression.parameters); } flowAnalysis.executableDeclaration_enter( node, @@ -1638,7 +1638,7 @@ class ResolverVisitor extends ResolverBase with ErrorDetectionHelpers { @override void visitMethodDeclaration(MethodDeclaration node) { - flowAnalysis.topLevelDeclaration_enter(node, node.parameters); + flowAnalysis.topLevelDeclaration_enter(this, node, node.parameters); flowAnalysis.executableDeclaration_enter(node, node.parameters, false); DartType returnType = node.declaredElement!.returnType; diff --git a/pkg/analyzer/lib/src/summary2/ast_resolver.dart b/pkg/analyzer/lib/src/summary2/ast_resolver.dart index 96f8760659e..04523e41ee1 100644 --- a/pkg/analyzer/lib/src/summary2/ast_resolver.dart +++ b/pkg/analyzer/lib/src/summary2/ast_resolver.dart @@ -58,7 +58,7 @@ class AstResolver { node.accept(_resolutionVisitor); node.accept(_scopeResolverVisitor); _prepareEnclosingDeclarations(); - _flowAnalysis.topLevelDeclaration_enter(node, null); + _flowAnalysis.topLevelDeclaration_enter(_resolverVisitor, node, null); node.accept(_resolverVisitor); _flowAnalysis.topLevelDeclaration_exit(); } @@ -76,7 +76,8 @@ class AstResolver { visit(_scopeResolverVisitor); _prepareEnclosingDeclarations(); - _flowAnalysis.topLevelDeclaration_enter(node, node.parameters, + _flowAnalysis.topLevelDeclaration_enter( + _resolverVisitor, node, node.parameters, visit: visit); visit(_resolverVisitor); _flowAnalysis.topLevelDeclaration_exit(); @@ -95,7 +96,8 @@ class AstResolver { node.accept(_scopeResolverVisitor); } _prepareEnclosingDeclarations(); - _flowAnalysis.topLevelDeclaration_enter(node.parent!, null); + _flowAnalysis.topLevelDeclaration_enter( + _resolverVisitor, node.parent!, null); node.accept(_resolverVisitor); _flowAnalysis.topLevelDeclaration_exit(); } diff --git a/pkg/analyzer/test/src/diagnostics/non_constant_map_element_test.dart b/pkg/analyzer/test/src/diagnostics/non_constant_map_element_test.dart index f60e50d95ab..35beee62c6f 100644 --- a/pkg/analyzer/test/src/diagnostics/non_constant_map_element_test.dart +++ b/pkg/analyzer/test/src/diagnostics/non_constant_map_element_test.dart @@ -79,12 +79,14 @@ void main() { } test_ifElementWithElse_mayBeConst() async { - await assertNoErrorsInCode(''' + await assertErrorsInCode(''' void main() { const isTrue = true; const {1: null, if (isTrue) null: null else null: null}; } -'''); +''', [ + error(HintCode.DEAD_CODE, 83, 12), + ]); } test_spreadElement_mayBeConst() async { diff --git a/pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart b/pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart index ff7e4aae20a..8ff93153f53 100644 --- a/pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart +++ b/pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart @@ -224,7 +224,9 @@ class TypeInferrerImpl implements TypeInferrer { flowAnalysis = library.isNonNullableByDefault ? new FlowAnalysis( new TypeOperationsCfe(engine.typeSchemaEnvironment), - assignedVariables) + assignedVariables, + respectImplicitlyTypedVarInitializers: + library.enableConstructorTearOffsInLibrary) : new FlowAnalysis.legacy( new TypeOperationsCfe(engine.typeSchemaEnvironment), assignedVariables) {} diff --git a/pkg/front_end/test/spell_checking_list_code.txt b/pkg/front_end/test/spell_checking_list_code.txt index 65b0ba7c7c4..69aec91a280 100644 --- a/pkg/front_end/test/spell_checking_list_code.txt +++ b/pkg/front_end/test/spell_checking_list_code.txt @@ -142,6 +142,7 @@ bs bsd bslash buffered +buggy builder`s bulk bump @@ -559,6 +560,7 @@ i'll i2b ic id +ideal identifies identifying ideographic diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart index d155843877d..62cd9a35a01 100644 --- a/pkg/nnbd_migration/lib/src/edge_builder.dart +++ b/pkg/nnbd_migration/lib/src/edge_builder.dart @@ -377,10 +377,14 @@ class EdgeBuilder extends GeneralizingAstVisitor var previousAssignedVariables = _assignedVariables; if (_flowAnalysis == null) { _assignedVariables = AssignedVariables(); + // Note: we are using flow analysis to help us track true nullabilities; + // it's not necessary to replicate old bugs. So we pass `true` for + // `respectImplicitlyTypedVarInitializers`. _flowAnalysis = FlowAnalysis( DecoratedTypeOperations(_typeSystem, _variables, _graph), - _assignedVariables!); + _assignedVariables!, + respectImplicitlyTypedVarInitializers: true); } try { _dispatch(node.name); @@ -2110,10 +2114,14 @@ class EdgeBuilder extends GeneralizingAstVisitor assert(_assignedVariables == null); _assignedVariables = FlowAnalysisHelper.computeAssignedVariables(node, parameters); + // Note: we are using flow analysis to help us track true nullabilities; + // it's not necessary to replicate old bugs. So we pass `true` for + // `respectImplicitlyTypedVarInitializers`. _flowAnalysis = FlowAnalysis( DecoratedTypeOperations(_typeSystem, _variables, _graph), - _assignedVariables!); + _assignedVariables!, + respectImplicitlyTypedVarInitializers: true); if (parameters != null) { for (var parameter in parameters.parameters) { _flowAnalysis!.declare(parameter.declaredElement!, true); diff --git a/pkg/vm/lib/transformations/type_flow/transformer.dart b/pkg/vm/lib/transformations/type_flow/transformer.dart index cf136cc11cf..5634a76d309 100644 --- a/pkg/vm/lib/transformations/type_flow/transformer.dart +++ b/pkg/vm/lib/transformations/type_flow/transformer.dart @@ -315,9 +315,15 @@ class AnnotateKernel extends RecursiveVisitor { Constant? constantValue; bool isInt = false; - final nullable = type is NullableType; + // Note: the explicit type `bool` is needed because the checked-in version + // of the CFE that we use for bootstrapping doesn't yet have constructor + // tearoffs enabled, and the fix for bug + // https://github.com/dart-lang/language/issues/1785 only takes effect when + // constructor tearoffs are enabled. TODO(paulberry): remove the type after + // the bootstrap CFE enables constructor tearoffs. + final bool nullable = type is NullableType; if (nullable) { - type = (type as NullableType).baseType; + type = type.baseType; } if (nullable && type == const EmptyType()) { diff --git a/tests/language/nnbd/flow_analysis/local_boolean_implicitly_typed_new_test.dart b/tests/language/nnbd/flow_analysis/local_boolean_implicitly_typed_new_test.dart new file mode 100644 index 00000000000..5fd472137ad --- /dev/null +++ b/tests/language/nnbd/flow_analysis/local_boolean_implicitly_typed_new_test.dart @@ -0,0 +1,202 @@ +// Copyright (c) 2021, 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. + +// SharedOptions=--enable-experiment=constructor-tearoffs + +import '../../static_type_helper.dart'; + +// This test checks whether a local boolean variable can be used to perform type +// promotion, if that variable is implicitly typed. +// +// This test confirms that once the "constructor tearoffs" language feature is +// enabled, initializer expressions on implicitly typed variables are no longer +// ignored for the purposes of type promotion +// (i.e. https://github.com/dart-lang/language/issues/1785 is fixed). + +parameterUnmodified(int? x) { + { + late final b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + late var b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late var b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + final b = x != null; + if (b) x.expectStaticType>(); + } + { + final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + var b = x != null; + if (b) x.expectStaticType>(); + } + { + var b; + b = x != null; + if (b) x.expectStaticType>(); + } +} + +parameterModifiedLater(int? x, int? y) { + x = y; + { + late final b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + late var b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late var b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + final b = x != null; + if (b) x.expectStaticType>(); + } + { + final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + var b = x != null; + if (b) x.expectStaticType>(); + } + { + var b; + b = x != null; + if (b) x.expectStaticType>(); + } +} + +localVariableInitialized(int? y) { + int? x = y; + { + late final b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + late var b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late var b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + final b = x != null; + if (b) x.expectStaticType>(); + } + { + final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + var b = x != null; + if (b) x.expectStaticType>(); + } + { + var b; + b = x != null; + if (b) x.expectStaticType>(); + } +} + +localVariableModifiedLater(int? y) { + int? x; + x = y; + { + late final b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + late var b = x != null; + // We don't promote based on the initializers of late locals anyhow, because + // we don't know when they execute. + if (b) x.expectStaticType>(); + } + { + late var b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + final b = x != null; + if (b) x.expectStaticType>(); + } + { + final b; + b = x != null; + if (b) x.expectStaticType>(); + } + { + var b = x != null; + if (b) x.expectStaticType>(); + } + { + var b; + b = x != null; + if (b) x.expectStaticType>(); + } +} + +main() { + parameterUnmodified(null); + parameterUnmodified(0); + parameterModifiedLater(null, null); + parameterModifiedLater(null, 0); + localVariableInitialized(null); + localVariableInitialized(0); + localVariableModifiedLater(null); + localVariableModifiedLater(0); +}