// void a({@doNotSubmit int? b}) {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
return;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...
void a({@doNotSubmit int? a}) {}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.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return;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...
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.
void a({@doNotSubmit int? a}) {}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.
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).
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// void a({@doNotSubmit int? b}) {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
Good catch. Updated docs.
return;Brian WilkersonThinking 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...
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.
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.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return;Brian WilkersonThinking 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...
Matan LureyIf 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.
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.
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.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return;Brian WilkersonThinking 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...
Matan LureyIf 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.
Brian WilkersonComing 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.
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.
Ah apologies, still re-learning `git cl`. Updated and also updated with `optionalParameter`.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
var grandparent = parent?.parent;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.
'Can only handle Identifier or PatternField, but got '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').
void a({@doNotSubmit int? a}) {}Brian WilkersonWait 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.
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).
It would be good to have a test for an optional positional parameter.
/// Used to annotate a parameter, method, getter or top-level getter or functionConsider "a parameter" --> "an optional parameter".
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return;Brian WilkersonThinking 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...
Matan LureyIf 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.
Brian WilkersonComing 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.
Matan LureyThe 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.
Ah apologies, still re-learning `git cl`. Updated and also updated with `optionalParameter`.
Done
var grandparent = parent?.parent;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.
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.
'Can only handle Identifier or PatternField, but got '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').
Done.
void a({@doNotSubmit int? a}) {}Brian WilkersonWait 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 WilkersonThe 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).
It would be good to have a test for an optional positional parameter.
Done
/// Used to annotate a parameter, method, getter or top-level getter or functionConsider "a parameter" --> "an optional parameter".
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
var grandparent = parent?.parent;Matan LureyNot 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.
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.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
var grandparent = parent?.parent;Matan LureyNot 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.
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.
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
This is great! I think this a very complete impl for handling annotated parameters.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +2 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +2 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
```
Fixed `@doNotSubmit` to prohibit same-library, allow nested functions.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |