Skip to content
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

"This binary expression is never nullish" errors due to the way I format my code #59546

Open
ChiriVulpes opened this issue Aug 7, 2024 · 12 comments Β· May be fixed by #59569
Open

"This binary expression is never nullish" errors due to the way I format my code #59546

ChiriVulpes opened this issue Aug 7, 2024 · 12 comments Β· May be fixed by #59569
Labels
Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@ChiriVulpes
Copy link

ChiriVulpes commented Aug 7, 2024

πŸ”Ž Search Terms

This binary expression is never nullish

πŸ•— Version & Regression Information

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.6.0-beta#code/KYOwrgtgBACghgJwC4HkBmAROBPKBvAWACgBIAOQEsBzACySQomCgF4oAmKAKi6gAYANMRJYA7iFYduvAIxDSAZTAgEFAM7M2nHh3kicDJpO28AzHqUgNSY9KgAWPRjBqA1rZ0BWecICCAG38sbENNKDEJAB8oS1UNKGjg0ISY5Ws9AP9KWnpGMOc3FOy6UIz-URw1SUykvJTM4tymeQBfYmIAE2AAY39EZm6AeysbKgRBsAAHAC4oACUewYQOgB54ZHRggShwCAAjYAQAbQBdU5TlLrQKEGAOgD4Abk6evoQB4bUbScRUTBxZus-sFnkRiEMRlBrFU2JdgNdbh1hAB6ZFQJAIXBjODgN5QH7IKCDNBQDo4KDXBBfYQAfhpUDGE0mRwJwJwJxRaKQNFAUG6PO67goJO5zGxuMQ N RJJZNw6igONwrJlO2oJTy2xxHSgwqhg3RmMVgTVOShP3Ealp9IAFEDNjgAHQNdVNZgAMilG3 uHpjKmR3tPudgUaoROUFmcIRdwAlJz0TyJPyekKRTyGQgcWA8SriaTyQqlV6bPm5aVFSAdXq1AaMbg4Ca5ea4JbrVA7b8HdgQ0EDHVPazu1A-eMA0Hgr3akwI1Gq-CbnGE2hG-49nBBeiDauC7g1BarO3-cyJ06AhVsGoTqCgA

πŸ’» Code

const sets = undefined
	// try granular part of day first
	?? group[partOfDay]
	// then check if the granular part of day is any part of nighttime, and if so try all night spawns
	?? (PartOfDay.AllNighttime & partOfDay ? group[PartOfDay.AllNighttime] : undefined)
	// then check if the granular part of day is any part of daytime, and if so try all day spawns
	?? (PartOfDay.AllDaytime & partOfDay ? group[PartOfDay.AllDaytime] : undefined)
	// fallback to all day spawns
	?? group[PartOfDay.Always];

πŸ™ Actual behavior

My codebases have a number of instances of things similar to the above β€” undefined is used to put the actual potential results each on their own line, all formatted the same way. However this no longer works in TS 5.6.0 due to it throwing an error. I think this is an over-eager error message.

πŸ™‚ Expected behavior

I should be able to format the code how I want to still

Additional information about the issue

Other examples of code in my codebases broken by this
let plugs: PlugRaw[] = undefined
	?? (this.state ? refresh.plugs ?? [] : undefined)
	?? this.plugs?.slice()
	?? (!this.plugSetHash ? undefined : (await DestinyPlugSetDefinition.get(this.plugSetHash))?.reusablePlugItems)
	?? (this.plugSetHash ? undefined : (await DeepsightSocketExtendedDefinition.get(this.item?.definition.hash))?.sockets[this.index]?.rewardPlugItems)
	?? [];
Component.create()
	.classes.add(ItemTooltipSourceClasses.ActivityName)
	.text.set(undefined
		?? Display.name(lostSectorDisplay)
		?? Display.name(source.dropTable.displayProperties)
		?? Display.name(activity))
	.appendTo(activityComponent);
.setRefreshMethod(() => undefined
	?? (status === UiStatusType.Cut ? this.human?.getStatusLevel(StatusType.Bleeding) === BleedLevel.Minor : undefined)
	?? (status === StatusType.Bleeding ? this.human?.getStatusLevel(StatusType.Bleeding) === BleedLevel.Major : undefined)
	?? !!this.human?.hasStatus(status as StatusType))

Also, this still works for true && conditions and false || conditions β€” would be nice if it stayed consistent:

const inRange = true
	&& gatherTile.x - treasureRange <= treasure.x && gatherTile.x   treasureRange >= treasure.x
	&& gatherTile.y - treasureRange <= treasure.y && gatherTile.y   treasureRange >= treasure.y
	&& executor.z === map.position.z;
const needsAuthView = false
	|| (viewRequiredAuth === "required" && !ProfileManager.isAuthenticated())
	|| (viewRequiredAuth === "spy" && !ProfileManager.get());
@jcalz
Copy link
Contributor

jcalz commented Aug 7, 2024

I wouldn't call this a formatting issue, since the line breaks aren't involved. Writing it all on the same line doesn't change the error.

Note that whether or not this is intentional behavior, the error message is bizarre. Right now it's flagging undefined with the error "This binary expression is never nullish. Are you missing parentheses?" πŸ˜•

@ChiriVulpes
Copy link
Author

That's true. I guess I was a little hyperfocused on the formatting aspect because I wouldn't do this kind of thing a single line, since it's just noise in that case

@MartinJohns
Copy link
Contributor

Note that whether or not this is intentional behavior, the error message is bizarre. Right now it's flagging undefined with the error "This binary expression is never nullish. Are you missing parentheses?" πŸ˜•

And you can't even work around it using a type assertion, which IMO would be a suitable solution to the OPs issue.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 7, 2024

The error message needs to be fixed but I'd still consider this a correctly-issued error. This doesn't seem to be an idiomatic pattern and honestly, as a human, that code does look like it has a bug. It's not really distinguishable from other cases which absolutely do represent bugs. As a workaround you could write something like

const seeBelow = undefined;
const p = seeBelow
  ?? a
  ?? b
  ?? c;

since the check is entirely syntactic on undefined.

true and false don't get the "static truthiness" check applied since things like while (true) or do { } while (false) are reasonable to write.

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Help Wanted You can do this labels Aug 7, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 7, 2024
@ChiriVulpes
Copy link
Author

Since you've added Help Wanted and put it in the Backlog, does that mean it won't be fixed in 5.6.0 unless I PR to fix it? I can try, I suppose.

Regarding the comment:

In the one project I had to port over to 5.6.0-beta due to a teammate, I changed about 30 instances of code like this to use a new _ global I set up instead. I probably could've also done something like Undefined. Either way is in my opinion pretty terrible, but it was what I had to do here.

I have been using TypeScript and doing this for a number of years and would not be exaggerating to say there are hundreds of instances of this in my code. I would really rather not have to change all of them. It's probably at least a thousand if we're including the true && and false || variants, which I see as functionally the same thing. It's really bizarre to me that it would work for those but not for this. In practice they all work the same, even if they're classified differently β€” && uses the right if the left is truthy, || uses the right if the left is falsy, and ?? uses the right if the left is undefined or null.

In an ideal world, I would be able to put nothing on the first line and have ?? or || or && work fine without something on the left, like how TypeScript supports starting types with a leading | or & for consistent formatting of the components of the type on each line (clearly at some point your design you all have seen the benefit of that?) But yeah, that does not work in JavaScript and will probably never work in JavaScript, because teams writing code with an extraneous line like this is not particularly common even if it is arguably easier to read when you're used to it due to all relevant lines being formatted identically.

So since JavaScript will probably never support this, I've been doing this alternative. (And like all programming things, if you haven't been doing it you're not used to code like this so it looks confusing. I promise, for people doing this consistently, it's not!)

As far as this not being idiomatic β€” when exactly does something become idiomatic? SQL commonly has people write WHERE 1=1 or 1=0 so that every subsequent AND condition or OR condition line can be more quickly commented out or added/removed entirely. No, I don't normally see this in JavaScript, but I don't really know why? In the case of undefined ?? at least, ?? is a fairly new construct so it makes sense to me that people don't use that operator like that yet.

@snarbies
Copy link

snarbies commented Aug 8, 2024

I imagine the PR that would be accepted is a correction on the error message. Setting aside the specific error message, that there is an error under these circumstance is "working as intended." Branching on a known/constant condition is indicative of a mistake and should be called out. It wouldn't make sense to get rid of that useful functionality to suit a coding style.

@RyanCavanaugh
Copy link
Member

In an ideal world, I would be able to put nothing on the first line and have ?? or || or && work fine without something on the left

ts-ignore seems suitable for this purpose? Since you wouldn't have a type error on the left side

As far as this not being idiomatic β€” when exactly does something become idiomatic?

"idiomatic" means "in common use". We ran this check on 800 repos and found zero instances of this pattern; everything the check did find was a bona fide bug. At some point it's worth the 0.1% false positive rate to find the other 99.9%.

@ChiriVulpes
Copy link
Author

I've cloned the latest commit and I've been playing with predicateSemantics.ts.

const p01 = null ?? 1; // This expression is always nullish.
const p02 = null ?? null; // This expression is always nullish.

const p10 = cond ?? null ?? 1; // OK
const p11 = cond ?? null ?? null; // OK
const p12 = cond ?? (null ?? 1); // This expression is always nullish.
const p13 = cond ?? (null ?? null); // This expression is always nullish.

To me it seems like the actually useful error would be on p10 and p11 here, since that could be indicative of missing parentheses, but they're apparently fine right now? And meanwhile I can't see any reason why null ?? at the start of an expression would ever exist unintentionally, since it wouldn't be affected at all by being wrapped in parentheses and null is a literal

@ChiriVulpes
Copy link
Author

This also doesn't error but probably should?

declare let opt: number | undefined;
declare let opt2: number | undefined;

const p19 = opt ?? (opt2 ? null : undefined) ?? 1; // OK

@ChiriVulpes
Copy link
Author

ChiriVulpes commented Aug 9, 2024

Well, I have it working as I think it should locally, so I could make a PR. But I think we're still not on the same page so maybe this isn't helpful? Thoughts appreciated though

image

Note: Making the middle always-nullish and never-nullish operands error involved checking the right operand as well as the left operand. I think it makes sense that it must be done though? Because given a situation where the first left operand is undefined, that expression is just whatever the right operand is, so it should be subject to the same checks

Edit: I went ahead and just made the PR because I wanted to have done the whole process once in case I need the knowledge another time in the future

@Knagis
Copy link
Contributor

Knagis commented Aug 26, 2024

I encountered this same in my project.

// OK
const a = false || obj["key1"] || obj["key2"];

// NOK
const b1 = undefined || obj["key1"] || obj["key2"];
const b2 = undefined ?? obj["key1"] ?? obj["key2"];

we had b - because at the time of writing it seemed better - since obj[] can return undefined, lets start with undefined.

It just seems slightly strange to see the special handling for false, but not null or undefined literals?

@RyanCavanaugh
Copy link
Member

From the PR description:

The expressions false, true, 0, and 1 are excluded from this check, since code like while (true) {, while(1) {, do { } while (false), if (true || i_am_just_debugging) { are common and unlikely to represent programmer errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
6 participants