-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Account for right operands & fix a weird error message for leftmost nullish literals in checkNullishCoalesceOperands #59569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@microsoft-github-policy-service agree |
…hCoalesceOperands
716a382
to
cab0009
Compare
According to #59546 this should be an error. |
No? That was the whole point of me making this PR 😛 Unless I’m misunderstanding what you mean? The PR accomplishes two things, it prevents erroring in the way I use it while preserving all other errors, and it adds new errors to right side operands that were previously not checked |
I'm pretty sure Ryan was saying that the error message needed to be fixed to not complain about parens where that doesn't make sense, and that an unconditional nullish value on the left side was dubious code that should still error. I'm not sure how #59546 (comment) could be read any other way. |
The error message was already fixed by another commit since the release of 5.6.0-beta, it was irrelevant to my issue. I failed to test in nightly when I made the initial issue report and it muddied everything. (I only noticed while working on this PR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the diff looks good but the special case for null ?? null
etc is not an approved change.
72ea8bd
to
df5f297
Compare
Okay so I've addressed the line you pointed out and some similar lines, unfortunately it increased the surface area of the PR a bit. I tried to limit it as much as I could. List of changes:
Note that this does result in more overlapping errors in the baselines, but I think most cases in real-world code won't get much or any of that. Hopefully this is what you wanted? Up to revert or do this differently if you prefer. |
@RyanCavanaugh Can you clarify if I missed something? In the issue #59546 you said something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like we're getting a "meeting of the minds" here and the PR is still trying to create a carve-out for the behavior described in the original bug. The only change approved from that bug is to tweak the error message to not incorrectly talk about binary expressions when one exists. It might be cleaner to start fresh on a new PR that only makes that change?
I wanted to make a PR that addressed my issue in whatever way was most likely to actually be considered and pulled into the project. If it won't be, then that's fine, I guess. It's bad for me, because it means that if I want to write code that's doing this that's still just as comprehensible I'll have to be maintaining a TS fork or a TS patch indefinitely, but I have to do what I have to do and I don't fault anyone for having different opinions. I do think the eagerness of the error makes the part of it I have issue with a better fit for linting than an unconfigurable TS level error, but w/e |
789a0c2
to
2d84410
Compare
This PR no longer addresses my issue, and instead only fixes the error message and adds in missing checks for the right operands. I don't think this will be controversial anymore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This was approved but I think just not merged. @ChiriVulpes do you mind pulling in main? If you don't have time, I'm happy to do it for you. |
…-right-operands-and-ignore-leftmost-nullish-literals
@jakebailey I think it should be good now, it's been months since I touched the TS codebase so I might have done something wrong, though. Let me know if it needs tweaks |
(The one fail appears to be unrelated and might go away with a rerun) |
@typescript-bot test it |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
Both things above are correctly caught. In hoarder: feedData.items.forEach((item) => {
item.guid = item.guid ?? `${item.id}` ?? item.link;
});
In vscode: const id = `${claims.tid}/${(claims.oid ?? (claims.altsecid ?? '' + claims.ipd ?? ''))}`; This parses like: const id = `${claims.tid}/${(claims.oid ?? (claims.altsecid ?? ('' + claims.ipd) ?? ''))}`; Clearly they meant: const id = `${claims.tid}/${(claims.oid ?? ((claims.altsecid ?? '') + (claims.ipd ?? '')))}`; |
Fixes #59546
This PR:
Examples:
The tests in
predicateSemantics.ts
have more cases to verify this works in all situations, but it's possible I've missed some and this needs more coverage.For more information, see #59546
Note: this overview has been edited to reflect the current PR