hegel icon indicating copy to clipboard operation
hegel copied to clipboard

require explicit check for undefined?

Open trusktr opened this issue 5 years ago • 11 comments

This code has a runtime error. It will not use the 0 value.

Should Hegel force us to explicitly check undefined to avoid the runtime error?

trusktr avatar May 20 '20 20:05 trusktr

This is the goal "Don't have a runtime error". But, I actually don't understand the example, because, I run it and didn't get an error. Could you please explain the problem?

JSMonk avatar May 20 '20 20:05 JSMonk

Sorry, not runtime error, but a silent error: the n && useNum(n) does not pass the 0 into useNum because 0 is falsy. So if the dev meant to pass all numbers into the function, it will pass them all except zero.

If it forces us to check for undefined, then it could prevent such a mistake.

trusktr avatar May 20 '20 20:05 trusktr

Maybe n && useNum(n) would still have a type error, and we would need to write something like n !== undefined && useNum(n). Not sure though.

trusktr avatar May 20 '20 20:05 trusktr

I mean, maybe the fact that undefined was not checked could somehow be a type error?

Or, maybe numbers shouldn't be treated as booleans? But I can see how this will make some code more verbose.

trusktr avatar May 20 '20 20:05 trusktr

Maybe https://github.com/JSMonk/hegel/issues/227 would help with eliminating verbosity.

Then

isNumber(n) && useNum(n)

would lead to no type error, because the type system would know that the check for typeof n === 'number' means that n can't possibly be undefined.

trusktr avatar May 20 '20 21:05 trusktr

Maybe n && useNum(n) would still have a type error, and we would need to write something like n !== undefined && useNum(n). Not sure though.

No, actually, if you will hove at the left part of the && you will see which values will be not passed into the function.

JSMonk avatar May 20 '20 21:05 JSMonk

Oh yeah, the tooltip shows 0 | undefined, but it isn't a guarantee people are going to look at that.

So TLDR, this type of error checking is out of scope?

trusktr avatar May 22 '20 00:05 trusktr

It seems like I can't show the error in this case. And I don't sure, that this is Type System responsibility (Actually, I think it's linter responsibility, because of this case is about coding style neither the type system issue). What do you think about it?

JSMonk avatar May 22 '20 00:05 JSMonk

i think that this would report error

const nums: Array<number> = [0,1,2,3,4,5]

for (let i=0; i<nums.length; i++) {
  const n = nums[i]
  n && useNum(n)// should report: Type "number | undefined" is incompatible with type "boolean"
}

function useNum(n: number) {
  n
}

like this does

const nums: Array<number> = [0,1,2,3,4,5]

for (let i=0; i<nums.length; i++) {
  const n = nums[i]
  if (n) useNum(n)// reports: Type "number | undefined" is incompatible with type "boolean"
}

function useNum(n: number) {
  n
}

i think && and || are boolean operators (and you should not be allowed to use it otherwise)

thecotne avatar May 22 '20 01:05 thecotne

I think it would make sense to warn on Nullish | PossblyFalsy used in if, while, ternary, etc. (where Nullish is either null or undefined or union of both, and PossiblyFalsy is any combination of string, number, bigint and boolean)

This is what Flow does (with sketchy-null lint enabled), and there's strict-boolean-expressions for typescript-eslint which also does that.

phaux avatar May 23 '20 17:05 phaux

I don't think a linter could do a good job checking these things, otherwise the linter would become something like Hegel already is.

trusktr avatar May 29 '20 20:05 trusktr