Skip to content

Commit

Permalink
Refactor strong mode to use standard Analyzer errors
Browse files Browse the repository at this point in the history
Also changes implicit casts errors to be determined by the type system isAssignableTo code, and updates our tests to show the user-facing error levels (StaticTypeWarningCode is upgraded to errors as far as users can see, but this wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583

R=brianwilkerson@google.com, leafp@google.com

Review URL: https://codereview.chromium.org/2060013002 .
  • Loading branch information
John Messerly committed Jun 13, 2016
1 parent 678cb04 commit 831c875
Show file tree
Hide file tree
Showing 13 changed files with 666 additions and 820 deletions.
129 changes: 129 additions & 0 deletions pkg/analyzer/lib/src/generated/error.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2851,6 +2851,24 @@ abstract class ErrorCode {
StaticWarningCode.UNDEFINED_SUPER_SETTER,
StaticWarningCode.VOID_RETURN_FOR_GETTER,
StaticWarningCode.MISSING_ENUM_CONSTANT_IN_SWITCH,
StrongModeCode.ASSIGNMENT_CAST,
StrongModeCode.DOWN_CAST_COMPOSITE,
StrongModeCode.DOWN_CAST_IMPLICIT,
StrongModeCode.DYNAMIC_CAST,
StrongModeCode.DYNAMIC_INVOKE,
StrongModeCode.INFERRED_TYPE,
StrongModeCode.INFERRED_TYPE_ALLOCATION,
StrongModeCode.INFERRED_TYPE_CLOSURE,
StrongModeCode.INFERRED_TYPE_LITERAL,
StrongModeCode.INVALID_FIELD_OVERRIDE,
StrongModeCode.INVALID_METHOD_OVERRIDE,
StrongModeCode.INVALID_METHOD_OVERRIDE_FROM_BASE,
StrongModeCode.INVALID_METHOD_OVERRIDE_FROM_MIXIN,
StrongModeCode.INVALID_PARAMETER_DECLARATION,
StrongModeCode.INVALID_SUPER_INVOCATION,
StrongModeCode.NON_GROUND_TYPE_CHECK_INFO,
StrongModeCode.STATIC_TYPE_ERROR,

TodoCode.TODO,

//
Expand Down Expand Up @@ -5806,6 +5824,117 @@ class StaticWarningCode extends ErrorCode {
ErrorType get type => ErrorType.STATIC_WARNING;
}

/**
* This class has Strong Mode specific error codes.
*
* These error codes tend to use the same message across different severity
* levels, so they are grouped for clarity.
*
* All of these error codes also use the "STRONG_MODE_" prefix in their name.
*/
class StrongModeCode extends ErrorCode {
static const String _implicitCastMessage =
'Unsound implicit cast from {0} to {1}';

static const String _typeCheckMessage =
'Type check failed: {0} is not of type {1}';

static const String _invalidOverrideMessage =
'The type of {0}.{1} ({2}) is not a '
'subtype of {3}.{1} ({4}).';

static const String _inferredTypeMessage = '{0} has inferred type {1}';

static const StrongModeCode DOWN_CAST_COMPOSITE = const StrongModeCode(
ErrorType.STATIC_WARNING, 'DOWN_CAST_COMPOSITE', _implicitCastMessage);

static const StrongModeCode DOWN_CAST_IMPLICIT = const StrongModeCode(
ErrorType.HINT, 'DOWN_CAST_IMPLICIT', _implicitCastMessage);

static const StrongModeCode DYNAMIC_CAST = const StrongModeCode(
ErrorType.HINT, 'DYNAMIC_CAST', _implicitCastMessage);

static const StrongModeCode ASSIGNMENT_CAST = const StrongModeCode(
ErrorType.HINT, 'ASSIGNMENT_CAST', _implicitCastMessage);

static const StrongModeCode INVALID_PARAMETER_DECLARATION =
const StrongModeCode(ErrorType.COMPILE_TIME_ERROR,
'INVALID_PARAMETER_DECLARATION', _typeCheckMessage);

static const StrongModeCode INFERRED_TYPE = const StrongModeCode(
ErrorType.HINT, 'INFERRED_TYPE', _inferredTypeMessage);

static const StrongModeCode INFERRED_TYPE_LITERAL = const StrongModeCode(
ErrorType.HINT, 'INFERRED_TYPE_LITERAL', _inferredTypeMessage);

static const StrongModeCode INFERRED_TYPE_ALLOCATION = const StrongModeCode(
ErrorType.HINT, 'INFERRED_TYPE_ALLOCATION', _inferredTypeMessage);

static const StrongModeCode INFERRED_TYPE_CLOSURE = const StrongModeCode(
ErrorType.HINT, 'INFERRED_TYPE_CLOSURE', _inferredTypeMessage);

static const StrongModeCode STATIC_TYPE_ERROR = const StrongModeCode(
ErrorType.COMPILE_TIME_ERROR,
'STATIC_TYPE_ERROR',
'Type check failed: {0} ({1}) is not of type {2}');

static const StrongModeCode INVALID_SUPER_INVOCATION = const StrongModeCode(
ErrorType.COMPILE_TIME_ERROR,
'INVALID_SUPER_INVOCATION',
"super call must be last in an initializer "
"list (see https://goo.gl/EY6hDP): {0}");

static const StrongModeCode NON_GROUND_TYPE_CHECK_INFO = const StrongModeCode(
ErrorType.HINT,
'NON_GROUND_TYPE_CHECK_INFO',
"Runtime check on non-ground type {0} may throw StrongModeError");

static const StrongModeCode DYNAMIC_INVOKE = const StrongModeCode(
ErrorType.HINT, 'DYNAMIC_INVOKE', '{0} requires a dynamic invoke');

static const StrongModeCode INVALID_METHOD_OVERRIDE = const StrongModeCode(
ErrorType.COMPILE_TIME_ERROR,
'INVALID_METHOD_OVERRIDE',
'Invalid override. $_invalidOverrideMessage');

static const StrongModeCode INVALID_METHOD_OVERRIDE_FROM_BASE =
const StrongModeCode(
ErrorType.COMPILE_TIME_ERROR,
'INVALID_METHOD_OVERRIDE_FROM_BASE',
'Base class introduces an invalid override. '
'$_invalidOverrideMessage');

static const StrongModeCode INVALID_METHOD_OVERRIDE_FROM_MIXIN =
const StrongModeCode(
ErrorType.COMPILE_TIME_ERROR,
'INVALID_METHOD_OVERRIDE_FROM_MIXIN',
'Mixin introduces an invalid override. $_invalidOverrideMessage');

static const StrongModeCode INVALID_FIELD_OVERRIDE = const StrongModeCode(
ErrorType.COMPILE_TIME_ERROR,
'INVALID_FIELD_OVERRIDE',
'Field declaration {3}.{1} cannot be '
'overridden in {0}.');

@override
final ErrorType type;

/**
* Initialize a newly created error code to have the given [type] and [name].
*
* The message associated with the error will be created from the given
* [message] template. The correction associated with the error will be
* created from the optional [correction] template.
*/
const StrongModeCode(ErrorType type, String name, String message,
[String correction])
: type = type,
super('STRONG_MODE_$name', message, correction);

@override
ErrorSeverity get errorSeverity => type.severity;
}

/**
* The error code indicating a marker in code for work that needs to be finished
* or revisited.
Expand Down
6 changes: 3 additions & 3 deletions pkg/analyzer/lib/src/generated/error_verifier.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import 'package:analyzer/src/generated/sdk.dart' show DartSdk, SdkLibrary;
import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/generated/utilities_dart.dart';
import 'package:analyzer/src/task/dart.dart';
import 'package:analyzer/src/task/strong/info.dart' show StaticInfo;
import 'package:analyzer/src/task/strong/checker.dart' as checker
show isKnownFunction;

/**
* A visitor used to traverse an AST structure looking for additional errors and
Expand Down Expand Up @@ -5743,8 +5744,7 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {

bool _expressionIsAssignableAtType(Expression expression,
DartType actualStaticType, DartType expectedStaticType) {
bool concrete =
_options.strongMode && StaticInfo.isKnownFunction(expression);
bool concrete = _options.strongMode && checker.isKnownFunction(expression);
if (concrete) {
actualStaticType =
_typeSystem.typeToConcreteType(_typeProvider, actualStaticType);
Expand Down
26 changes: 17 additions & 9 deletions pkg/analyzer/lib/src/generated/resolver.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/generated/static_type_analyzer.dart';
import 'package:analyzer/src/generated/type_system.dart';
import 'package:analyzer/src/generated/utilities_dart.dart';
import 'package:analyzer/src/task/strong/info.dart'
show InferredType, StaticInfo;

export 'package:analyzer/src/dart/resolver/inheritance_manager.dart';
export 'package:analyzer/src/dart/resolver/scope.dart';
Expand Down Expand Up @@ -4646,7 +4644,7 @@ class InferenceContext {
/**
* The error listener on which to record inference information.
*/
final AnalysisErrorListener _errorListener;
final ErrorReporter _errorReporter;

/**
* If true, emit hints when types are inferred
Expand Down Expand Up @@ -4679,7 +4677,7 @@ class InferenceContext {
// https://github.com/dart-lang/sdk/issues/25322
final List<DartType> _returnStack = <DartType>[];

InferenceContext._(this._errorListener, TypeProvider typeProvider,
InferenceContext._(this._errorReporter, TypeProvider typeProvider,
this._typeSystem, this._inferenceHints)
: _typeProvider = typeProvider;

Expand Down Expand Up @@ -4758,12 +4756,22 @@ class InferenceContext {
* [type] has been inferred as the type of [node].
*/
void recordInference(Expression node, DartType type) {
StaticInfo info = InferredType.create(_typeSystem, node, type);
if (!_inferenceHints || info == null) {
if (!_inferenceHints) {
return;
}
AnalysisError error = info.toAnalysisError();
_errorListener.onError(error);

ErrorCode error;
if (node is Literal) {
error = StrongModeCode.INFERRED_TYPE_LITERAL;
} else if (node is InstanceCreationExpression) {
error = StrongModeCode.INFERRED_TYPE_ALLOCATION;
} else if (node is FunctionExpression) {
error = StrongModeCode.INFERRED_TYPE_CLOSURE;
} else {
error = StrongModeCode.INFERRED_TYPE;
}

_errorReporter.reportErrorForNode(error, node, [node, type]);
}

List<DartType> _matchTypes(InterfaceType t1, InterfaceType t2) {
Expand Down Expand Up @@ -5577,7 +5585,7 @@ class ResolverVisitor extends ScopedVisitor {
strongModeHints = options.strongModeHints;
}
this.inferenceContext = new InferenceContext._(
errorListener, typeProvider, typeSystem, strongModeHints);
errorReporter, typeProvider, typeSystem, strongModeHints);
this.typeAnalyzer = new StaticTypeAnalyzer(this);
}

Expand Down
36 changes: 24 additions & 12 deletions pkg/analyzer/lib/src/generated/type_system.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/dart/element/type.dart';
import 'package:analyzer/src/generated/engine.dart' show AnalysisContext;
import 'package:analyzer/src/generated/engine.dart'
show AnalysisContext, AnalysisOptionsImpl;
import 'package:analyzer/src/generated/resolver.dart' show TypeProvider;
import 'package:analyzer/src/generated/utilities_dart.dart';

Expand All @@ -23,6 +24,15 @@ typedef bool _GuardedSubtypeChecker<T>(T t1, T t2, Set<Element> visited);
* https://github.com/dart-lang/dev_compiler/blob/master/STRONG_MODE.md
*/
class StrongTypeSystemImpl extends TypeSystem {
/**
* True if implicit casts should be allowed, otherwise false.
*
* This affects the behavior of [isAssignableTo].
*/
final bool implicitCasts;

StrongTypeSystemImpl({this.implicitCasts: true});

bool anyParameterType(FunctionType ft, bool predicate(DartType t)) {
return ft.parameters.any((p) => predicate(p.type));
}
Expand Down Expand Up @@ -290,6 +300,10 @@ class StrongTypeSystemImpl extends TypeSystem {
return true;
}

if (!implicitCasts) {
return false;
}

// Don't allow implicit downcasts between function types
// and call method objects, as these will almost always fail.
if ((fromType is FunctionType && getCallMethodType(toType) != null) ||
Expand All @@ -309,16 +323,13 @@ class StrongTypeSystemImpl extends TypeSystem {
return false;
}

// If the subtype relation goes the other way, allow the implicit downcast.
// TODO(leafp): Emit warnings and hints for these in some way.
// TODO(leafp): Consider adding a flag to disable these? Or just rely on
// --warnings-as-errors?
// If the subtype relation goes the other way, allow the implicit
// downcast.
if (isSubtypeOf(toType, fromType) || toType.isAssignableTo(fromType)) {
// TODO(leafp): error if type is known to be exact (literal,
// instance creation).
// TODO(leafp): Warn on composite downcast.
// TODO(leafp): hint on object/dynamic downcast.
// TODO(leafp): Consider allowing assignment casts.
// TODO(leafp,jmesserly): we emit warnings/hints for these in
// src/task/strong/checker.dart, which is a bit inconsistent. That
// code should be handled into places that use isAssignableTo, such as
// ErrorVerifier.
return true;
}

Expand Down Expand Up @@ -1180,8 +1191,9 @@ abstract class TypeSystem {
* Create either a strong mode or regular type system based on context.
*/
static TypeSystem create(AnalysisContext context) {
return (context.analysisOptions.strongMode)
? new StrongTypeSystemImpl()
var options = context.analysisOptions as AnalysisOptionsImpl;
return options.strongMode
? new StrongTypeSystemImpl(implicitCasts: options.implicitCasts)
: new TypeSystemImpl();
}
}
Expand Down
9 changes: 6 additions & 3 deletions pkg/analyzer/lib/src/task/dart.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5682,9 +5682,12 @@ class StrongModeVerifyUnitTask extends SourceBasedAnalysisTask {
CompilationUnit unit = getRequiredInput(UNIT_INPUT);
AnalysisOptionsImpl options = context.analysisOptions;
if (options.strongMode) {
unit.accept(new CodeChecker(
typeProvider, new StrongTypeSystemImpl(), errorListener,
options));
CodeChecker checker = new CodeChecker(
typeProvider,
new StrongTypeSystemImpl(implicitCasts: options.implicitCasts),
errorListener,
options);
checker.visitCompilationUnit(unit);
}
//
// Record outputs.
Expand Down
3 changes: 0 additions & 3 deletions pkg/analyzer/lib/src/task/options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import 'package:analyzer/src/generated/java_engine.dart';
import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/generated/utilities_general.dart';
import 'package:analyzer/src/task/general.dart';
import 'package:analyzer/src/task/strong/info.dart';
import 'package:analyzer/task/general.dart';
import 'package:analyzer/task/model.dart';
import 'package:source_span/source_span.dart';
Expand Down Expand Up @@ -161,8 +160,6 @@ class ErrorFilterOptionValidator extends OptionsValidator {
_errorCodes = new HashSet<String>();
// Engine codes.
_errorCodes.addAll(ErrorCode.values.map((ErrorCode code) => code.name));
// Strong-mode codes.
_errorCodes.addAll(StaticInfo.names);
}
return _errorCodes;
}
Expand Down
49 changes: 49 additions & 0 deletions pkg/analyzer/lib/src/task/strong/ast_properties.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright (c) 2016, 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.

/// Properties that result from Strong Mode analysis on an AST.
///
/// These properties are not public, but provided by use of back-ends such as
/// Dart Dev Compiler.
import 'package:analyzer/analyzer.dart';
import 'package:analyzer/dart/element/type.dart';

const String _implicitCast = '_implicitCast';
const String _hasImplicitCasts = '_hasImplicitCasts';
const String _isDynamicInvoke = '_isDynamicInvoke';

/// True if this compilation unit has any implicit casts, otherwise false.
///
/// See also [getImplicitCast].
bool hasImplicitCasts(CompilationUnit node) {
return node.getProperty/*<bool>*/(_hasImplicitCasts) ?? false;
}

/// Sets [hasImplicitCasts] property for this compilation unit.
void setHasImplicitCasts(CompilationUnit node, bool value) {
node.setProperty(_hasImplicitCasts, value == true ? true : null);
}

/// If this expression has an implicit cast, returns the type it is coerced to,
/// otherwise returns null.
DartType getImplicitCast(Expression node) {
return node.getProperty/*<DartType>*/(_implicitCast);
}

/// Sets the result of [getImplicitCast] for this node.
void setImplicitCast(Expression node, DartType type) {
node.setProperty(_implicitCast, type);
}

/// True if this node is a dynamic operation that requires dispatch and/or
/// checking at runtime.
bool isDynamicInvoke(Expression node) {
return node.getProperty/*<bool>*/(_isDynamicInvoke) ?? false;
}

/// Sets [isDynamicInvoke] property for this expression
void setIsDynamicInvoke(Expression node, bool value) {
node.setProperty(_isDynamicInvoke, value == true ? true : null);
}
Loading

0 comments on commit 831c875

Please sign in to comment.