languages/dart-sdk
languages/dart-sdk
1
0
Fork 0
mirror of https://github.com/dart-lang/sdk synced 2024-09-16 02:27:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

Revert "[analyzer] Support hints for visibleForOverriding"

Browse source 
This reverts commit 8357efa65c.

Reason for revert: This caused some internal breakage because the annotation is not being used as defined by this CL. There is a path forward, but it requires reverting the CL temporarily. At least one problem is that the annotation is not allowed on a constructor, which is technically valid, but in practice might be something we want to allow anyway. While constructors aren't overridden, they do need to be called from subclasses.

Also, it uncovered a minor problem: when the annotation is applied to an unnamed constructor the message contains an empty name.

Original change's description:
> [analyzer] Support hints for visibleForOverriding
>
> This adds support to detect whether an annotation is
> `@visibleForOverriding` or whether such annotation is present on an
> element.
> Also, the analyzer now reports invalid uses of that annotation (when
> the annotated member is static, in an extension or private).
> Finally, we report hints when a member declared with that annotation is
> uses outside of a method declaration.
>
> Closes: https://github.com/dart-lang/sdk/issues/46155
> Change-Id: I368ea4be6bd5c1da5383a3c9dacdeee02a72af86
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202620
> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
> Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

TBR=brianwilkerson@google.com,srawlins@google.com,oss@simonbinder.eu

Change-Id: Ib9731dbebfc41d0d68aac0fb90faf3bd3ac1ef94
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202842
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
This commit is contained in:
Brian Wilkerson 2021-06-08 21:26:11 +00:00 committed by commit-bot@chromium.org
parent eb2836872a
commit 465a8a8754
10 changed files with 10 additions and 340 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
pkg/analyzer/lib/dart/element/element.dart
View file

@ -610,10 +610,6 @@ abstract class Element implements AnalysisTarget {
/// or `@UseResult('..')`.
bool get hasUseResult;
/// Return `true` if this element has an annotation of the form
/// `@visibleForOverriding`.
bool get hasVisibleForOverriding;
/// Return `true` if this element has an annotation of the form
/// `@visibleForTemplate`.
bool get hasVisibleForTemplate;
@ -824,10 +820,6 @@ abstract class ElementAnnotation implements ConstantEvaluationTarget {
/// requiring use.
bool get isUseResult;
/// Return `true` if this annotation marks the associated member as being
/// visible for overriding only.
bool get isVisibleForOverriding;
/// Return `true` if this annotation marks the associated member as being
/// visible for template files.
bool get isVisibleForTemplate;

2
pkg/analyzer/lib/error/error.dart
View file

@ -554,11 +554,9 @@ const List<ErrorCode> errorCodeValues = [
HintCode.INVALID_SEALED_ANNOTATION,
HintCode.INVALID_USE_OF_INTERNAL_MEMBER,
HintCode.INVALID_USE_OF_PROTECTED_MEMBER,
HintCode.INVALID_USE_OF_VISIBLE_FOR_OVERRIDING_MEMBER,
HintCode.INVALID_USE_OF_VISIBLE_FOR_TEMPLATE_MEMBER,
HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER,
HintCode.INVALID_VISIBILITY_ANNOTATION,
HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION,
HintCode.MISSING_JS_LIB_ANNOTATION,
HintCode.MISSING_REQUIRED_PARAM,
HintCode.MISSING_REQUIRED_PARAM_WITH_DETAILS,

23
pkg/analyzer/lib/src/dart/element/element.dart
View file

@ -1749,10 +1749,6 @@ class ElementAnnotationImpl implements ElementAnnotation {
/// requiring use.
static const String _USE_RESULT_VARIABLE_NAME = "useResult";
/// The name of the top-level variable used to mark a member as being visible
/// for overriding only.
static const String _VISIBLE_FOR_OVERRIDING_NAME = 'visibleForOverriding';
/// The name of the top-level variable used to mark a method as being
/// visible for templates.
static const String _VISIBLE_FOR_TEMPLATE_VARIABLE_NAME =
@ -1871,10 +1867,6 @@ class ElementAnnotationImpl implements ElementAnnotation {
libraryName: _META_LIB_NAME, className: _USE_RESULT_CLASS_NAME) ||
_isPackageMetaGetter(_USE_RESULT_VARIABLE_NAME);
@override
bool get isVisibleForOverriding =>
_isPackageMetaGetter(_VISIBLE_FOR_OVERRIDING_NAME);
@override
bool get isVisibleForTemplate => _isTopGetter(
libraryName: _NG_META_LIB_NAME,
@ -2253,18 +2245,6 @@ abstract class ElementImpl implements Element {
return false;
}
@override
bool get hasVisibleForOverriding {
final metadata = this.metadata;
for (var i = 0; i < metadata.length; i++) {
var annotation = metadata[i];
if (annotation.isVisibleForOverriding) {
return true;
}
}
return false;
}
@override
bool get hasVisibleForTemplate {
final metadata = this.metadata;
@ -4377,9 +4357,6 @@ class MultiplyDefinedElementImpl implements MultiplyDefinedElement {
@override
bool get hasUseResult => false;
@override
bool get hasVisibleForOverriding => false;
@override
bool get hasVisibleForTemplate => false;

3
pkg/analyzer/lib/src/dart/element/member.dart
View file

@ -521,9 +521,6 @@ abstract class Member implements Element {
@override
bool get hasUseResult => _declaration.hasUseResult;
@override
bool get hasVisibleForOverriding => _declaration.hasVisibleForOverriding;
@override
bool get hasVisibleForTemplate => _declaration.hasVisibleForTemplate;

20
pkg/analyzer/lib/src/dart/error/hint_codes.dart
View file

@ -1241,17 +1241,6 @@ class HintCode extends AnalyzerErrorCode {
"The member '{0}' can only be used within instance members of subclasses "
"of '{1}'.");
/**
* This hint is generated anywhere where a member annotated with
* `@visibleForOverriding` is used for another purpose than overriding it.
*
* Parameters:
* 0: the name of the member
*/
static const HintCode INVALID_USE_OF_VISIBLE_FOR_OVERRIDING_MEMBER = HintCode(
'INVALID_USE_OF_VISIBLE_FOR_OVERRIDING_MEMBER',
"The member '{0}' can only be used for overriding.");
/**
* This hint is generated anywhere where a member annotated with
* `@visibleForTemplate` is used outside of a "template" Dart file.
@ -1330,15 +1319,6 @@ class HintCode extends AnalyzerErrorCode {
"meaningful on declarations of public members.",
hasPublishedDocs: true);
/// Hint when an `@visibleForOverriding` annotation is used on something that
/// isn't an interface member.
static const HintCode INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION = HintCode(
'INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION',
"The declaration '{0}' is annotated with 'visibleForOverriding'. As '{0}' "
"is not an interface member that could be overriden, the annotation is "
'meaningless.',
);
/**
* Generate a hint for an element that is annotated with `@JS(...)` whose
* library declaration is not similarly annotated.

67
pkg/analyzer/lib/src/error/best_practices_verifier.dart
View file

@ -221,8 +221,7 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
HintCode.INVALID_SEALED_ANNOTATION, node, [node.element!.name]);
}
} else if (element.isVisibleForTemplate == true ||
element.isVisibleForTesting == true ||
element.isVisibleForOverriding == true) {
element.isVisibleForTesting == true) {
if (parent is Declaration) {
void reportInvalidAnnotation(Element declaredElement) {
_errorReporter.reportErrorForNode(
@ -231,49 +230,23 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
[declaredElement.name, node.name.name]);
}
void reportInvalidVisibleForOverriding(Element declaredElement) {
_errorReporter.reportErrorForNode(
HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION,
node,
[declaredElement.name, node.name.name]);
}
if (parent is TopLevelVariableDeclaration) {
for (VariableDeclaration variable in parent.variables.variables) {
var variableElement =
variable.declaredElement as TopLevelVariableElement;
if (Identifier.isPrivateName(variableElement.name)) {
reportInvalidAnnotation(variableElement);
}
if (element.isVisibleForOverriding == true) {
// Top-level variables can't be overridden.
reportInvalidVisibleForOverriding(variableElement);
var element = variable.declaredElement as TopLevelVariableElement;
if (Identifier.isPrivateName(element.name)) {
reportInvalidAnnotation(element);
}
}
} else if (parent is FieldDeclaration) {
for (VariableDeclaration variable in parent.fields.variables) {
var fieldElement = variable.declaredElement as FieldElement;
if (parent.isStatic && element.isVisibleForOverriding == true) {
reportInvalidVisibleForOverriding(fieldElement);
}
if (Identifier.isPrivateName(fieldElement.name)) {
reportInvalidAnnotation(fieldElement);
var element = variable.declaredElement as FieldElement;
if (Identifier.isPrivateName(element.name)) {
reportInvalidAnnotation(element);
}
}
} else if (parent.declaredElement != null) {
final declaredElement = parent.declaredElement!;
if (element.isVisibleForOverriding &&
(!declaredElement.isInstanceMember ||
declaredElement.enclosingElement is ExtensionElement)) {
reportInvalidVisibleForOverriding(declaredElement);
}
if (Identifier.isPrivateName(declaredElement.name!)) {
reportInvalidAnnotation(declaredElement);
}
} else if (parent.declaredElement != null &&
Identifier.isPrivateName(parent.declaredElement!.name!)) {
reportInvalidAnnotation(parent.declaredElement!);
}
} else {
// Something other than a declaration was annotated. Whatever this is,
@ -1897,8 +1870,6 @@ class _InvalidAccessVerifier {
}
}
bool hasVisibleForOverriding = _hasVisibleForOverriding(element);
// At this point, [identifier] was not cleared as protected access, nor
// cleared as access for templates or testing. Report a violation for each
// annotation present.
@ -1936,11 +1907,6 @@ class _InvalidAccessVerifier {
node,
[name, definingClass!.source!.uri]);
}
if (hasVisibleForOverriding) {
_errorReporter.reportErrorForNode(
HintCode.INVALID_USE_OF_VISIBLE_FOR_OVERRIDING_MEMBER, node, [name]);
}
}
bool _hasInternal(Element? element) {
@ -1977,19 +1943,6 @@ class _InvalidAccessVerifier {
return element.thisType.asInstanceOf(superElement) != null;
}
bool _hasVisibleForOverriding(Element element) {
if (element.hasVisibleForOverriding) {
return true;
}
if (element is PropertyAccessorElement &&
element.variable.hasVisibleForOverriding) {
return true;
}
return false;
}
bool _hasVisibleForTemplate(Element? element) {
if (element == null) {
return false;

4
pkg/analyzer/lib/src/test_utilities/mock_packages.dart
View file

@ -37,7 +37,6 @@ const _Protected protected = const _Protected();
const Required required = const Required();
const _Sealed sealed = const _Sealed();
const UseResult useResult = UseResult();
const _VisibleForOverriding visibleForOverriding = _VisibleForOverriding();
const _VisibleForTesting visibleForTesting = const _VisibleForTesting();
class _AlwaysThrows {
@ -89,9 +88,6 @@ class UseResult {
final String reason;
const UseResult([this.reason = '']);
}
class _VisibleForOverriding {
const _VisibleForOverriding();
}
class _VisibleForTesting {
const _VisibleForTesting();
}

106
pkg/analyzer/test/src/diagnostics/invalid_use_of_visible_for_overriding_member_test.dart
View file

@ -1,106 +0,0 @@
// 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.
import 'package:analyzer/src/error/codes.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
import '../dart/resolution/context_collection_resolution.dart';
void main() {
defineReflectiveSuite(() {
defineReflectiveTests(InvalidUseOfVisibleForOverridingMemberTest);
});
}
@reflectiveTest
class InvalidUseOfVisibleForOverridingMemberTest
extends PubPackageResolutionTest {
@override
void setUp() {
super.setUp();
writeTestPackageConfigWithMeta();
}
test_differentLibrary_invalid() async {
newFile('$testPackageLibPath/a.dart', content: '''
import 'package:meta/meta.dart';
class Parent {
@visibleForOverriding
void foo() {}
}
''');
await assertErrorsInCode('''
import 'a.dart';
class Child extends Parent {
Child() {
foo();
}
}
''', [
error(HintCode.INVALID_USE_OF_VISIBLE_FOR_OVERRIDING_MEMBER, 63, 3),
]);
}
test_differentLibrary_valid_onlyOverride() async {
newFile('$testPackageLibPath/a.dart', content: '''
import 'package:meta/meta.dart';
class Parent {
@visibleForOverriding
void foo() {}
}
''');
await assertNoErrorsInCode('''
import 'a.dart';
class Child extends Parent {
@override
void foo() {}
}
''');
}
test_differentLibrary_valid_overrideAndUse() async {
newFile('$testPackageLibPath/a.dart', content: '''
import 'package:meta/meta.dart';
class Parent {
@visibleForOverriding
void foo() {}
}
''');
await assertNoErrorsInCode('''
import 'a.dart';
class Child extends Parent {
@override
void foo() {}
void bar() {
foo();
}
}
''');
}
test_sameLibrary() async {
await assertNoErrorsInCode('''
import 'package:meta/meta.dart';
class Parent {
@visibleForOverriding
void foo() {}
}
class Child extends Parent {
Child() {
foo();
}
}
''');
}
}

111
pkg/analyzer/test/src/diagnostics/invalid_visible_for_overriding_annotation_test.dart
View file

@ -1,111 +0,0 @@
// 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.
import 'package:analyzer/src/dart/error/hint_codes.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
import '../dart/resolution/context_collection_resolution.dart';
void main() {
defineReflectiveSuite(() {
defineReflectiveTests(InvalidVisibleForOverridingAnnotationTest);
});
}
@reflectiveTest
class InvalidVisibleForOverridingAnnotationTest
extends PubPackageResolutionTest {
@override
void setUp() {
super.setUp();
writeTestPackageConfigWithMeta();
}
test_invalid_class() async {
await assertErrorsInCode('''
import 'package:meta/meta.dart';
@visibleForOverriding
class C {}
''', [error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 33, 21)]);
}
test_invalid_constructor() async {
await assertErrorsInCode(r'''
import 'package:meta/meta.dart';
class C {
@visibleForOverriding
C();
}
''', [
error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 45, 21),
]);
}
test_invalid_extensionMember() async {
await assertErrorsInCode(r'''
import 'package:meta/meta.dart';
extension E on String {
@visibleForOverriding
void foo() {}
}
''', [
error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 59, 21),
]);
}
test_invalid_staticMember() async {
await assertErrorsInCode(r'''
import 'package:meta/meta.dart';
class C {
@visibleForOverriding
static void m() {}
}
''', [
error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 45, 21),
]);
}
test_invalid_topLevelFunction() async {
await assertErrorsInCode(r'''
import 'package:meta/meta.dart';
@visibleForOverriding void foo() {}
''', [
error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 33, 21),
]);
}
test_invalid_topLevelVariable() async {
await assertErrorsInCode(r'''
import 'package:meta/meta.dart';
@visibleForOverriding final a = 1;
''', [
error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 33, 21),
]);
}
test_invalid_topLevelVariable_multi() async {
await assertErrorsInCode(r'''
import 'package:meta/meta.dart';
@visibleForOverriding var a = 1, b;
''', [
error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 33, 21),
error(HintCode.INVALID_VISIBLE_FOR_OVERRIDING_ANNOTATION, 33, 21),
]);
}
test_valid() async {
await assertNoErrorsInCode(r'''
import 'package:meta/meta.dart';
class C {
@visibleForOverriding
void m() {}
@visibleForOverriding
int x = 3;
@visibleForOverriding
int get y => 5;
}
''');
}
}

6
pkg/analyzer/test/src/diagnostics/test_all.dart
View file

@ -353,16 +353,12 @@ import 'invalid_use_of_internal_member_test.dart'
as invalid_use_of_internal_member;
import 'invalid_use_of_protected_member_test.dart'
as invalid_use_of_protected_member;
import 'invalid_use_of_visible_for_overriding_member_test.dart'
as invalid_use_of_visible_for_overriding_member;
import 'invalid_use_of_visible_for_template_member_test.dart'
as invalid_use_of_visible_for_template_member;
import 'invalid_use_of_visible_for_testing_member_test.dart'
as invalid_use_of_visible_for_testing_member;
import 'invalid_visibility_annotation_test.dart'
as invalid_visibility_annotation;
import 'invalid_visible_for_overriding_annotation_test.dart'
as invalid_visible_for_overriding_annotation;
import 'invocation_of_extension_without_call_test.dart'
as invocation_of_extension_without_call;
import 'invocation_of_non_function_expression_test.dart'
@ -939,11 +935,9 @@ main() {
invalid_use_of_covariant_in_extension.main();
invalid_use_of_internal_member.main();
invalid_use_of_protected_member.main();
invalid_use_of_visible_for_overriding_member.main();
invalid_use_of_visible_for_template_member.main();
invalid_use_of_visible_for_testing_member.main();
invalid_visibility_annotation.main();
invalid_visible_for_overriding_annotation.main();
invocation_of_extension_without_call.main();
invocation_of_non_function_expression.main();
label_in_outer_scope.main();