Should error when unwrapping an unwrappable variable

[tinganho]

s cannot be null or undefined below. So I think it is appropriate to error when trying to unwrap.

let s: string;
id = '';
if (s!) { // should throw an error

}

[basarat]

I think you meant :

let s: string;
s = '';
if (!s) { // should throw an error

}

That said it can cause issues as people might try defensive coding due to external influences e.g. #8452 now allows people to do null/undefined checks even though they are not in the domain of the type 🌹

[tinganho]

@basarat I actually meant s!.

[basarat]

@basarat I actually meant s!.

Isn't it a syntax error?

[tinganho]

I'm not getting any error. The unwrap operator removes undefined | null from the type:

[basarat]

Indeed not an error on master

Apparently its a NonNullExpression (like you said, to remove undefined | null from the type, a TypeScript type system thing) as opposed to a PrefixUnaryExpression (a JavaScript thing). I've got some searching to do. Sorry for misunderstanding 🌹

[basarat]

Got it. I really should have read #7140 better instead of winging it 🌹

Did spend some time searching for "unwrap operator" :)

[tinganho]

Did spend some time searching for "unwrap operator" :)

Sorry might have used the wrong terminology. Non-null assertion operator is more correct.

[RyanCavanaugh]

We allow benign coercions everywhere else (e.g. unary + is valid on number, you can type-assert to the same type, etc) and it doesn't seem to be much of a problem. What's the reasoning to disallow this one?

[tinganho]

@RyanCavanaugh I would like if TS did the same with the two example you mentioned. Though I think the unwrap operator is different. It is more common. Actually I found out that had a case where a variable where most of the time of type T 99% of the cases and 1% of the cases it was undefined. So spontaneously, I put it as T | undefined. Though I discovered I needed to put ! everywhere. So I then switched it to type it as T instead. During refactor it could really help me if there was errors where ever I put an unnecessary !.

Besides refactoring benefits, I think it is correct to error just because it is unnecessary operation.

Swift also errors:

[mhegazy]

what about type parameters?

[tinganho]

what about type parameters?

Can you elaborate?

[mhegazy]

function f<T>(a: T) {
    return a!; // is this an error or not?
}

[tinganho]

It should error. I think a nullable type argument should be declared at the parameter declaration, because it is more declarative and it is not the call site that decides if it is nullable or not.

A nullable type passed in a non-nullable type argument function should also error.

function f<T>(a: T) {
    return a!; // is this an error or not? yes it is
}

let str: string | undefined;
f(str); //error str is nullable

function f<T>(a: T | undefined) {
    return a!; // is this an error or not? no it isn't
}

FWIW, that is what swift does.

(Interestingly Swift allows nullable passed as an non-null type argument):

[tinganho]

Maybe it is best to expose this feature behind a flag. As CFA improves it will break code that it didn't cover before. In my opinon it is good to error, because devs can learn new CFA:s.

[DanielRosenwasser]

I think at one point @alexeagle mentioned that if your dependencies weren't updated to use non-null assertions, you could add the assertions as something of a TODO until your dependencies were updated (and if I'm misquoting, feel free to clarify).

So it seems like using the assertion is okay in some contexts where you're migrating code, but eventually I think you'd want to get rid of any places where you don't really need those assertions. We should consider how often this scenario pops up.

[RyanCavanaugh]

We're going to leave this as an OK thing to do. In addition to precedence with other allowed facile assertions, the real risk here is that it could make improvements to the control flow analysis into breaking changes, which would be really unfortunate.