From dab4eb045fa72e05a3a36582851da5a84c1947e4 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Tue, 11 Jun 2019 23:17:43 +0000 Subject: [PATCH] Add a "legacy type asserter," manage broken test cases Change-Id: Ic046805c3246ad7e5e7c56fb9188d6ebd197e2fa Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105560 Commit-Queue: Mike Fairhurst Reviewed-by: Brian Wilkerson --- .../src/dart/analysis/library_analyzer.dart | 4 + .../dart/resolver/legacy_type_asserter.dart | 170 ++++++++++++++++ .../generated/testing/ast_test_factory.dart | 17 ++ .../resolver/legacy_type_asserter_test.dart | 184 ++++++++++++++++++ .../test/src/dart/resolver/test_all.dart | 2 + .../diagnostics/non_null_opt_out_test.dart | 15 +- 6 files changed, 390 insertions(+), 2 deletions(-) create mode 100644 pkg/analyzer/lib/src/dart/resolver/legacy_type_asserter.dart create mode 100644 pkg/analyzer/test/src/dart/resolver/legacy_type_asserter_test.dart diff --git a/pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart b/pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart index f3b0f77472b..83c51289ea9 100644 --- a/pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart +++ b/pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart @@ -20,6 +20,7 @@ import 'package:analyzer/src/dart/constant/utilities.dart'; import 'package:analyzer/src/dart/element/element.dart'; import 'package:analyzer/src/dart/element/handle.dart'; import 'package:analyzer/src/dart/element/inheritance_manager2.dart'; +import 'package:analyzer/src/dart/resolver/legacy_type_asserter.dart'; import 'package:analyzer/src/error/codes.dart'; import 'package:analyzer/src/error/inheritance_override.dart'; import 'package:analyzer/src/error/pending_error.dart'; @@ -201,6 +202,9 @@ class LibraryAnalyzer { } }); } + + assert(units.values.every(LegacyTypeAsserter.assertLegacyTypes)); + timerLibraryAnalyzerVerify.stop(); // Return full results. diff --git a/pkg/analyzer/lib/src/dart/resolver/legacy_type_asserter.dart b/pkg/analyzer/lib/src/dart/resolver/legacy_type_asserter.dart new file mode 100644 index 00000000000..43377ba518c --- /dev/null +++ b/pkg/analyzer/lib/src/dart/resolver/legacy_type_asserter.dart @@ -0,0 +1,170 @@ +// Copyright (c) 2019, 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. + +import 'dart:collection'; + +import 'package:analyzer/dart/analysis/features.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:analyzer/src/dart/element/type.dart'; + +/// A visitor to assert that legacy libraries deal with legacy types. +/// +/// Intended to be used via the static method +/// [LegacyTypeAsserter.assertLegacyTypes], inside an `assert()` node. +/// +/// Has a defense against being accidentally run outside of an assert statement, +/// but that can be overridden if needed. +/// +/// Checks that the static type of every node, as well as the elements of many +/// nodes, have legacy types, and asserts that the legacy types are deep legacy +/// types. +class LegacyTypeAsserter extends GeneralizingAstVisitor { + // TODO(mfairhurst): remove custom equality/hashCode once both use nullability + Set visitedTypes = LinkedHashSet( + equals: (a, b) => + a == b && + (a as TypeImpl).nullabilitySuffix == + (b as TypeImpl).nullabilitySuffix, + hashCode: (a) => + a.hashCode * 11 + (a as TypeImpl).nullabilitySuffix.hashCode); + + LegacyTypeAsserter({bool requireIsDebug = true}) { + if (requireIsDebug) { + bool isDebug = false; + + assert(() { + isDebug = true; + return true; + }()); + + if (!isDebug) { + throw UnsupportedError( + 'Legacy type asserter is being run outside of a debug environment'); + } + } + } + + @override + visitClassDeclaration(ClassDeclaration node) { + _assertLegacyType(node.element?.type); + super.visitClassDeclaration(node); + } + + @override + visitClassMember(ClassMember node) { + final element = node.element; + if (element is ExecutableElement) { + _assertLegacyType(element?.type); + } + super.visitClassMember(node); + } + + @override + visitCompilationUnit(CompilationUnit node) { + if (!node.featureSet.isEnabled(Feature.non_nullable)) { + super.visitCompilationUnit(node); + } + } + + @override + visitDeclaredIdentifier(DeclaredIdentifier node) { + _assertLegacyType(node.element?.type); + super.visitDeclaredIdentifier(node); + } + + @override + visitEnumDeclaration(EnumDeclaration node) { + _assertLegacyType(node.element?.type); + super.visitEnumDeclaration(node); + } + + @override + visitExpression(Expression e) { + _assertLegacyType(e.staticType); + super.visitExpression(e); + } + + @override + visitFormalParameter(FormalParameter node) { + _assertLegacyType(node.element?.type); + super.visitFormalParameter(node); + } + + @override + visitFunctionDeclaration(FunctionDeclaration node) { + _assertLegacyType(node.element?.type); + super.visitFunctionDeclaration(node); + } + + @override + visitTypeAnnotation(TypeAnnotation node) { + _assertLegacyType(node.type); + super.visitTypeAnnotation(node); + } + + @override + visitTypeName(TypeName node) { + _assertLegacyType(node.type); + super.visitTypeName(node); + } + + @override + visitVariableDeclaration(VariableDeclaration node) { + _assertLegacyType(node.element?.type); + super.visitVariableDeclaration(node); + } + + void _assertLegacyType(DartType type) { + if (type == null) { + return; + } + + if (type.isDynamic || type.isVoid) { + return; + } + + if (type.isBottom && type.isDartCoreNull) { + // Never?, which is ok. + // + // Note: we could allow Null? and Null, but we really should be able to + // guarantee that we are only working with Null*, so that's what this + // currently does. + return; + } + + if (visitedTypes.contains(type)) { + return; + } + + visitedTypes.add(type); + + if (type is TypeParameterType) { + _assertLegacyType(type.bound); + } else if (type is InterfaceType) { + type.typeArguments.forEach(_assertLegacyType); + type.typeParameters.map((param) => param.type).forEach(_assertLegacyType); + } else if (type is FunctionType) { + _assertLegacyType(type.returnType); + type.parameters.map((param) => param.type).forEach(_assertLegacyType); + type.typeArguments.forEach(_assertLegacyType); + type.typeParameters.map((param) => param.type).forEach(_assertLegacyType); + } + + if ((type as TypeImpl).nullabilitySuffix == NullabilitySuffix.star) { + return; + } + + throw AssertionError('Expected all legacy types, but got ' + '${(type as TypeImpl).toString(withNullability: true)} ' + '(${type.runtimeType})'); + } + + static bool assertLegacyTypes(CompilationUnit compilationUnit) { + new LegacyTypeAsserter().visitCompilationUnit(compilationUnit); + return true; + } +} diff --git a/pkg/analyzer/lib/src/generated/testing/ast_test_factory.dart b/pkg/analyzer/lib/src/generated/testing/ast_test_factory.dart index 469e29c4463..923733910be 100644 --- a/pkg/analyzer/lib/src/generated/testing/ast_test_factory.dart +++ b/pkg/analyzer/lib/src/generated/testing/ast_test_factory.dart @@ -2,6 +2,7 @@ // 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. +import 'package:analyzer/dart/analysis/features.dart'; import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/standard_ast_factory.dart'; import 'package:analyzer/dart/ast/token.dart'; @@ -287,6 +288,22 @@ class AstTestFactory { endToken: TokenFactory.tokenFromType(TokenType.EOF), featureSet: null); + static CompilationUnit compilationUnit9( + {String scriptTag, + List directives, + List declarations, + FeatureSet featureSet}) => + astFactory.compilationUnit2( + beginToken: TokenFactory.tokenFromType(TokenType.EOF), + scriptTag: + scriptTag == null ? null : AstTestFactory.scriptTag(scriptTag), + directives: directives == null ? new List() : directives, + declarations: declarations == null + ? new List() + : declarations, + endToken: TokenFactory.tokenFromType(TokenType.EOF), + featureSet: featureSet); + static ConditionalExpression conditionalExpression(Expression condition, Expression thenExpression, Expression elseExpression) => astFactory.conditionalExpression( diff --git a/pkg/analyzer/test/src/dart/resolver/legacy_type_asserter_test.dart b/pkg/analyzer/test/src/dart/resolver/legacy_type_asserter_test.dart new file mode 100644 index 00000000000..b1f5a8e19d0 --- /dev/null +++ b/pkg/analyzer/test/src/dart/resolver/legacy_type_asserter_test.dart @@ -0,0 +1,184 @@ +// Copyright (c) 2019, 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. + +import 'package:analyzer/dart/analysis/features.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer/src/dart/ast/ast.dart'; +import 'package:analyzer/src/dart/element/element.dart'; +import 'package:analyzer/src/dart/element/type.dart'; +import 'package:analyzer/src/dart/resolver/legacy_type_asserter.dart'; +import 'package:analyzer/src/generated/resolver.dart'; +import 'package:analyzer/src/generated/testing/ast_test_factory.dart'; +import 'package:test/test.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +import '../ast/parse_base.dart'; +import '../resolution/driver_resolution.dart'; + +main() { + defineReflectiveSuite(() { + defineReflectiveTests(LegacyTypeAsserterTest); + }); +} + +/// Tests for the [ExitDetector] that require that the control flow and spread +/// experiments be enabled. +@reflectiveTest +class LegacyTypeAsserterTest extends DriverResolutionTest { + TypeProvider typeProvider; + setUp() async { + await super.setUp(); + typeProvider = await this.driver.currentSession.typeProvider; + } + + test_nullableUnit_expressionStaticType_bottom() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = BottomTypeImpl.instance; + try { + LegacyTypeAsserter.assertLegacyTypes(unit); + fail('expected an exception'); + } on AssertionError { + // expected + } + } + + test_nullableUnit_expressionStaticType_bottomQuestion() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = BottomTypeImpl.instanceNullable; + LegacyTypeAsserter.assertLegacyTypes(unit); + } + + test_nullableUnit_expressionStaticType_dynamic() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = typeProvider.dynamicType; + LegacyTypeAsserter.assertLegacyTypes(unit); + } + + test_nullableUnit_expressionStaticType_nonNull() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = (typeProvider.intType as TypeImpl) + .withNullability(NullabilitySuffix.none); + try { + LegacyTypeAsserter.assertLegacyTypes(unit); + fail('expected an exception'); + } on AssertionError { + // expected + } + } + + test_nullableUnit_expressionStaticType_nonNullTypeArgument() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = typeProvider.listType.instantiate([ + (typeProvider.intType as TypeImpl) + .withNullability(NullabilitySuffix.question) + ]); + + try { + LegacyTypeAsserter.assertLegacyTypes(unit); + fail('expected an exception'); + } on AssertionError { + // expected + } + } + + test_nullableUnit_expressionStaticType_nonNullTypeParameter() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + final listType = typeProvider.listType; + listType.typeParameters[0] = TypeParameterElementImpl('E', 0) + ..type = (listType.typeParameters[0].type as TypeImpl) + .withNullability(NullabilitySuffix.none) as TypeParameterTypeImpl; + identifier.staticType = listType; + expect( + (listType as dynamic) + .typeParameters[0] + .type + .toString(withNullability: true), + 'E'); + try { + LegacyTypeAsserter.assertLegacyTypes(unit); + fail('expected an exception'); + } on AssertionError { + // expected + } + } + + test_nullableUnit_expressionStaticType_nonNullTypeParameterBound() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + final listType = typeProvider.listType; + (listType.typeParameters[0] as TypeParameterElementImpl).bound = + (typeProvider.intType as TypeImpl) + .withNullability(NullabilitySuffix.none); + identifier.staticType = listType; + expect( + (listType as dynamic) + .typeParameters[0] + .type + .bound + .toString(withNullability: true), + 'int'); + try { + LegacyTypeAsserter.assertLegacyTypes(unit); + fail('expected an exception'); + } on AssertionError { + // expected + } + } + + test_nullableUnit_expressionStaticType_null() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = typeProvider.nullType; + LegacyTypeAsserter.assertLegacyTypes(unit); + } + + test_nullableUnit_expressionStaticType_question() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = (typeProvider.intType as TypeImpl) + .withNullability(NullabilitySuffix.question); + try { + LegacyTypeAsserter.assertLegacyTypes(unit); + fail('expected an exception'); + } on AssertionError { + // expected + } + } + + test_nullableUnit_expressionStaticType_star() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = (typeProvider.intType as TypeImpl) + .withNullability(NullabilitySuffix.star); + LegacyTypeAsserter.assertLegacyTypes(unit); + } + + test_nullableUnit_expressionStaticType_void() async { + var identifier = AstTestFactory.identifier3('foo'); + var unit = _wrapExpression(identifier); + identifier.staticType = VoidTypeImpl.instance; + LegacyTypeAsserter.assertLegacyTypes(unit); + } + + CompilationUnit _wrapExpression(Expression e, {bool nonNullable = false}) { + return AstTestFactory.compilationUnit9( + declarations: [ + AstTestFactory.functionDeclaration( + null, + null, + null, + AstTestFactory.functionExpression2( + null, AstTestFactory.expressionFunctionBody(e))) + ], + featureSet: FeatureSet.forTesting( + additionalFeatures: nonNullable ? [Feature.non_nullable] : [])); + } +} diff --git a/pkg/analyzer/test/src/dart/resolver/test_all.dart b/pkg/analyzer/test/src/dart/resolver/test_all.dart index e6430bd4d01..f100f90e9ed 100644 --- a/pkg/analyzer/test/src/dart/resolver/test_all.dart +++ b/pkg/analyzer/test/src/dart/resolver/test_all.dart @@ -5,9 +5,11 @@ import 'package:test_reflective_loader/test_reflective_loader.dart'; import 'exit_detector_test.dart' as exit_detector; +import 'legacy_type_asserter_test.dart' as legacy_type_asserter; main() { defineReflectiveSuite(() { exit_detector.main(); + legacy_type_asserter.main(); }, name: 'resolver'); } diff --git a/pkg/analyzer/test/src/diagnostics/non_null_opt_out_test.dart b/pkg/analyzer/test/src/diagnostics/non_null_opt_out_test.dart index 8c5936c5dbe..933fa1bad30 100644 --- a/pkg/analyzer/test/src/diagnostics/non_null_opt_out_test.dart +++ b/pkg/analyzer/test/src/diagnostics/non_null_opt_out_test.dart @@ -27,8 +27,8 @@ class NonNullOptOutTest extends DriverResolutionTest { assertErrorsInCode(''' // @dart = 2.2 // NNBD syntax is not allowed -f(x, y, z) { (x is String?) ? (x + y) : z; } -''', [error(ParserErrorCode.EXPERIMENT_NOT_ENABLED, 70, 1)]); +f(x, z) { (x is String?) ? x : z; } +''', [error(ParserErrorCode.EXPERIMENT_NOT_ENABLED, 67, 1)]); } test_nnbd_optOut_late() async { @@ -38,6 +38,17 @@ class C { // "late" is allowed as an identifier int late; } +'''); + } + + @failingTest + test_nnbd_optOut_transformsOptedInSignatures() async { + // Failing because we don't transform opted out signatures. + await assertNoErrorsInCode(''' +// @dart = 2.2 +f(String x) { + x + null; // OK because we're in a nullable library. +} '''); } }