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

Disallow comparison against NaN #49962

Closed
arzeth opened this issue Jul 19, 2022 · 8 comments Β· Fixed by #50626
Closed

Disallow comparison against NaN #49962

arzeth opened this issue Jul 19, 2022 · 8 comments Β· Fixed by #50626
Labels
Committed The team has roadmapped this issue Experience Enhancement Noncontroversial enhancements Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@arzeth
Copy link

arzeth commented Jul 19, 2022

Suggestion

πŸ” Search Terms

disallow NaN
comparing with NaN
disallow comparing NaN

βœ… Viability Checklist

My suggestion meets these guidelines:

  • [?] This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • βœ… This wouldn't change the runtime behavior of existing JavaScript code
  • βœ… This could be implemented without emitting different JS based on the types of the expressions
  • βœ… This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • βœ… This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Similar to #45978 . So the error should be something like The condition will always return 'false' since NaN !== NaN. Use Number.isNaN.

πŸ“ƒ Motivating Example

function setReputation (n: number)
{
  // ! here is the bug:
  if (n === NaN) throw new Error('setReputation:: Impossible reputation')
  // update the record in a remote DB
}
const foundGolems = 0
const killedGolems = 0
setReputation(killedGolems / foundGolems * 100)
// 0/0 is NaN

or what has actually happened to me:

export function clamp ( // @license CC0
	n:   number,
	min: number|bigint,
	max: number|bigint,
): typeof n
export function clamp (
	n:   bigint,
	min: number|bigint,
	max: number|bigint,
): typeof n
export function clamp (
	n:   number|bigint,
	min: number|bigint,
	max: number|bigint,
): typeof n
{
	// +null === 0
	// +'' === 0
	// +undefined === NaN
	const nFixed = typeof n === 'bigint' ? n : (
		// @ts-ignore
		'' === n || n === null || typeof n === 'symbol'
		? NaN : +n
	)
	const minFixed = typeof min === 'bigint' ? min : (
		// @ts-ignore
		'' === min || min === null || typeof min === 'symbol'
		? NaN : +min
	)
	const maxFixed = typeof max === 'bigint' ? max : (
		// @ts-ignore
		'' === max || max === null || typeof max === 'symbol'
		? NaN : +max
	)
	if (nFixed === NaN)
	{
		const ret = nFixed
		//const ret = min == max ? (typeof min === 'bigint' ? +min.toString() : min) : nFixed
		console.warn(
			'clamp(n=%O, min=%O, max=%O):: n === NaN; returning %O',
			n, min, max, ret,
		)
		return ret
	}
	let ret: typeof n
	if (
		minFixed === NaN
		&&
		maxFixed === NaN
	)
	{
		ret = nFixed
		console.warn(
			'clamp(n=%O, min=%O, max=%O):: min === NaN && max === NaN; returning `n` (%O)',
			n, min, max, ret,
		)
	}
	else if (maxFixed < minFixed)
	{
		// throw new RangeError
		//ret = maxFixed // similar to torch.clamp
		ret = minFixed // because arguments are specified in the left-to-right *order*.
		//ret = nFixed < maxFixed ? maxFixed : (nFixed > minFixed ? minFixed : nFixed)
		/*console.warn(
			'clamp(n=%O, min=%O, max=%O):: max<min; returning `min` (%O)',
			n, min, max, ret
		)*/
	}
	else
	{
		ret = nFixed < minFixed ? minFixed : (nFixed > maxFixed ? maxFixed : nFixed)
	}
	if (typeof n === 'bigint')
	{
		return typeof ret === 'bigint' ? ret : BigInt(Math.floor(ret))
	}
	if (typeof ret === 'bigint')
	{
		return +ret.toString()
	}
	return ret
}

πŸ’» Use Cases

Since NaN is very rare, a programmer could forget to use Number.isNaN(variable), and accidentally use ===.
Or a programmer could have never even learned this, and just applies knowledge from other langs where NaN === NaN.

@fatcerberus
Copy link

other langs where NaN === NaN

Are there any such languages? NaN != NaN (as well as NaN itself) is, as far as I know, a quirk of the IEEE floating point specification and not specific to any particular programming language.

@jcalz
Copy link
Contributor

jcalz commented Jul 20, 2022

Related: #28682 and #15135

@arzeth
Copy link
Author

arzeth commented Jul 20, 2022

other langs where NaN === NaN

Are there any such languages? NaN != NaN (as well as NaN itself) is, as far as I know, a quirk of the IEEE floating point specification and not specific to any particular programming language.

a quirk of the IEEE floating point specification

I didn't know that, all my life I thought it was one of quirks of JavaScript, so I supposed there are such languages since there are so many of them now. Now I doubt there are such languages.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Experience Enhancement Noncontroversial enhancements labels Jul 20, 2022
@RyanCavanaugh
Copy link
Member

A syntactic check that == and === RHS isn't NaN seems like a good plan

@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jul 20, 2022
@nicolas377
Copy link
Contributor

nicolas377 commented Jul 25, 2022

Zeroing in on checking RHS, is there precedent for ignoring specific checks on LHS of equality checks? It's a bit uglier to write NaN === someVariable, but it's still possible to write.

@RyanCavanaugh
Copy link
Member

Everything costs some amount of performance and I think the set of people weird enough to write constants-on-the-left are the people who know better than to put NaN there.

@HolgerJeromin
Copy link
Contributor

Just a side note:
SymbianOS (OS code, not application code) had a style guide to put constants-on-the-left to guard against missing = chars like if(0 = myvar) instead of intended if(0 == myvar).
if(0 = myvar) is detected by the compiler, but if(myvar = 0) is valid code.

@DanielRosenwasser DanielRosenwasser added Committed The team has roadmapped this issue Fixed A PR has been merged for this issue labels Sep 20, 2022
@DanielRosenwasser
Copy link
Member

TypeScript now errors on ===, ==, !==, or != when either operand is NaN. Our error messages will suggest using some variation of Number.isNaN(...), and our language service will provide a quick fix in such cases.

Thanks @a-tarasyuk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Experience Enhancement Noncontroversial enhancements Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants