Skip to content

Commit

Permalink
Initial implementation and tests of @Donotsubmit.
Browse files Browse the repository at this point in the history
This is my first time contributing something like this, so I mostly went with my gut and tried to refer to similar annotations and diagnostics.

Happy to make adjustments.

[email protected],[email protected]
CC=​[email protected]

Bug: #28113
Change-Id: I628311ed99f62b04c37bd1e0ec82ae0b1652d7f9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/360481
Commit-Queue: Matan Lurey <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>
  • Loading branch information
matanlurey authored and Commit Queue committed Apr 2, 2024
1 parent 8ca9771 commit a7c57c3
Show file tree
Hide file tree
Showing 14 changed files with 754 additions and 109 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3673,6 +3673,8 @@ WarningCode.INVALID_REQUIRED_POSITIONAL_PARAM:
status: hasFix
WarningCode.INVALID_SEALED_ANNOTATION:
status: hasFix
WarningCode.INVALID_USE_OF_DO_NOT_SUBMIT_MEMBER:
status: needsEvaluation
WarningCode.INVALID_USE_OF_INTERNAL_MEMBER:
status: noFix
WarningCode.INVALID_USE_OF_PROTECTED_MEMBER:
Expand Down
6 changes: 6 additions & 0 deletions pkg/analyzer/lib/dart/element/element.dart
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,9 @@ abstract class Element implements AnalysisTarget {
/// Whether the element has an annotation of the form `@doNotStore`.
bool get hasDoNotStore;

/// Whether the element has an annotation of the form `@doNotSubmit`.
bool get hasDoNotSubmit;

/// Whether the element has an annotation of the form `@factory`.
bool get hasFactory;

Expand Down Expand Up @@ -840,6 +843,9 @@ abstract class ElementAnnotation implements ConstantEvaluationTarget {
/// Whether the annotation marks the associated element as not to be stored.
bool get isDoNotStore;

/// Whether the annotation marks the associated member as not to be used.
bool get isDoNotSubmit;

/// Whether the annotation marks the associated member as a factory.
bool get isFactory;

Expand Down
22 changes: 22 additions & 0 deletions pkg/analyzer/lib/src/dart/element/element.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1511,6 +1511,10 @@ class ElementAnnotationImpl implements ElementAnnotation {
/// stored.
static const String _doNotStoreVariableName = 'doNotStore';

/// The name of the top-level variable used to mark a declaration as not to be
/// used (for ephemeral testing and debugging only).
static const String _doNotSubmitVariableName = 'doNotSubmit';

/// The name of the top-level variable used to mark a method as being a
/// factory.
static const String _factoryVariableName = 'factory';
Expand Down Expand Up @@ -1703,6 +1707,9 @@ class ElementAnnotationImpl implements ElementAnnotation {
@override
bool get isDoNotStore => _isPackageMetaGetter(_doNotStoreVariableName);

@override
bool get isDoNotSubmit => _isPackageMetaGetter(_doNotSubmitVariableName);

@override
bool get isFactory => _isPackageMetaGetter(_factoryVariableName);

Expand Down Expand Up @@ -2003,6 +2010,18 @@ abstract class ElementImpl implements Element {
return false;
}

@override
bool get hasDoNotSubmit {
final metadata = this.metadata;
for (var i = 0; i < metadata.length; i++) {
var annotation = metadata[i];
if (annotation.isDoNotSubmit) {
return true;
}
}
return false;
}

@override
bool get hasFactory {
final metadata = this.metadata;
Expand Down Expand Up @@ -5466,6 +5485,9 @@ class MultiplyDefinedElementImpl implements MultiplyDefinedElement {
@override
bool get hasDoNotStore => false;

@override
bool get hasDoNotSubmit => false;

@override
bool get hasFactory => false;

Expand Down
3 changes: 3 additions & 0 deletions pkg/analyzer/lib/src/dart/element/member.dart
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,9 @@ abstract class Member implements Element {
@override
bool get hasDoNotStore => _declaration.hasDoNotStore;

@override
bool get hasDoNotSubmit => _declaration.hasDoNotSubmit;

@override
bool get hasFactory => _declaration.hasFactory;

Expand Down
42 changes: 42 additions & 0 deletions pkg/analyzer/lib/src/error/best_practices_verifier.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1785,6 +1785,29 @@ class _InvalidAccessVerifier {
}

void _checkForOtherInvalidAccess(AstNode node, Element element) {
bool hasDoNotSubmit = _hasDoNotSubmit(element);
if (hasDoNotSubmit) {
// It's valid for a member annotated with `@doNotSubmit` to access another
// member annotated with `@doNotSubmit`. For example, this is valid:
// ```
// @doNotSubmit
// void foo() {}
//
// @doNotSubmit
// void bar() {
// // OK: `foo` is annotated with `@doNotSubmit` but so is `bar`.
// foo();
// }
// ```
var declaration = node.thisOrAncestorOfType<Declaration>();
if (declaration != null) {
var element = declaration.declaredElement;
if (element != null && _hasDoNotSubmit(element)) {
return;
}
}
}

var hasProtected = element.isProtected;
if (hasProtected) {
var definingClass = element.enclosingElement as InterfaceElement;
Expand Down Expand Up @@ -1844,6 +1867,14 @@ class _InvalidAccessVerifier {
if (definingClass == null) {
return;
}
if (hasDoNotSubmit) {
_errorReporter.atOffset(
offset: errorEntity.offset,
length: errorEntity.length,
errorCode: WarningCode.INVALID_USE_OF_DO_NOT_SUBMIT_MEMBER,
arguments: [name],
);
}
if (hasProtected) {
_errorReporter.atOffset(
offset: errorEntity.offset,
Expand Down Expand Up @@ -1892,6 +1923,17 @@ class _InvalidAccessVerifier {
}
}

bool _hasDoNotSubmit(Element element) {
if (element.hasDoNotSubmit) {
return true;
}
if (element is PropertyAccessorElement) {
var variable = element.variable2;
return variable != null && variable.hasDoNotSubmit;
}
return false;
}

bool _hasTypeOrSuperType(
InterfaceElement? element,
InterfaceElement superElement,
Expand Down
8 changes: 8 additions & 0 deletions pkg/analyzer/lib/src/error/codes.g.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6734,6 +6734,14 @@ class WarningCode extends AnalyzerErrorCode {
hasPublishedDocs: true,
);

/// Parameters:
/// 0: the name of the member
static const WarningCode INVALID_USE_OF_DO_NOT_SUBMIT_MEMBER = WarningCode(
'INVALID_USE_OF_DO_NOT_SUBMIT_MEMBER',
"Uses of '{0}' should not be submitted to source control.",
correctionMessage: "Try removing the reference to '{0}'.",
);

/// Parameters:
/// 0: the name of the member
static const WarningCode INVALID_USE_OF_INTERNAL_MEMBER = WarningCode(
Expand Down
1 change: 1 addition & 0 deletions pkg/analyzer/lib/src/error/error_code_values.g.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,7 @@ const List<ErrorCode> errorCodeValues = [
WarningCode.INVALID_REQUIRED_OPTIONAL_POSITIONAL_PARAM,
WarningCode.INVALID_REQUIRED_POSITIONAL_PARAM,
WarningCode.INVALID_SEALED_ANNOTATION,
WarningCode.INVALID_USE_OF_DO_NOT_SUBMIT_MEMBER,
WarningCode.INVALID_USE_OF_INTERNAL_MEMBER,
WarningCode.INVALID_USE_OF_PROTECTED_MEMBER,
WarningCode.INVALID_USE_OF_VISIBLE_FOR_OVERRIDING_MEMBER,
Expand Down
6 changes: 6 additions & 0 deletions pkg/analyzer/lib/src/test_utilities/mock_packages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ const _Checked checked = _Checked();
const _DoNotStore doNotStore = _DoNotStore();
const _DoNotSubmit doNotSubmit = _DoNotSubmit();
const _Experimental experimental = _Experimental();
const _Factory factory = _Factory();
Expand Down Expand Up @@ -200,6 +202,10 @@ class _DoNotStore {
const _DoNotStore();
}
class _DoNotSubmit {
const _DoNotSubmit();
}
class _Experimental {
const _Experimental();
}
Expand Down
57 changes: 57 additions & 0 deletions pkg/analyzer/messages.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24083,6 +24083,63 @@ WarningCode:

Parameters:
0: the name of the member
INVALID_USE_OF_DO_NOT_SUBMIT_MEMBER:
problemMessage: "Uses of '{0}' should not be submitted to source control."
correctionMessage: "Try removing the reference to '{0}'."
comment: |-
Parameters:
0: the name of the member
hasPublishedDocs: false
documentation: |-
#### Description

The analyzer produces this diagnostic when a member that is annotated with
[`@doNotSubmit`][meta-doNotSubmit] is referenced outside of a member
declaration that is also annotated with `@doNotSubmit`.

#### Example

Given a file `a.dart` containing the following declaration:

```dart
%uri="lib/a.dart"
import 'package:meta/meta.dart';

@doNotSubmit
void emulateCrash() { /* ... */ }
```

The following code produces this diagnostic because the declaration is
being referenced outside of a member that is also annotated with
`@doNotSubmit`:

```dart
import 'a.dart';

void f() {
[!emulateCrash!]();
}
```

#### Common fixes

Most commonly, when complete with local testing, the reference to the
member should be removed.

If building additional functionality on top of the member, annotate the
newly added member with `@doNotSubmit` as well:

```dart
import 'package:meta/meta.dart';

import 'a.dart';

@doNotSubmit
void emulateCrashWithOtherFunctionality() {
emulateCrash();
// do other things.
}
```
INVALID_USE_OF_INTERNAL_MEMBER:
problemMessage: "The member '{0}' can only be used within its package."
comment: |-
Expand Down
Loading

0 comments on commit a7c57c3

Please sign in to comment.