[M] Change in dart/sdk[main]: Fixed `@doNotSubmit` to prohibit same-library, allow nested functions.

36 views
Skip to first unread message

Samuel Rawlins (Gerrit)

unread,
Apr 24, 2024, 5:05:35 PM4/24/24
to Matan Lurey, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Brian Wilkerson and Matan Lurey

Samuel Rawlins added 3 comments

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Line 1803, Patchset 2 (Latest): // void a({@doNotSubmit int? b}) {
Samuel Rawlins . unresolved

This was probably just missed in the original review, but I don't think we document that parameters can have `@doNotSubmit`. It says [1]

Used to annotate a method, getter or top-level getter or function that is ...

[1]: https://github.com/dart-lang/sdk/blob/main/pkg/meta/lib/meta.dart

Line 1837, Patchset 2 (Latest): return;
Samuel Rawlins . unresolved

Thinking on this more, I suspect this can be much simpler. I could be missing something.

If you ever have an AstNode `node` with a staticElement that is a `ParameterElement`, aren't you definitely inside the function that declares that parameter (except for a single case, below)? I can't figure a way that you could be holding onto a reference to a ParameterElement from outside it's function.

For the named argument case on line 1815, that should be a NamedExpression (`b: 0`), with a Label with a `label`, a SimpleIdentifier, which might have a `staticElement` that is a ParameterElement. OK, maybe we just rule out this case: If you ever pass an argument for a `@doNotSubmit` named parameter, you've broken the rule. (Unless the containing function is so annotated, but that's ruled out on line 1796.) As for recursive calls... le sigh...

File pkg/analyzer/test/src/diagnostics/invalid_do_not_submit_test.dart
Line 320, Patchset 2 (Latest):void a({@doNotSubmit int? a}) {}
Samuel Rawlins . unresolved

Wait I think we missed a whole category of tests. What about a positional parameter so annotated?

```
void a([@doNotSubmit int? a]);

void b() => a(0);
```

We can choose to not act on this case, and just document it. And hopefully report such an annotation as invaild. Or we can act on it.

Open in Gerrit

Related details

Attention is currently required from:
  • Brian Wilkerson
  • Matan Lurey
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 2
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Matan Lurey <mat...@google.com>
Gerrit-Attention: Brian Wilkerson <brianwi...@google.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 21:05:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
unsatisfied_requirement
open
diffy

Brian Wilkerson (Gerrit)

unread,
Apr 24, 2024, 6:13:55 PM4/24/24
to Matan Lurey, Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Matan Lurey and Samuel Rawlins

Brian Wilkerson added 2 comments

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Samuel Rawlins . unresolved

Thinking on this more, I suspect this can be much simpler. I could be missing something.

If you ever have an AstNode `node` with a staticElement that is a `ParameterElement`, aren't you definitely inside the function that declares that parameter (except for a single case, below)? I can't figure a way that you could be holding onto a reference to a ParameterElement from outside it's function.

For the named argument case on line 1815, that should be a NamedExpression (`b: 0`), with a Label with a `label`, a SimpleIdentifier, which might have a `staticElement` that is a ParameterElement. OK, maybe we just rule out this case: If you ever pass an argument for a `@doNotSubmit` named parameter, you've broken the rule. (Unless the containing function is so annotated, but that's ruled out on line 1796.) As for recursive calls... le sigh...

Brian Wilkerson

If you ever have an AstNode node with a staticElement that is a ParameterElement, aren't you definitely inside the function that declares that parameter (except for a single case, below)?

Yes. That's probably my bad. Sorry. I didn't get enough context before responding.

The key, though, is that we don't care at all about references to an annotated parameter, we only care about passing an argument for such a parameter. (And it should be illegal to put the annotation on a required parameter.)

What you want isn't `node.staticElement`, which tells you what the node refers to, but `node.staticParameterElement`, which tells you which parameter corresponds to the `node` in the invocation. (Of course, that's assuming that the `node` is in an argument list.)

It would probably be easier to leave `_checkForInvalidDoNotSubmitAccess` for everything other than an annotated parameter and to have a new method to invoke from `visitArgumentList` that only deals with arguments to invocations.

File pkg/analyzer/test/src/diagnostics/invalid_do_not_submit_test.dart
Line 320, Patchset 2 (Latest):void a({@doNotSubmit int? a}) {}
Samuel Rawlins . unresolved

Wait I think we missed a whole category of tests. What about a positional parameter so annotated?

```
void a([@doNotSubmit int? a]);

void b() => a(0);
```

We can choose to not act on this case, and just document it. And hopefully report such an annotation as invaild. Or we can act on it.

Brian Wilkerson

The easiest implementation is to get the `staticParameterElement`, and if it's required then skip it, otherwise do all the necessary checking. That will cover the case above without any extra work (other than adding a test for it).

Open in Gerrit

Related details

Attention is currently required from:
  • Matan Lurey
  • Samuel Rawlins
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 2
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Matan Lurey <mat...@google.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 22:13:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Samuel Rawlins <sraw...@google.com>
unsatisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
May 2, 2024, 8:54:16 PM5/2/24
to Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Brian Wilkerson and Samuel Rawlins

Matan Lurey added 2 comments

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Line 1803, Patchset 2 (Latest): // void a({@doNotSubmit int? b}) {
Samuel Rawlins . resolved

This was probably just missed in the original review, but I don't think we document that parameters can have `@doNotSubmit`. It says [1]

Used to annotate a method, getter or top-level getter or function that is ...

[1]: https://github.com/dart-lang/sdk/blob/main/pkg/meta/lib/meta.dart

Matan Lurey

Good catch. Updated docs.

Samuel Rawlins . unresolved

Thinking on this more, I suspect this can be much simpler. I could be missing something.

If you ever have an AstNode `node` with a staticElement that is a `ParameterElement`, aren't you definitely inside the function that declares that parameter (except for a single case, below)? I can't figure a way that you could be holding onto a reference to a ParameterElement from outside it's function.

For the named argument case on line 1815, that should be a NamedExpression (`b: 0`), with a Label with a `label`, a SimpleIdentifier, which might have a `staticElement` that is a ParameterElement. OK, maybe we just rule out this case: If you ever pass an argument for a `@doNotSubmit` named parameter, you've broken the rule. (Unless the containing function is so annotated, but that's ruled out on line 1796.) As for recursive calls... le sigh...

Brian Wilkerson

If you ever have an AstNode node with a staticElement that is a ParameterElement, aren't you definitely inside the function that declares that parameter (except for a single case, below)?

Yes. That's probably my bad. Sorry. I didn't get enough context before responding.

The key, though, is that we don't care at all about references to an annotated parameter, we only care about passing an argument for such a parameter. (And it should be illegal to put the annotation on a required parameter.)

What you want isn't `node.staticElement`, which tells you what the node refers to, but `node.staticParameterElement`, which tells you which parameter corresponds to the `node` in the invocation. (Of course, that's assuming that the `node` is in an argument list.)

It would probably be easier to leave `_checkForInvalidDoNotSubmitAccess` for everything other than an annotated parameter and to have a new method to invoke from `visitArgumentList` that only deals with arguments to invocations.

Matan Lurey

Coming back to this after some time off, thanks for the feedback.

All of that makes sense to me, and I think I've incorporated your feedback - but if not please point out what I could do better. The only thing I have _not_ done is:

And it should be illegal to put the annotation on a required parameter

I am guessing that is probably out of scope of this particular PR, it would be nice to have a `TargetKind.optionalParameter`, but I think hand-writing something specific just for this annotation would go against the spirit. I am happy to open up a new issue/PR for that feature.

Open in Gerrit

Related details

Attention is currently required from:
  • Brian Wilkerson
  • Samuel Rawlins
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 2
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Brian Wilkerson <brianwi...@google.com>
Gerrit-Comment-Date: Fri, 03 May 2024 00:54:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Samuel Rawlins <sraw...@google.com>
Comment-In-Reply-To: Brian Wilkerson <brianwi...@google.com>
unsatisfied_requirement
open
diffy

Brian Wilkerson (Gerrit)

unread,
May 6, 2024, 11:30:35 AM5/6/24
to Matan Lurey, Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Matan Lurey and Samuel Rawlins

Brian Wilkerson added 1 comment

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Samuel Rawlins . unresolved

Thinking on this more, I suspect this can be much simpler. I could be missing something.

If you ever have an AstNode `node` with a staticElement that is a `ParameterElement`, aren't you definitely inside the function that declares that parameter (except for a single case, below)? I can't figure a way that you could be holding onto a reference to a ParameterElement from outside it's function.

For the named argument case on line 1815, that should be a NamedExpression (`b: 0`), with a Label with a `label`, a SimpleIdentifier, which might have a `staticElement` that is a ParameterElement. OK, maybe we just rule out this case: If you ever pass an argument for a `@doNotSubmit` named parameter, you've broken the rule. (Unless the containing function is so annotated, but that's ruled out on line 1796.) As for recursive calls... le sigh...

Brian Wilkerson

If you ever have an AstNode node with a staticElement that is a ParameterElement, aren't you definitely inside the function that declares that parameter (except for a single case, below)?

Yes. That's probably my bad. Sorry. I didn't get enough context before responding.

The key, though, is that we don't care at all about references to an annotated parameter, we only care about passing an argument for such a parameter. (And it should be illegal to put the annotation on a required parameter.)

What you want isn't `node.staticElement`, which tells you what the node refers to, but `node.staticParameterElement`, which tells you which parameter corresponds to the `node` in the invocation. (Of course, that's assuming that the `node` is in an argument list.)

It would probably be easier to leave `_checkForInvalidDoNotSubmitAccess` for everything other than an annotated parameter and to have a new method to invoke from `visitArgumentList` that only deals with arguments to invocations.

Matan Lurey

Coming back to this after some time off, thanks for the feedback.

All of that makes sense to me, and I think I've incorporated your feedback - but if not please point out what I could do better. The only thing I have _not_ done is:

And it should be illegal to put the annotation on a required parameter

I am guessing that is probably out of scope of this particular PR, it would be nice to have a `TargetKind.optionalParameter`, but I think hand-writing something specific just for this annotation would go against the spirit. I am happy to open up a new issue/PR for that feature.

Brian Wilkerson

The target kind was added in CL 364541.

I think you might have forgotten to upload the changes you made, as Gerrit is telling me that patch set 2, which is what I reviewed before, is still the latest.

Open in Gerrit

Related details

Attention is currently required from:
  • Matan Lurey
  • Samuel Rawlins
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 2
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Matan Lurey <mat...@google.com>
Gerrit-Comment-Date: Mon, 06 May 2024 15:30:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Samuel Rawlins <sraw...@google.com>
Comment-In-Reply-To: Matan Lurey <mat...@google.com>
Comment-In-Reply-To: Brian Wilkerson <brianwi...@google.com>
unsatisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
May 6, 2024, 1:15:15 PM5/6/24
to Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Brian Wilkerson and Samuel Rawlins

Matan Lurey added 1 comment

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Samuel Rawlins . unresolved

Thinking on this more, I suspect this can be much simpler. I could be missing something.

If you ever have an AstNode `node` with a staticElement that is a `ParameterElement`, aren't you definitely inside the function that declares that parameter (except for a single case, below)? I can't figure a way that you could be holding onto a reference to a ParameterElement from outside it's function.

For the named argument case on line 1815, that should be a NamedExpression (`b: 0`), with a Label with a `label`, a SimpleIdentifier, which might have a `staticElement` that is a ParameterElement. OK, maybe we just rule out this case: If you ever pass an argument for a `@doNotSubmit` named parameter, you've broken the rule. (Unless the containing function is so annotated, but that's ruled out on line 1796.) As for recursive calls... le sigh...

Brian Wilkerson

If you ever have an AstNode node with a staticElement that is a ParameterElement, aren't you definitely inside the function that declares that parameter (except for a single case, below)?

Yes. That's probably my bad. Sorry. I didn't get enough context before responding.

The key, though, is that we don't care at all about references to an annotated parameter, we only care about passing an argument for such a parameter. (And it should be illegal to put the annotation on a required parameter.)

What you want isn't `node.staticElement`, which tells you what the node refers to, but `node.staticParameterElement`, which tells you which parameter corresponds to the `node` in the invocation. (Of course, that's assuming that the `node` is in an argument list.)

It would probably be easier to leave `_checkForInvalidDoNotSubmitAccess` for everything other than an annotated parameter and to have a new method to invoke from `visitArgumentList` that only deals with arguments to invocations.

Matan Lurey

Coming back to this after some time off, thanks for the feedback.

All of that makes sense to me, and I think I've incorporated your feedback - but if not please point out what I could do better. The only thing I have _not_ done is:

And it should be illegal to put the annotation on a required parameter

I am guessing that is probably out of scope of this particular PR, it would be nice to have a `TargetKind.optionalParameter`, but I think hand-writing something specific just for this annotation would go against the spirit. I am happy to open up a new issue/PR for that feature.

Brian Wilkerson

The target kind was added in CL 364541.

I think you might have forgotten to upload the changes you made, as Gerrit is telling me that patch set 2, which is what I reviewed before, is still the latest.

Matan Lurey

Ah apologies, still re-learning `git cl`. Updated and also updated with `optionalParameter`.

Open in Gerrit

Related details

Attention is currently required from:
  • Brian Wilkerson
  • Samuel Rawlins
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 2
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Brian Wilkerson <brianwi...@google.com>
Gerrit-Comment-Date: Mon, 06 May 2024 17:15:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Samuel Rawlins <sraw...@google.com>
Comment-In-Reply-To: Brian Wilkerson <brianwi...@google.com>
Comment-In-Reply-To: Matan Lurey <mat...@google.com>
unsatisfied_requirement
open
diffy

Brian Wilkerson (Gerrit)

unread,
May 6, 2024, 2:36:29 PM5/6/24
to Matan Lurey, Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Matan Lurey and Samuel Rawlins

Brian Wilkerson added 4 comments

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Line 1944, Patchset 3 (Latest): var grandparent = parent?.parent;
Brian Wilkerson . unresolved

Not clear why we're recomputing `parent` on line 1940, nor `parent` here. Seems like neither of them (nor `node`) is re-assigned. If this is because one of them _is_ being reassigned (and I'm guessing that I'm just not seeing where), then ignore this comment.

Line 2100, Patchset 3 (Latest): 'Can only handle Identifier or PatternField, but got '
Brian Wilkerson . unresolved

nit: the code also handles `NamedType`. Consider 'Unhandled node type: ${...}' to avoid getting out of date as the list of handled cases is expanded (or doing nothing, which is what I always mean by 'nit').

File pkg/analyzer/test/src/diagnostics/invalid_do_not_submit_test.dart
Line 320, Patchset 2:void a({@doNotSubmit int? a}) {}
Samuel Rawlins . unresolved

Wait I think we missed a whole category of tests. What about a positional parameter so annotated?

```
void a([@doNotSubmit int? a]);

void b() => a(0);
```

We can choose to not act on this case, and just document it. And hopefully report such an annotation as invaild. Or we can act on it.

Brian Wilkerson

The easiest implementation is to get the `staticParameterElement`, and if it's required then skip it, otherwise do all the necessary checking. That will cover the case above without any extra work (other than adding a test for it).

Brian Wilkerson

It would be good to have a test for an optional positional parameter.

File pkg/meta/lib/meta.dart
Line 93, Patchset 3 (Latest):/// Used to annotate a parameter, method, getter or top-level getter or function
Brian Wilkerson . unresolved

Consider "a parameter" --> "an optional parameter".

Open in Gerrit

Related details

Attention is currently required from:
  • Matan Lurey
  • Samuel Rawlins
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 3
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Matan Lurey <mat...@google.com>
Gerrit-Comment-Date: Mon, 06 May 2024 18:36:24 +0000
unsatisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
May 6, 2024, 3:03:54 PM5/6/24
to Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Brian Wilkerson and Samuel Rawlins

Matan Lurey added 5 comments

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Samuel Rawlins . resolved

Thinking on this more, I suspect this can be much simpler. I could be missing something.

If you ever have an AstNode `node` with a staticElement that is a `ParameterElement`, aren't you definitely inside the function that declares that parameter (except for a single case, below)? I can't figure a way that you could be holding onto a reference to a ParameterElement from outside it's function.

For the named argument case on line 1815, that should be a NamedExpression (`b: 0`), with a Label with a `label`, a SimpleIdentifier, which might have a `staticElement` that is a ParameterElement. OK, maybe we just rule out this case: If you ever pass an argument for a `@doNotSubmit` named parameter, you've broken the rule. (Unless the containing function is so annotated, but that's ruled out on line 1796.) As for recursive calls... le sigh...

Brian Wilkerson

If you ever have an AstNode node with a staticElement that is a ParameterElement, aren't you definitely inside the function that declares that parameter (except for a single case, below)?

Yes. That's probably my bad. Sorry. I didn't get enough context before responding.

The key, though, is that we don't care at all about references to an annotated parameter, we only care about passing an argument for such a parameter. (And it should be illegal to put the annotation on a required parameter.)

What you want isn't `node.staticElement`, which tells you what the node refers to, but `node.staticParameterElement`, which tells you which parameter corresponds to the `node` in the invocation. (Of course, that's assuming that the `node` is in an argument list.)

It would probably be easier to leave `_checkForInvalidDoNotSubmitAccess` for everything other than an annotated parameter and to have a new method to invoke from `visitArgumentList` that only deals with arguments to invocations.

Matan Lurey

Coming back to this after some time off, thanks for the feedback.

All of that makes sense to me, and I think I've incorporated your feedback - but if not please point out what I could do better. The only thing I have _not_ done is:

And it should be illegal to put the annotation on a required parameter

I am guessing that is probably out of scope of this particular PR, it would be nice to have a `TargetKind.optionalParameter`, but I think hand-writing something specific just for this annotation would go against the spirit. I am happy to open up a new issue/PR for that feature.

Brian Wilkerson

The target kind was added in CL 364541.

I think you might have forgotten to upload the changes you made, as Gerrit is telling me that patch set 2, which is what I reviewed before, is still the latest.

Matan Lurey

Ah apologies, still re-learning `git cl`. Updated and also updated with `optionalParameter`.

Matan Lurey

Done

Line 1944, Patchset 3: var grandparent = parent?.parent;
Brian Wilkerson . unresolved

Not clear why we're recomputing `parent` on line 1940, nor `parent` here. Seems like neither of them (nor `node`) is re-assigned. If this is because one of them _is_ being reassigned (and I'm guessing that I'm just not seeing where), then ignore this comment.

Matan Lurey

Before when the code that is now in `_getIdentifierNameAndErrorEntity` was inlined in this function (it had to get moved out to be re-used effectively), it was just re-used from what is in context.

This particular code branch, at the latest commit, does not have any other (local) declaration of `parent` (or `grandParent`). I _could_ change `_getIdentifierNameAndErrorEntity` to return `parent` and `grandParent` as well, but I am not sure if saving on a single `?.` lookup is worth the complexity/indirection.

Line 2100, Patchset 3: 'Can only handle Identifier or PatternField, but got '
Brian Wilkerson . resolved

nit: the code also handles `NamedType`. Consider 'Unhandled node type: ${...}' to avoid getting out of date as the list of handled cases is expanded (or doing nothing, which is what I always mean by 'nit').

Matan Lurey

Done.

File pkg/analyzer/test/src/diagnostics/invalid_do_not_submit_test.dart
Line 320, Patchset 2:void a({@doNotSubmit int? a}) {}
Samuel Rawlins . resolved

Wait I think we missed a whole category of tests. What about a positional parameter so annotated?

```
void a([@doNotSubmit int? a]);

void b() => a(0);
```

We can choose to not act on this case, and just document it. And hopefully report such an annotation as invaild. Or we can act on it.

Brian Wilkerson

The easiest implementation is to get the `staticParameterElement`, and if it's required then skip it, otherwise do all the necessary checking. That will cover the case above without any extra work (other than adding a test for it).

Brian Wilkerson

It would be good to have a test for an optional positional parameter.

Matan Lurey

Done

File pkg/meta/lib/meta.dart
Line 93, Patchset 3:/// Used to annotate a parameter, method, getter or top-level getter or function
Brian Wilkerson . resolved

Consider "a parameter" --> "an optional parameter".

Matan Lurey

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Brian Wilkerson
  • Samuel Rawlins
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 5
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Brian Wilkerson <brianwi...@google.com>
Gerrit-Comment-Date: Mon, 06 May 2024 19:03:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Samuel Rawlins <sraw...@google.com>
unsatisfied_requirement
open
diffy

Brian Wilkerson (Gerrit)

unread,
May 6, 2024, 4:53:50 PM5/6/24
to Matan Lurey, Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Matan Lurey and Samuel Rawlins

Brian Wilkerson voted and added 1 comment

Votes added by Brian Wilkerson

Code-Review+1

1 comment

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Line 1944, Patchset 3: var grandparent = parent?.parent;
Brian Wilkerson . resolved

Not clear why we're recomputing `parent` on line 1940, nor `parent` here. Seems like neither of them (nor `node`) is re-assigned. If this is because one of them _is_ being reassigned (and I'm guessing that I'm just not seeing where), then ignore this comment.

Matan Lurey

Before when the code that is now in `_getIdentifierNameAndErrorEntity` was inlined in this function (it had to get moved out to be re-used effectively), it was just re-used from what is in context.

This particular code branch, at the latest commit, does not have any other (local) declaration of `parent` (or `grandParent`). I _could_ change `_getIdentifierNameAndErrorEntity` to return `parent` and `grandParent` as well, but I am not sure if saving on a single `?.` lookup is worth the complexity/indirection.

Brian Wilkerson

Given that, I wouldn't change it either.

Open in Gerrit

Related details

Attention is currently required from:
  • Matan Lurey
  • Samuel Rawlins
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 5
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Matan Lurey <mat...@google.com>
Gerrit-Comment-Date: Mon, 06 May 2024 20:53:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
May 6, 2024, 5:03:07 PM5/6/24
to Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Samuel Rawlins

Matan Lurey voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention is currently required from:
  • Samuel Rawlins
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 5
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Comment-Date: Mon, 06 May 2024 21:03:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
May 6, 2024, 7:25:21 PM5/6/24
to Commit Queue, Brian Wilkerson, Samuel Rawlins, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Samuel Rawlins

Matan Lurey added 1 comment

File pkg/analyzer/lib/src/error/best_practices_verifier.dart
Line 1944, Patchset 3: var grandparent = parent?.parent;
Brian Wilkerson . resolved

Not clear why we're recomputing `parent` on line 1940, nor `parent` here. Seems like neither of them (nor `node`) is re-assigned. If this is because one of them _is_ being reassigned (and I'm guessing that I'm just not seeing where), then ignore this comment.

Matan Lurey

Before when the code that is now in `_getIdentifierNameAndErrorEntity` was inlined in this function (it had to get moved out to be re-used effectively), it was just re-used from what is in context.

This particular code branch, at the latest commit, does not have any other (local) declaration of `parent` (or `grandParent`). I _could_ change `_getIdentifierNameAndErrorEntity` to return `parent` and `grandParent` as well, but I am not sure if saving on a single `?.` lookup is worth the complexity/indirection.

Matan Lurey

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Samuel Rawlins
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 5
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Samuel Rawlins <sraw...@google.com>
Gerrit-Comment-Date: Mon, 06 May 2024 23:25:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
open
diffy

Samuel Rawlins (Gerrit)

unread,
May 7, 2024, 12:49:31 PM5/7/24
to Matan Lurey, Commit Queue, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Matan Lurey

Samuel Rawlins voted and added 1 comment

Votes added by Samuel Rawlins

Code-Review+1

1 comment

Patchset-level comments
File-level comment, Patchset 5 (Latest):
Samuel Rawlins . resolved

This is great! I think this a very complete impl for handling annotated parameters.

Open in Gerrit

Related details

Attention is currently required from:
  • Matan Lurey
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 5
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Attention: Matan Lurey <mat...@google.com>
Gerrit-Comment-Date: Tue, 07 May 2024 16:49:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
May 9, 2024, 4:58:08 PM5/9/24
to Samuel Rawlins, Commit Queue, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org

Matan Lurey voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 5
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Samuel Rawlins <sraw...@google.com>
Gerrit-Comment-Date: Thu, 09 May 2024 20:58:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
Jun 28, 2024, 8:27:24 PM6/28/24
to Sam Rawlins, Commit Queue, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org

Matan Lurey voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 6
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Sam Rawlins <sraw...@google.com>
Gerrit-Comment-Date: Sat, 29 Jun 2024 00:27:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Matan Lurey (Gerrit)

unread,
Jul 1, 2024, 7:37:45 PM7/1/24
to Sam Rawlins, Commit Queue, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org

Matan Lurey voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 7
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
Gerrit-Reviewer: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Sam Rawlins <sraw...@google.com>
Gerrit-Comment-Date: Mon, 01 Jul 2024 23:37:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Commit Queue (Gerrit)

unread,
Jul 1, 2024, 8:10:35 PM7/1/24
to Matan Lurey, Sam Rawlins, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org

Commit Queue submitted the change with unreviewed changes

Unreviewed changes

5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: pkg/meta/CHANGELOG.md
Insertions: 51, Deletions: 0.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:
Fixed `@doNotSubmit` to prohibit same-library, allow nested functions.
Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Commit-Queue: Matan Lurey <mat...@google.com>
Reviewed-by: Brian Wilkerson <brianwi...@google.com>
Reviewed-by: Sam Rawlins <sraw...@google.com>
Files:
  • M pkg/analyzer/lib/src/error/best_practices_verifier.dart
  • M pkg/analyzer/test/src/diagnostics/invalid_do_not_submit_test.dart
  • M pkg/meta/CHANGELOG.md
  • M pkg/meta/lib/meta.dart
Change size: L
Delta: 4 files changed, 293 insertions(+), 67 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Brian Wilkerson, +1 by Sam Rawlins
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: Ib0aa786e81e6fc9fecc8af7a3edbbd541cabcaad
Gerrit-Change-Number: 364382
Gerrit-PatchSet: 8
Gerrit-Owner: Matan Lurey <mat...@google.com>
Gerrit-Reviewer: Brian Wilkerson <brianwi...@google.com>
open
diffy
satisfied_requirement
Reply all
Reply to author
Forward
0 new messages