JSMonk / hegel

An advanced static type checker
https://hegel.js.org
MIT License
2.09k stars 59 forks source link

require explicit check for undefined? #226

Open trusktr opened 4 years ago

trusktr commented 4 years ago

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?

JSMonk commented 4 years ago

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?

trusktr commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

JSMonk commented 4 years ago

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.

trusktr commented 4 years ago

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?

JSMonk commented 4 years ago

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?

thecotne commented 4 years ago

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)

phaux commented 4 years ago

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.

trusktr commented 4 years ago

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