feat: fix false negatives in no-this-before-super
#17762
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Tanujkanti4441
added
rule
lib/rules/no-this-before-super.js
Outdated
| /* | ||
| * Traverse the segments in order starting from the first segment | ||
| * and check for `this` used before `super()`. | ||
| */ | ||
| while (segmentStack.length > 0) { |
There was a problem hiding this comment.
Should we maybe try to fix CodePath#traverseSegments() instead of reimplementing it here?
There was a problem hiding this comment.
I changed this PR to fix traverseSegments 👍
mdjermanovic
added
accepted
| class A extends B { | ||
| constructor() { | ||
| while (foo) { | ||
| this.a(); | ||
| super(); | ||
| } | ||
| } | ||
| }`, |
There was a problem hiding this comment.
This was another, unrelated false negative, caused by how onCodePathSegmentLoop was implemented in this rule?
There was a problem hiding this comment.
It seems you're right. This false negative does not seem to be related to the traverseSegments bug. Should I do a different PR?
There was a problem hiding this comment.
I think we can include it in this PR, just wanted to make sure I understood the problem correctly.
mdjermanovic
changed the title
no-this-before-superno-this-before-super
| visited.add(segment); | ||
|
|
||
|
|
||
| // Skips the segment if itself or all previous segments have been skipped. | ||
| const skip = skipped.has(segment) || |
There was a problem hiding this comment.
Can skipped.has(segment) ever be true at this point?
| @@ -59,6 +60,7 @@ function getOrderOfTraversing(codePath, options, callback) { | |||
| callback(segment, controller); // eslint-disable-line n/callback-return -- At end of inner function | |||
| } | |||
| }); | |||
| debug.dumpDot(codePath); | |||
There was a problem hiding this comment.
Was this intentionally committed or maybe left over from debugging?
| skipped.size > 0 && | ||
| segment.prevSegments.length > 0 && | ||
| segment.prevSegments.every(prevSegment => skipped.has(prevSegment)) |
There was a problem hiding this comment.
I'm not sure if this logic works well when there are loops afterwards. For example:
const codePath = parseCodePaths("a; while (b) { c; }")[0];
const order = getOrderOfTraversing(codePath, null, (segment, controller) => {
if (segment.id === "s1_1") {
controller.skip();
}
});
assert.deepStrictEqual(order, ["s1_1"]); // actual result is ["s1_1", "s1_2", "s1_3", "s1_4"] but it should be just ["s1_1"]?There was a problem hiding this comment.
Thank you for the review and finding the bug!
I fixed the issues.
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
|
@ota-meshi are you still working on this? |
|
I would like to work on this PR. But right now, I'm prioritizing other PR. |
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
ota-meshi
force-pushed
the
fix-no-this-before-super
branch
from
6c3dcb4
to
454285b
Compare
| visited.add(segment); | ||
|
|
||
|
|
||
| // Skips the segment if all previous segments have been skipped. | ||
| const skip = ( |
There was a problem hiding this comment.
Maybe shouldSkip would be clearer for this variable name?
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Fixes #17605
This PR fixes some false negatives in the no-this-before-super rule.
Is there anything you'd like reviewers to focus on?