Improve error range for ts2657 (jsx expr must have parent element), add code fix for it #37917
Conversation
sandersn
added
the
For Uncommitted Bug
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
Just a few nits; I think this looks like a big improvement.
| if (!compilerOptions.allowUnreachableCode && isSideEffectFree(left) && !isEvalNode(right)) { | ||
| error(left, Diagnostics.Left_side_of_comma_operator_is_unused_and_has_no_side_effects); | ||
| const sf = getSourceFileOfNode(left); | ||
| const isInDiag2657 = sf.parseDiagnostics.some(diag => { |
There was a problem hiding this comment.
You could consider using forEach or a for/of loop to terminate once diag.start > left.end.
There was a problem hiding this comment.
I didn't catch that, Array.some can quite early, I think it is same as for/of + break
There was a problem hiding this comment.
I mean that with .some, if you don’t find the diagnostic you’re looking for, you’ll keep looking all the way to the end of the array. But since these diagnostics are added in the order that the parser sees them, you know that after you move all the way past the end of left, you’re not going to find what you’re looking for at all. So while .some can terminate early in the positive case, you also have an opportunity to terminate early in the negative case.
That said, I don’t think it’s a big deal since this is an error scenario and the body of the loop isn’t expensive.
Jack-Works
force-pushed
the
codefix/jsx-2695
branch
from
e610faa to
b8cf1bc
Compare
Jack-Works
force-pushed
the
codefix/jsx-2695
branch
from
5e81b32 to
a52a1d0
Compare
|
rebased to resolve conflict |
Jack-Works
force-pushed
the
codefix/jsx-2695
branch
from
a52a1d0 to
389efeb
Compare
|
rebased to resolve conflict again |
Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>
Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>
* upstream/master: (78 commits) LEGO: check in for master to temporary branch. Skip default when initially iterating exports in __importStar, same as __exportStar (microsoft#38808) fix line endings Improve error range for ts2657 (jsx expr must have parent element), add code fix for it (microsoft#37917) fix(32341): add prefix name for module exports properties (microsoft#38541) fix(19385): add space after brace in the multiline string template (microsoft#38742) fix(38815): dive in arrow functions to check only this usage instead of checking all statements (microsoft#38865) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Convert HTML tags in doc-comments into markdown fix linting error LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. fix(38722): change error message for use-before-declaration on const enum (microsoft#38728) ...
|
Is the code fix still woring? I cannot use it in 4.3.0-dev-20210323. Is it removed or a regression? |
|
I think it must be a regression. |
Should I open a new issue for it? Or is there a fix already on the fly? |
|
You can open a new issue. The weird thing is that tests are of course passing. So some investigation is probably needed. |
3 participants
Before:
After:
Only one error with the correct length (but trivia is included, I don't know how to remove it from the error range)
With code fix:
After code fix:
After fix all:
