Repository navigation
feat(analyze/js): implement noShadow - #5761
Merged
Merged
Conversation
github-actions
Bot
added
A-Linter
dyc3
marked this pull request as ready for review
April 24, 2025 15:45
Contributor
CodSpeed Performance ReportMerging #5761 will not alter performanceComparing Summary
|
This comment was marked as resolved.
This comment was marked as resolved.
dyc3
force-pushed
the
dyc3/no-shadow
branch
2 times, most recently
from
April 25, 2025 19:23
37e5887 to
f324591
Compare
Contributor
Parser conformance results onjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
ematipico
reviewed
Apr 26, 2025
dyc3
force-pushed
the
dyc3/no-shadow
branch
2 times, most recently
from
April 29, 2025 22:32
6174383 to
15b8296
Compare
dyc3
commented
Apr 29, 2025
Comment on lines
+65
to
+68
| RuleSource::Eslint("no-shadow"), | ||
| // uncomment when we can handle the test cases from typescript-eslint | ||
| // RuleSource::EslintTypeScript("no-shadow"), | ||
| ], |
Contributor
Author
There was a problem hiding this comment.
I have excluded the unit tests from typescript-eslint, and therefore the rule source here, because of #5822.
Comment on lines
+140
to
+146
| if is_declaration(&binding) | ||
| && is_declaration(&upper_binding) | ||
| && upper_binding.syntax().text_range().start() >= binding.scope().range().end() | ||
| { | ||
| // the shadowed binding must be declared before the shadowing one | ||
| continue; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This check is probably the most dubious one here because I don't have a source for it. I'm not entirely sure which part of the JS spec would be relevant.
Contributor
Author
|
I'm not sure why the windows test job is failing. Doesn't feel related to this PR. |
ematipico
approved these changes
Apr 30, 2025
Contributor
Author
|
I think its fine to close the issue. We can just leave the rule in nursery until the TS cases are fixed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements
noShadow, which is a port of eslint'sno-shadowrule.It doesn't include any options for the time being (as usual for new rules).
closes #5346
Test Plan
Added some snapshot tests. I also briefly went through the eslint rule to pick up some edge cases that I hadn't thought about.