[M] Change in dart/sdk[main]: [analyzer] Fixes `initializing_formals` false-positive
Felipe Morschel (Gerrit)
Felipe Morschel has uploaded the change for review![]( "Open in Gerrit")
Commit message
[analyzer] Fixes `initializing_formals` false-positive
Change information
- M pkg/analyzer/lib/src/dart/analysis/analysis_context_collection.dart
- M pkg/linter/lib/src/rules/prefer_initializing_formals.dart
- M pkg/linter/test/rules/prefer_initializing_formals_test.dart
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
void processExpression(Expression expression) {At this point I wondered if making a small visitor only for assignment expressions wasn't best since I've not handled assignment expressions inside `if` and so many more cases...
I'll leave it up for you to decide if it is worth it.
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
void processExpression(Expression expression) {At this point I wondered if making a small visitor only for assignment expressions wasn't best since I've not handled assignment expressions inside `if` and so many more cases...
I'll leave it up for you to decide if it is worth it.
Yeah I think an actual AstVisitor would be better, and also quite idiomatic. If the question is "does this thing ever happen anywhere inside this method body (or expression, I suppose)", then an AstVisitor is the way to go, and should remain very simple.
The only caution you might want to take is to only visit a function body once, tracking all of the variables that are ever assigned. Just for performance reasons.
Related details
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Samuel RawlinsAt this point I wondered if making a small visitor only for assignment expressions wasn't best since I've not handled assignment expressions inside `if` and so many more cases...
I'll leave it up for you to decide if it is worth it.
Yeah I think an actual AstVisitor would be better, and also quite idiomatic. If the question is "does this thing ever happen anywhere inside this method body (or expression, I suppose)", then an AstVisitor is the way to go, and should remain very simple.
The only caution you might want to take is to only visit a function body once, tracking all of the variables that are ever assigned. Just for performance reasons.
I think the RecursiveAstVisitor should be fine for this. Much cleaner too.
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 2 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
2 comments
List<AssignmentExpression> assignments = [];nit: can be final.
static List<AssignmentExpression> assignmentsBellow(FunctionBody body) {nit: spelling: 'Below'
Related details
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
List<AssignmentExpression> assignments = [];Felipe Morschelnit: can be final.
Done
static List<AssignmentExpression> assignmentsBellow(FunctionBody body) {Felipe Morschelnit: spelling: 'Below'
Thanks for catching that!
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
static List<AssignmentExpression> assignmentsBellow(FunctionBody body) {Konstantin Shcheglovnit: spelling: 'Below'
Sorry, below what?
Related details
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
static List<AssignmentExpression> assignmentsBellow(FunctionBody body) {Konstantin Shcheglovnit: spelling: 'Below'
Sorry, below what?
Below the given body node. Maybe the naming is incorrect?
I meant `If any children (grandchildren, etc) of the given body is an assignment expression, it will be added to the list and returned`.
Should I replace the wording? Prehaps add docs?
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Konstantin Shcheglov
| Code-Review | +1 |
1 comment
static List<AssignmentExpression> assignmentsBellow(FunctionBody body) {Konstantin Shcheglovnit: spelling: 'Below'
Felipe MorschelSorry, below what?
Below the given body node. Maybe the naming is incorrect?
I meant `If any children (grandchildren, etc) of the given body is an assignment expression, it will be added to the list and returned`.
Should I replace the wording? Prehaps add docs?
OK, now I understand.
It is "below" in a tree meaning.
I'd use "in" word, but as it is, it is OK too.
Related details
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
static List<AssignmentExpression> assignmentsBellow(FunctionBody body) {Konstantin Shcheglovnit: spelling: 'Below'
Felipe MorschelSorry, below what?
Konstantin ShcheglovBelow the given body node. Maybe the naming is incorrect?
I meant `If any children (grandchildren, etc) of the given body is an assignment expression, it will be added to the list and returned`.
Should I replace the wording? Prehaps add docs?
OK, now I understand.
It is "below" in a tree meaning.
I'd use "in" word, but as it is, it is OK too.
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted Code-Review+1![]( "Open in Gerrit")
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
if (usedParameters.isNotEmpty) {Without this part, the new `test_withConditionalAssignment` fils and without the `return` at the end (or else) then `test_assignedParameter_if` would break. I think this is good but I'd love your opinions on this.
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
if (usedParameters.isNotEmpty) {Without this part, the new `test_withConditionalAssignment` fils and without the `return` at the end (or else) then `test_assignedParameter_if` would break. I think this is good but I'd love your opinions on this.
and without the `return` at the end (or else) then `test_assignedParameter_if` would break.
I'm confused about this bit. Are you saying that the code _might_ be concerning because it requires an early return if `usedParameters.isNotEmpty`? Because then we don't call `super.visitIfStatement`? That is strange. Why does `test_assignedParameter_if` fail if you don't `return` (and thus call `super.visitIfStatement`)?
Related details
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
if (usedParameters.isNotEmpty) {Samuel RawlinsWithout this part, the new `test_withConditionalAssignment` fils and without the `return` at the end (or else) then `test_assignedParameter_if` would break. I think this is good but I'd love your opinions on this.
and without the `return` at the end (or else) then `test_assignedParameter_if` would break.
I'm confused about this bit. Are you saying that the code _might_ be concerning because it requires an early return if `usedParameters.isNotEmpty`? Because then we don't call `super.visitIfStatement`? That is strange. Why does `test_assignedParameter_if` fail if you don't `return` (and thus call `super.visitIfStatement`)?
Sorry I wrote down the other way around.
Without `return` (or else) then `test_withConditionalAssignment` would break because it would be looking for inner assignments normally without considering that it could have been used in a condition.
Then if we _don't_ use the super call for the "`else`" case, `test_assignedParameter_if` would break.
I would also like to ask about other conditions like `for`, `while`, `switch` guards... Maybe we could handle all but I'm not sure.
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
if (usedParameters.isNotEmpty) {Samuel RawlinsWithout this part, the new `test_withConditionalAssignment` fils and without the `return` at the end (or else) then `test_assignedParameter_if` would break. I think this is good but I'd love your opinions on this.
Felipe Morscheland without the `return` at the end (or else) then `test_assignedParameter_if` would break.
I'm confused about this bit. Are you saying that the code _might_ be concerning because it requires an early return if `usedParameters.isNotEmpty`? Because then we don't call `super.visitIfStatement`? That is strange. Why does `test_assignedParameter_if` fail if you don't `return` (and thus call `super.visitIfStatement`)?
Sorry I wrote down the other way around.
Without `return` (or else) then `test_withConditionalAssignment` would break because it would be looking for inner assignments normally without considering that it could have been used in a condition.
Then if we _don't_ use the super call for the "`else`" case, `test_assignedParameter_if` would break.
I would also like to ask about other conditions like `for`, `while`, `switch` guards... Maybe we could handle all but I'm not sure.
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
if (usedParameters.isNotEmpty) {Samuel RawlinsWithout this part, the new `test_withConditionalAssignment` fils and without the `return` at the end (or else) then `test_assignedParameter_if` would break. I think this is good but I'd love your opinions on this.
Felipe Morscheland without the `return` at the end (or else) then `test_assignedParameter_if` would break.
I'm confused about this bit. Are you saying that the code _might_ be concerning because it requires an early return if `usedParameters.isNotEmpty`? Because then we don't call `super.visitIfStatement`? That is strange. Why does `test_assignedParameter_if` fail if you don't `return` (and thus call `super.visitIfStatement`)?
Felipe MorschelSorry I wrote down the other way around.
Without `return` (or else) then `test_withConditionalAssignment` would break because it would be looking for inner assignments normally without considering that it could have been used in a condition.
Then if we _don't_ use the super call for the "`else`" case, `test_assignedParameter_if` would break.
I would also like to ask about other conditions like `for`, `while`, `switch` guards... Maybe we could handle all but I'm not sure.
Friendly ping @sraw...@google.com
Sorry, I'm working on something a bit pressing this week; hopefully I can review at the end of the week.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
if (usedParameters.isNotEmpty) {Samuel RawlinsWithout this part, the new `test_withConditionalAssignment` fils and without the `return` at the end (or else) then `test_assignedParameter_if` would break. I think this is good but I'd love your opinions on this.
Felipe Morscheland without the `return` at the end (or else) then `test_assignedParameter_if` would break.
I'm confused about this bit. Are you saying that the code _might_ be concerning because it requires an early return if `usedParameters.isNotEmpty`? Because then we don't call `super.visitIfStatement`? That is strange. Why does `test_assignedParameter_if` fail if you don't `return` (and thus call `super.visitIfStatement`)?
Felipe MorschelSorry I wrote down the other way around.
Without `return` (or else) then `test_withConditionalAssignment` would break because it would be looking for inner assignments normally without considering that it could have been used in a condition.
Then if we _don't_ use the super call for the "`else`" case, `test_assignedParameter_if` would break.
I would also like to ask about other conditions like `for`, `while`, `switch` guards... Maybe we could handle all but I'm not sure.
Samuel RawlinsFriendly ping @sraw...@google.com
Sorry, I'm working on something a bit pressing this week; hopefully I can review at the end of the week.
OK I see. This shouldn't be too crazy. I think all we need to do is store a stack of used parameters on this visitor. And store something like a "am I in a condition" bool. I think this `visitIfStatement` would look something like:
```
visitIfStatement(IfStatement node) {
var previousInCondition = inCondition;
inBool = true;
// push empty list of used parameters.
node.condition.accept(this);
inCondition = previousInCondition;
node.then.accept(this);
node.else.accept(this);
// pop empty list of used parameters.
}
```
and that would be used in `visitSimpleIdentifier`:
```dart
void visitSimpleIdentifier(SimpleIdentifier node) {
if (node.element is in parameters && inCondition) {
// add node.element to usedParameters
}
super.visitSimpleIdentifier();
}
```
and `visitAssignmentExpression` would be something like:
```dart
visitAssignmentExpression(AssignmentExpression node) {
// if node's assignee is in the used parameters list, then report!
super.visitAssignmentExpression(node);
}
```
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
You do need to be careful about the case where the visitor reaches a nested closure (function body). Then you need to reset `isBool` to `false` and restore it's previous value when you leave the closure.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
I think the changes I made are complete. I'm going to mark this as resolved and if you think about something else we can discuss in more specific comments. Thanks for the help!
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 7 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
7 comments
final assignments = <AssignmentExpression>[];This is cool; this is now a very robust check with this visitor. And much more elegant than I was picturing.
Since this visitor is non-trivial, we should now document the fields here, since they change a lot. And we should probably write a few sentences in a comment on the class itself explaining specifically how `_processConditions` works with `visitAssignmentExpression` and `visitSimpleIdentifier`.
var closure = false;I think this should be called `inClosure`, to be consistent.
inCondition) {Let's make this the first condition to check. It's false any time where not, well, in a condition, so it should usually be false, letting us short-circuit.
test_assignedInClosure() async {I think we need to track 'inClosure' again keeping the previous value. For example, here's a good test:
```
class C {
int? x;
C(int x) {
foo(() {
foo(() {this.x = x;});
// Oops, 'inClosure' better still be true.
this.x = x;
});
}
}
```
test_withConditionalAssignment_for() async {With the addition of handling loops (for, while, and do-while), I think we actually don't even need to check the condition. I feel like any assignment to `this.x` in a loop means we should not report lint. In your case here, you have a break, but that seems like the exception, the standard might be more like:
```
class C {
late List<String> x;
C(List<String> x) {
for (var e in ['a', 'b', 'c']) {
x.add(e);
this.x = x;
}
}
}
```
WDYT?
test_withConditionalAssignment_while() async {Love these new tests; I don't see an example with `do-while` though.
Related details
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
test_withConditionalAssignment_while() async {Love these new tests; I don't see an example with `do-while` though.
I wasn't sure if it would add any value. Since it would simply "do" first. Should I add it anyway? I wouldn't expect it to break ever but who knows.
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 5 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
5 comments
This is cool; this is now a very robust check with this visitor. And much more elegant than I was picturing.
Since this visitor is non-trivial, we should now document the fields here, since they change a lot. And we should probably write a few sentences in a comment on the class itself explaining specifically how `_processConditions` works with `visitAssignmentExpression` and `visitSimpleIdentifier`.
Done
I think this should be called `inClosure`, to be consistent.
Done
Let's make this the first condition to check. It's false any time where not, well, in a condition, so it should usually be false, letting us short-circuit.
Great idea!
I think we need to track 'inClosure' again keeping the previous value. For example, here's a good test:
```
class C {
int? x;
C(int x) {
foo(() {
foo(() {this.x = x;});
// Oops, 'inClosure' better still be true.
this.x = x;
});
}
}
```
Great cath! I was going to miss this case.
test_withConditionalAssignment_for() async {With the addition of handling loops (for, while, and do-while), I think we actually don't even need to check the condition. I feel like any assignment to `this.x` in a loop means we should not report lint. In your case here, you have a break, but that seems like the exception, the standard might be more like:
```
class C {
late List<String> x;
C(List<String> x) {
for (var e in ['a', 'b', 'c']) {
x.add(e);
this.x = x;
}
}
}
```WDYT?
Any case? Or only when we "use" `x` like accessing `add` and such? Because if we don't alter the value of the parameter, we could probably make it an initializing formal parameter. I've made the changes considering this, any objections?
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 2 comments![]( "Open in Gerrit")
test_withConditionalAssignment_for() async {Felipe MorschelWith the addition of handling loops (for, while, and do-while), I think we actually don't even need to check the condition. I feel like any assignment to `this.x` in a loop means we should not report lint. In your case here, you have a break, but that seems like the exception, the standard might be more like:
```
class C {
late List<String> x;
C(List<String> x) {
for (var e in ['a', 'b', 'c']) {
x.add(e);
this.x = x;
}
}
}
```WDYT?
Any case? Or only when we "use" `x` like accessing `add` and such? Because if we don't alter the value of the parameter, we could probably make it an initializing formal parameter. I've made the changes considering this, any objections?
It's so far-fetched... these loops and such. It really probably doesn't matter what we do here. But I like your idea of using the data of whether x is "used".
test_withConditionalAssignment_while() async {Love these new tests; I don't see an example with `do-while` though.
I wasn't sure if it would add any value. Since it would simply "do" first. Should I add it anyway? I wouldn't expect it to break ever but who knows.
Here, I'm thinking again of assigning `this.x` in a loop. Seems like any situation like this is not a situation where we should report this lint. Including a do-while.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
test_withConditionalAssignment_while() async {Felipe MorschelLove these new tests; I don't see an example with `do-while` though.
Samuel RawlinsI wasn't sure if it would add any value. Since it would simply "do" first. Should I add it anyway? I wouldn't expect it to break ever but who knows.
Here, I'm thinking again of assigning `this.x` in a loop. Seems like any situation like this is not a situation where we should report this lint. Including a do-while.
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
1 comment
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
That bot is basically reporting timeouts. I'm looking for an infinite loop...
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
super.visitIfStatement(node);I think the timeout in the g3 bot is due to this double visiting: `_newScopeForUsedParameters` takes each node it is handed, and calls `node.accept(this)`. _Then_ `super.visitIfStatement(node)` visits the children again. That sort of represents just a 2x in visits, and maybe does not represent a functional incorrectness. But these visits are recursive. So if you have 4 if-statements nested, then we visit the children of the outermost if-statement 2 times, and children of the next if-statement 4 times; we visit the next if-statement's children 8 times, and the last one's children 16 times.
I think each function which calls `_newScopeForUsedParameters` should not call `super.visit...`. It needs to carefully have specific children accept this visitor.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
I think the timeout in the g3 bot is due to this double visiting: `_newScopeForUsedParameters` takes each node it is handed, and calls `node.accept(this)`. _Then_ `super.visitIfStatement(node)` visits the children again. That sort of represents just a 2x in visits, and maybe does not represent a functional incorrectness. But these visits are recursive. So if you have 4 if-statements nested, then we visit the children of the outermost if-statement 2 times, and children of the next if-statement 4 times; we visit the next if-statement's children 8 times, and the last one's children 16 times.
I think each function which calls `_newScopeForUsedParameters` should not call `super.visit...`. It needs to carefully have specific children accept this visitor.
I've did some refactoring. Mind running it again for me please? I can close the other comment if it passes. Thanks!
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
1 comment
_newScopeForUsedParameters([?condition, node.body]);This one's going to be tricky to get perfectly right, I think. You definitely still want to visit all children, one way or another. In this case, you want to make sure to visit the other for-loop parts. An example would be _very_ contrived. But you know, there could be something like `for (var e in a.map((x) => x == parameter))`.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
Samuel Rawlins@sraw...@google.com I can't see the failure for the g3 bot.
That bot is basically reporting timeouts. I'm looking for an infinite loop...
It seems like it still had errors. Not sure if it is the same thing? I wasn't expecting it to be failing.
This one's going to be tricky to get perfectly right, I think. You definitely still want to visit all children, one way or another. In this case, you want to make sure to visit the other for-loop parts. An example would be _very_ contrived. But you know, there could be something like `for (var e in a.map((x) => x == parameter))`.
I think the implementation is better now. Thanks for the example!
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |