Account for right operands & fix a weird error message for leftmost nullish literals in checkNullishCoalesceOperands #59569

ChiriVulpes · 2024-08-09T03:18:29Z

This PR:

Removes the error for nullish literals as the leftmost operand of an expression of all nullish coalescing operators.
Performs the same checks for right-side operands that are not the last operand in an expression of all nullish coalescing operators.

Examples:

declare let opt: number | undefined;

const p01 = null ?? 1; // fixed error message, says "always nullish" instead of "never nullish"
const p02 = (opt ? null : undefined) ?? 1; // previously error, still error
const p03 = 1 ?? 2; // previously error, still error

const p10 = opt ?? null ?? 1; // previously OK, now errors on `null` as "always nullish"
const p11 = opt ?? (opt ? null : undefined) ?? 1; // previously OK, now errors on `opt ? null : undefined` as "always nullish"
const p12 = opt ?? (opt ? 1 : 2) ?? 3; // previously OK, now errors on `opt ? 1 : 2` as "never nullish"

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

ChiriVulpes · 2024-08-09T03:22:42Z

@microsoft-github-policy-service agree

…hCoalesceOperands

MartinJohns · 2024-08-09T05:01:48Z

const p01 = null ?? 1; // previously error, now OK

According to #59546 this should be an error.

ChiriVulpes · 2024-08-09T05:07:51Z

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

jakebailey · 2024-08-09T05:28:28Z

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.

ChiriVulpes · 2024-08-09T05:30:57Z

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)

RyanCavanaugh

Most of the diff looks good but the special case for null ?? null etc is not an approved change.

tests/baselines/reference/predicateSemantics.errors.txt

…ways nullish

ChiriVulpes · 2024-08-10T01:24:47Z

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:

The PredicateSemantics of binary expressions in getSyntacticNullishnessSemantics are now actually calculated like the ConditionalExpression, which results in situations like null ?? null ?? null actually erroring over the whole expression. As a result of these changes, the right operand check for never nullish is no longer necessary, so it was removed
Since nullish coalesce errors were all previously added to the operands of a nullish coalesce expression, null ?? null as an expression by itself was not able to be caught, so now the semantics of the left and right operands are used to add a new error if both operands are always nullish and that"s the whole expression
Added a couple more test lines

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.

ChiriVulpes · 2024-08-10T01:37:24Z

Actually maybe it would be more useful for me to explain the changes with two variations.

So the current code I just pushed results in errors like this:

The alternative would be reverting the change to start resolving PredicateSemantics of binary expresions and keeping the right side operand never nullish check, which looks like this:

I thought the former was probably closer to what you actually wanted though so that"s what I went with. If you wanted something between the two of them, IE preferring the second version of p22 and p23, but wanting to error on more than just the middle null of null ?? null ?? null, I think it would result in the surface area of this PR getting even bigger than it already is

MartinJohns · 2024-08-10T08:45:12Z

@RyanCavanaugh Can you clarify if I missed something? In the issue #59546 you said something like undefined ?? value should be an error, but this PR will allow null ?? value. Is null treated differently here, did you change your mind, or should null ?? value be an error?

RyanCavanaugh

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?

tests/baselines/reference/predicateSemantics.errors.txt

ChiriVulpes · 2024-08-12T21:12:12Z

~~As I"ve already stated, the error message being confusing was already fixed due to other changes since the initial beta release, I just forgot to test nightly when I made my initial bug report.~~ [Edit: Apparently I was just hallucinating this, nevermind. I"m dumb]

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

ChiriVulpes · 2024-08-13T11:07:50Z

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

RyanCavanaugh

Thanks!

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 9, 2024

Account for right operands & leftmost nullish literals in checkNullis…

cab0009

…hCoalesceOperands

ChiriVulpes force-pushed the check-non-rightmost-right-operands-and-ignore-leftmost-nullish-literals branch from 716a382 to cab0009 Compare August 9, 2024 03:24

ChiriVulpes marked this pull request as ready for review August 9, 2024 03:48

RyanCavanaugh requested changes Aug 9, 2024

View reviewed changes

tests/baselines/reference/predicateSemantics.errors.txt Show resolved Hide resolved

Also error on nullish coalesce expressions where both operands are al…

df5f297

…ways nullish

ChiriVulpes force-pushed the check-non-rightmost-right-operands-and-ignore-leftmost-nullish-literals branch from 72ea8bd to df5f297 Compare August 10, 2024 01:15

ChiriVulpes requested a review from RyanCavanaugh August 10, 2024 01:47

RyanCavanaugh requested changes Aug 12, 2024

View reviewed changes

tests/baselines/reference/predicateSemantics.errors.txt Show resolved Hide resolved

ChiriVulpes changed the title ~~Account for right operands & leftmost nullish literals in checkNullishCoalesceOperands~~ Account for right operands & fix a weird error message for leftmost nullish literals in checkNullishCoalesceOperands Aug 13, 2024

Pare down the PR to only fix the error message and check right operands

2d84410

ChiriVulpes force-pushed the check-non-rightmost-right-operands-and-ignore-leftmost-nullish-literals branch from 789a0c2 to 2d84410 Compare August 13, 2024 11:03

ChiriVulpes requested a review from RyanCavanaugh August 13, 2024 11:08

RyanCavanaugh approved these changes Aug 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for right operands & fix a weird error message for leftmost nullish literals in checkNullishCoalesceOperands #59569

Account for right operands & fix a weird error message for leftmost nullish literals in checkNullishCoalesceOperands #59569

ChiriVulpes commented Aug 9, 2024 •

edited

Loading

ChiriVulpes commented Aug 9, 2024

MartinJohns commented Aug 9, 2024

ChiriVulpes commented Aug 9, 2024

jakebailey commented Aug 9, 2024 •

edited

Loading

ChiriVulpes commented Aug 9, 2024 •

edited

Loading

RyanCavanaugh left a comment

ChiriVulpes commented Aug 10, 2024 •

edited

Loading

ChiriVulpes commented Aug 10, 2024

MartinJohns commented Aug 10, 2024

RyanCavanaugh left a comment

ChiriVulpes commented Aug 12, 2024 •

edited

Loading

ChiriVulpes commented Aug 13, 2024

RyanCavanaugh left a comment

Account for right operands & fix a weird error message for leftmost nullish literals in checkNullishCoalesceOperands #59569

Are you sure you want to change the base?

Account for right operands & fix a weird error message for leftmost nullish literals in checkNullishCoalesceOperands #59569

Conversation

ChiriVulpes commented Aug 9, 2024 • edited Loading

ChiriVulpes commented Aug 9, 2024

MartinJohns commented Aug 9, 2024

ChiriVulpes commented Aug 9, 2024

jakebailey commented Aug 9, 2024 • edited Loading

ChiriVulpes commented Aug 9, 2024 • edited Loading

RyanCavanaugh left a comment

Choose a reason for hiding this comment

ChiriVulpes commented Aug 10, 2024 • edited Loading

ChiriVulpes commented Aug 10, 2024

MartinJohns commented Aug 10, 2024

RyanCavanaugh left a comment

Choose a reason for hiding this comment

ChiriVulpes commented Aug 12, 2024 • edited Loading

ChiriVulpes commented Aug 13, 2024

RyanCavanaugh left a comment

Choose a reason for hiding this comment

ChiriVulpes commented Aug 9, 2024 •

edited

Loading

jakebailey commented Aug 9, 2024 •

edited

Loading

ChiriVulpes commented Aug 9, 2024 •

edited

Loading

ChiriVulpes commented Aug 10, 2024 •

edited

Loading

ChiriVulpes commented Aug 12, 2024 •

edited

Loading