Skip to content

Commit

Permalink
Reland: Implement inheritance/override checks from the spec.
Browse files Browse the repository at this point in the history 
Relands https://dart-review.googlesource.com/c/sdk/+/76061
Was reverted in https://dart-review.googlesource.com/c/sdk/+/76301

The difference with the original CL is that we don't look into
superclass and mixins of mixed-in for concrete members. This reduces
the number of errors in Flutter codebase to 1.


R=brianwilkerson@google.com

Bug: dart-lang/sdk#34392
Change-Id: I86256b598d116439194cbaf4d09b4f72013d6563
Reviewed-on: https://dart-review.googlesource.com/76340
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
  • Loading branch information
@scheglov
scheglov authored and commit-bot@chromium.org committed Sep 25, 2018
1 parent c23a981 commit 9185294
Show file tree
Hide file tree
Showing 24 changed files with 856 additions and 1,051 deletions.
15 changes: 3 additions & 12 deletions pkg/analyzer/lib/error/error.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ const List<ErrorCode> errorCodeValues = const [
CompileTimeErrorCode.IMPORT_INTERNAL_LIBRARY,
CompileTimeErrorCode.IMPORT_OF_NON_LIBRARY,
CompileTimeErrorCode.INCONSISTENT_CASE_EXPRESSION_TYPES,
CompileTimeErrorCode.INCONSISTENT_INHERITANCE,
CompileTimeErrorCode.INCONSISTENT_INHERITANCE_GETTER_AND_METHOD,
CompileTimeErrorCode.INITIALIZER_FOR_NON_EXISTENT_FIELD,
CompileTimeErrorCode.INITIALIZER_FOR_STATIC_FIELD,
CompileTimeErrorCode.INITIALIZING_FORMAL_FOR_NON_EXISTENT_FIELD,
Expand All @@ -164,6 +166,7 @@ const List<ErrorCode> errorCodeValues = const [
CompileTimeErrorCode.INVALID_MODIFIER_ON_CONSTRUCTOR,
CompileTimeErrorCode.INVALID_MODIFIER_ON_SETTER,
CompileTimeErrorCode.INVALID_INLINE_FUNCTION_TYPE,
CompileTimeErrorCode.INVALID_OVERRIDE,
CompileTimeErrorCode.INVALID_REFERENCE_TO_THIS,
CompileTimeErrorCode.INVALID_TYPE_ARGUMENT_IN_CONST_LIST,
CompileTimeErrorCode.INVALID_TYPE_ARGUMENT_IN_CONST_MAP,
Expand Down Expand Up @@ -532,7 +535,6 @@ const List<ErrorCode> errorCodeValues = const [
StaticTypeWarningCode.ILLEGAL_ASYNC_GENERATOR_RETURN_TYPE,
StaticTypeWarningCode.ILLEGAL_ASYNC_RETURN_TYPE,
StaticTypeWarningCode.ILLEGAL_SYNC_GENERATOR_RETURN_TYPE,
StaticTypeWarningCode.INCONSISTENT_METHOD_INHERITANCE,
StaticTypeWarningCode.INSTANCE_ACCESS_TO_STATIC_MEMBER,
StaticTypeWarningCode.INVALID_ASSIGNMENT,
StaticTypeWarningCode.INVOCATION_OF_NON_FUNCTION,
Expand Down Expand Up @@ -591,14 +593,6 @@ const List<ErrorCode> errorCodeValues = const [
StaticWarningCode.FUNCTION_WITHOUT_CALL,
StaticWarningCode.IMPORT_DUPLICATED_LIBRARY_NAMED,
StaticWarningCode.IMPORT_OF_NON_LIBRARY,
StaticWarningCode.INCONSISTENT_METHOD_INHERITANCE_GETTER_AND_METHOD,
StaticWarningCode.INVALID_GETTER_OVERRIDE_RETURN_TYPE,
StaticWarningCode.INVALID_METHOD_OVERRIDE_NAMED_PARAM_TYPE,
StaticWarningCode.INVALID_METHOD_OVERRIDE_NORMAL_PARAM_TYPE,
StaticWarningCode.INVALID_METHOD_OVERRIDE_OPTIONAL_PARAM_TYPE,
StaticWarningCode.INVALID_METHOD_OVERRIDE_RETURN_TYPE,
StaticWarningCode.INVALID_METHOD_OVERRIDE_TYPE_PARAMETERS,
StaticWarningCode.INVALID_METHOD_OVERRIDE_TYPE_PARAMETER_BOUND,
StaticWarningCode.INVALID_OVERRIDE_DIFFERENT_DEFAULT_VALUES_NAMED,
StaticWarningCode.INVALID_OVERRIDE_DIFFERENT_DEFAULT_VALUES_POSITIONAL,
StaticWarningCode.INVALID_OVERRIDE_NAMED,
Expand Down Expand Up @@ -674,9 +668,6 @@ const List<ErrorCode> errorCodeValues = const [
StrongModeCode.INVALID_CAST_METHOD,
StrongModeCode.INVALID_CAST_FUNCTION,
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.NO_DEFAULT_BOUNDS,
Expand Down
23 changes: 15 additions & 8 deletions pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,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/error/codes.dart';
import 'package:analyzer/src/error/inheritance_override.dart';
import 'package:analyzer/src/error/pending_error.dart';
import 'package:analyzer/src/generated/declaration_resolver.dart';
import 'package:analyzer/src/generated/engine.dart';
Expand Down Expand Up @@ -294,14 +295,13 @@ class LibraryAnalyzer {
RecordingErrorListener errorListener = _getErrorListener(file);

AnalysisOptionsImpl options = _analysisOptions as AnalysisOptionsImpl;
CodeChecker checker = new CodeChecker(
_typeProvider,
new StrongTypeSystemImpl(_typeProvider,
implicitCasts: options.implicitCasts,
declarationCasts: options.declarationCasts,
nonnullableTypes: options.nonnullableTypes),
errorListener,
options);
var typeSystem = new StrongTypeSystemImpl(_typeProvider,
implicitCasts: options.implicitCasts,
declarationCasts: options.declarationCasts,
nonnullableTypes: options.nonnullableTypes);

CodeChecker checker =
new CodeChecker(_typeProvider, typeSystem, errorListener, options);
checker.visitCompilationUnit(unit);

ErrorReporter errorReporter = _getErrorReporter(file);
Expand All @@ -316,6 +316,13 @@ class LibraryAnalyzer {
//
_computeConstantErrors(errorReporter, unit);

//
// Compute inheritance and override errors.
//
var inheritanceOverrideVerifier =
new InheritanceOverrideVerifier(typeSystem, errorReporter);
inheritanceOverrideVerifier.verifyUnit(unit);

//
// Use the ErrorVerifier to compute errors.
//
Expand Down
42 changes: 29 additions & 13 deletions pkg/analyzer/lib/src/dart/element/type.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1766,13 +1766,19 @@ class InterfaceTypeImpl extends TypeImpl implements InterfaceType {
}

ExecutableElement lookUpInheritedMember(String name, LibraryElement library,
{bool concrete: false, int stopMixinIndex, bool setter: false}) {
{bool concrete: false,
int startMixinIndex,
bool setter: false,
bool thisType: false}) {
HashSet<ClassElement> visitedClasses = new HashSet<ClassElement>();

/// TODO(scheglov) Remove [includeSupers]. It is used only to work around
/// the problem with Flutter code base (using old super-mixins).
ExecutableElement lookUpImpl(InterfaceTypeImpl type,
{bool acceptAbstract: false,
bool includeType: true,
int stopMixinIndex}) {
bool includeSupers: true,
int startMixinIndex}) {
if (type == null || !visitedClasses.add(type.element)) {
return null;
}
Expand All @@ -1792,14 +1798,18 @@ class InterfaceTypeImpl extends TypeImpl implements InterfaceType {
}
}

var mixins = type.mixins;
for (var i = 0; i < mixins.length; i++) {
if (stopMixinIndex != null && i >= stopMixinIndex) {
break;
}
var result = lookUpImpl(mixins[i], acceptAbstract: acceptAbstract);
if (result != null) {
return result;
if (includeSupers) {
var mixins = type.mixins;
startMixinIndex ??= mixins.length;
for (var i = startMixinIndex - 1; i >= 0; i--) {
var result = lookUpImpl(
mixins[i],
acceptAbstract: acceptAbstract,
includeSupers: false,
);
if (result != null) {
return result;
}
}
}

Expand All @@ -1814,10 +1824,16 @@ class InterfaceTypeImpl extends TypeImpl implements InterfaceType {
}
}

return lookUpImpl(type.superclass, acceptAbstract: acceptAbstract);
if (includeSupers) {
return lookUpImpl(type.superclass, acceptAbstract: acceptAbstract);
}

return null;
}

if (element.isMixin) {
// TODO(scheglov) We should choose the most specific signature.
// Not just the first signature.
for (InterfaceType constraint in superclassConstraints) {
var result = lookUpImpl(constraint, acceptAbstract: true);
if (result != null) {
Expand All @@ -1828,8 +1844,8 @@ class InterfaceTypeImpl extends TypeImpl implements InterfaceType {
} else {
return lookUpImpl(
this,
includeType: false,
stopMixinIndex: stopMixinIndex,
includeType: thisType,
startMixinIndex: startMixinIndex,
);
}
}
Expand Down
66 changes: 2 additions & 64 deletions pkg/analyzer/lib/src/dart/resolver/inheritance_manager.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import 'package:analyzer/src/dart/ast/token.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/dart/element/member.dart';
import 'package:analyzer/src/dart/element/type.dart';
import 'package:analyzer/src/error/codes.dart';
import 'package:analyzer/src/generated/type_system.dart';
import 'package:analyzer/src/generated/utilities_dart.dart';

Expand Down Expand Up @@ -716,34 +715,10 @@ class InheritanceManager {
}
}

/**
* This method is used to report errors on when they are found computing inheritance information.
* See [ErrorVerifier.checkForInconsistentMethodInheritance] to see where these generated
* error codes are reported back into the analysis engine.
*
* @param classElt the location of the source for which the exception occurred
* @param offset the offset of the location of the error
* @param length the length of the location of the error
* @param errorCode the error code to be associated with this error
* @param arguments the arguments used to build the error message
*/
void _reportError(
ClassElement classElt, ErrorCode errorCode, List<Object> arguments) {
if (ignoreErrors) {
return;
}
HashSet<AnalysisError> errorSet = _errorsInClassElement.putIfAbsent(
classElt, () => new HashSet<AnalysisError>());
errorSet.add(new AnalysisError(classElt.source, classElt.nameOffset,
classElt.nameLength, errorCode, arguments));
}

/**
* Given the set of methods defined by classes above [classElt] in the class hierarchy,
* apply the appropriate inheritance rules to determine those methods inherited by or overridden
* by [classElt]. Also report static warnings
* [StaticTypeWarningCode.INCONSISTENT_METHOD_INHERITANCE] and
* [StaticWarningCode.INCONSISTENT_METHOD_INHERITANCE_GETTER_AND_METHOD] if appropriate.
* by [classElt].
*
* @param classElt the class element to query.
* @param unionMap a mapping from method name to the set of unique (in terms of signature) methods
Expand Down Expand Up @@ -838,38 +813,7 @@ class InheritanceManager {
//
resultMap[key] = elements[subtypesOfAllOtherTypesIndexes[0]];
} else {
if (subtypesOfAllOtherTypesIndexes.isEmpty) {
//
// Determine if the current class has a method or accessor with
// the member name, if it does then then this class does not
// "inherit" from any of the supertypes. See issue 16134.
//
bool classHasMember = false;
if (allMethods) {
classHasMember = classElt.getMethod(key) != null;
} else {
List<PropertyAccessorElement> accessors = classElt.accessors;
for (int i = 0; i < accessors.length; i++) {
if (accessors[i].name == key) {
classHasMember = true;
}
}
}
//
// Example: class A inherited only 2 method named 'm'.
// One has the function type '() -> int' and one has the function
// type '() -> String'. Since neither is a subtype of the other,
// we create a warning, and have this class inherit nothing.
//
if (!classHasMember) {
String firstTwoFunctionTypesStr =
"${executableElementTypes[0]}, ${executableElementTypes[1]}";
_reportError(
classElt,
StaticTypeWarningCode.INCONSISTENT_METHOD_INHERITANCE,
[key, firstTwoFunctionTypesStr]);
}
} else {
if (!subtypesOfAllOtherTypesIndexes.isEmpty) {
//
// Example: class A inherits 2 methods named 'm'.
// One has the function type '(int) -> dynamic' and one has the
Expand All @@ -896,12 +840,6 @@ class InheritanceManager {
resultMap[key] = mergedExecutableElement;
}
}
} else {
_reportError(
classElt,
StaticWarningCode
.INCONSISTENT_METHOD_INHERITANCE_GETTER_AND_METHOD,
[key]);
}
}
});
Expand Down
Loading

0 comments on commit 9185294

Please sign in to comment.