Repository navigation
Special case arrow functions with only parameter unused - #21144
Conversation
| oldFunction.equalsGreaterThanToken, | ||
| oldFunction.body); | ||
|
|
||
| suppressLeadingAndTrailingTrivia(newFunction); |
There was a problem hiding this comment.
Could you add a comment why this is done?
There was a problem hiding this comment.
I see the comment above newFunction, but I was talking about the line suppressLeadingAndTrailingTrivia.
|
|
||
| suppressLeadingAndTrailingTrivia(newFunction); | ||
|
|
||
| changes.replaceRange(sourceFile, { pos: oldFunction.getStart(), end: oldFunction.end }, newFunction); |
There was a problem hiding this comment.
Could this use replaceNode?
There was a problem hiding this comment.
Possibly we need to add another method to ChangeTracker, because I don't believe replaceNode is suitable for (nearly?) any code fix or refactoring - it clobbers the leading trivia of the replaced node.
There was a problem hiding this comment.
Hm, replaceNode starts with node.getStart() but then adjusts it -- maybe there's a bug in getAdjustedStartPosition, because it should be replacing a range that doesn't include leading trivia.
There was a problem hiding this comment.
I understood the adjusted start position was an attempt to establish the boundary between the trailing trivia of the preceding token and the leading trivia of the current token.
There was a problem hiding this comment.
Well, that's a problem if we want to preserve the leading trivia of the current token -- maybe all the uses of replaceNode have this bug and replaceNode should be fixed to just use getStart().
There was a problem hiding this comment.
Yes, every caller of replaceNode probably needs to be reviewed. Few of the existing tests include interesting trivia.
There was a problem hiding this comment.
OK, I've created an issue for this at #21246. This PR should be good to go for now.
|
|
||
| // @noUnusedLocals: true | ||
| // @noUnusedParameters: true | ||
| //// function f1() { |
There was a problem hiding this comment.
Is the outer function needed? Could we reproduce this with just x => {}?
There was a problem hiding this comment.
I just copied the existing test, but I can remove it, if you prefer.
There was a problem hiding this comment.
Yeah, would be better to simplify the other test too.
3fb01f1 to
9a83077
Compare
ghost
mentioned this pull request
Fixes #18274
I've made some notes in the regression tests where the trivia handling isn't perfect, but none of the issues seemed impactful enough to justify deeper changes.