Suggestion: should non-null assert propagate?

[jods4]

I was wondering whether ! should be taken into account by the dataflow analysis, and I don't see why it shouldn't.

It is like a cast that says "this value is not null", so from this point I guess the compiler could remove undefined/null from future uses.

For example, I use ! to work around #9631, so I have code that looks like (simplified):

protected dataArrayChanged(changes: ChangeRecord[]) {
  // #9631: TS incorrectly infers `change: ChangeRecord | undefined`
  for (const change of changes) {
    for (let i = 0; i < change!.removed.length; i++)
      this.dt.row(i).remove();
    if (change!.addedCount > 0)
      this.dt.rows.add(this.data.slice(change!.index, change!.index + change!.addedCount));
  }
  this.dt.draw();
}

Observe how I added 5 ! to make change not null.
The first one could have been enough. After all, once I say "change is not undefined", there is no reason to assume it could be until I modify the variable again.

// Let's say I know x is not undefined
const x: number | undefined;
// Here x: number | undefined, so x! is required
x!.toString();
// Here x: number because of x! above
x.toString(); // ok

[svieira]

I'm not in favor of this because of the legibility of the assertion. Unlike the leading ! in a if (!change) the ! in for (let i = 0; i < change!.removed.length; i++) is almost entirely invisible, which would make discovering such mutations to the type much harder for human beings.

Why not simply add a check for undefined to strip it out instead?

for (const change of changes) {
  // TODO: Remove when typescript#9631 is resolved
  if (change === undefined) continue;
  // change is now *just* ChangeRecord unless I'm missing something
}

[jods4]

@svieira Sure, it would work. But it kind of defeats the purpose of !, doesn't it?
Also, ! is a cast that generates no code. Your code has no effect in practice but will be emitted to JS.

BTW what is the scenario you have in mind here? I don't think adding a ! will ever create errors downstream, as the compiler doesn't warn if you do things like if (a === null) when a: number.
So I can't imagine a case where you would be hunting that extra ! that creates errors in your code.

If your issue is rather that you find ! unreadable in itself, then that's a different problem (probably #9637) -- and you don't have to use it. I believe that for those of us who use ! occasionnally, narrowing the variable type is a logical thing to do.

[svieira]

The problem is with a larger code base without consistent type annotations:

function assumptions(value?: string, options: OptionsType) {
  // code
  // code
  // code
  // new code
  // should have been a conditional check but someone chose to do the wrong thing
  // (because right now we always provide value)
  const something = value!.length;
  // code
  // new code using something
  // code
  return {
    // fields
    details: value
   // details *used* to be a string | undefined
   // now, with the bad change it's a string
  };
}

Consider the alternatives:

if (value != null) {
  // new code
}
// value is still string | undefined here

if (value == null) return defaultOptions;
// value is *really* a string here

const nonNullValue = value as string;
// Hey, you made a cast here, very clear to see what you're doing
// If you use `value` it is still string | undefined
// Only using nonNullValue gets you the cast value

The only one that is magic is non-null assertions (!):

const anythingElse = value!.some.property.access;
// value is now stripped of undefined even if that wasn't the intent

I would rather see something like this:

value = value!;
// Hey I'm really intending on mutating the type here for everything that comes after me

[jods4]

Where I don't follow you is here:

const anythingElse = value!.some.property.access;
// value is now stripped of undefined even if that wasn't the intent

If there should have been a check and in the future value suddently becomes unexpectedly null then your code crash right at value!.some. At this point I'm not sure if it matters what happens with the code after it. Buggy code is buggy.

[RyanCavanaugh]

I agree the example is not compelling as written. But consider something very similar:

// in external-lib.d.ts
 // File written pre-strictNullCheck; in reality accepts string | undefined
declare function doSomething(x: string): void;

// in my-file.ts
function foo(x: string | undefined): string {
  // Use ! to silence error about function call that I know is actually valid
  doSomething(x!);
  return x;
}

let y = foo(undefined); // oops, y: string but has value 'undefined'

[jods4]

@RyanCavanaugh I see. This is an usage of ! that I never considered. You are not marking x as not null, you are hacking around an incorrect definition.

When I have an incomplete .d.ts (which of course happens frequently at this point) I fix it. I have a feeling that starting to hack calls with ! is a very slippery slope and I won't be happy with a code base like this one year from now. Is this a scenario that you really want to support? In this case my suggestion is not playing nice, indeed.

Nitpicker's corner: your example is broken because you explicitly say foo(): string, which would be an error from the coder rather than the compiler. I guess you meant to have an implicitly typed return value.

[RyanCavanaugh]

Re: Nitpick - correct, I'm saying that under the proposed behavior, there wouldn't be an error, which is bad.

I'm not sure what you're getting at with the first 2 paragraphs there. Is there some way to meaningfully distinguish the 'bad' cases from the 'good' cases?

[jods4]

@RyanCavanaugh What I meant is this:

I think of ! as a special cast that removes the undefined/null types.
In other words when I see this code: doSomething(x!) I think: x is nullable but it is guaranteed to be not null here for some reason.

When in fact your usage is different. x could very well be null here, but you want to satisfy the typechecker that only let you pass a non-null value to the function, because of an incorrect definition.

This is really the same as this situation:

// bad .d.ts, maybe because it's for an old version of a library that was not updated yet
// Actually, frob now also accepts an array: `x: number | number[]`
function frob(x: number): void;

// So in your code you do:
let some: number | number[] = [1, 2, 3];
frob(<number>some);

I would not consider this good style, and to me your ! example is the same thing. You lie about the real type of x to pass a bad definition.

Is there some way to meaningfully distinguish the 'bad' cases from the 'good' cases?

Well yes: if at runtime x! is guaranteed to be non null that looks like a good case. If it could actually be null I think you're doing something fishy.

[buola]

@jods4

Precisely because ! is a cast that removed the undefined/null types, it shouldn't behave differently than any other cast.

What you're saying is not necessarily 'I know it is not undefined' but more 'I want to treat it as not undefined in this expression'.

In my opinion, this suggestion would be the same as accepting something like this

function foo(): number | number[] { return 3; }
function frob(x: number): void {}

let some = foo(); //some is of type 'number | number[]'
frob(some as number); //we might know it's a number, so we do this. Okay.
frob(some); //If casts "teach about" types, this would not be an error. Currently, it is.

Another problem would be that, when closures are involved, a variable being 'not undefined' in one line doesn't necessarily mean that it will still be 'not undefined' in the next line.
However, this is a problem of type assertions in general. For example, the following legal code throws because typescript can't detect that a is undefined, and it can't really be expected to.

(function (){
    var a: number | undefined = 3;
    var b = () => { a = undefined; };

    if(a) {
        b();
        console.log(a.toFixed());
    }
})();

[jods4]

@buola this actually crossed my mind when I wrote this suggestion. I did not want to push it too far but I think that it would be perfectly logical that a cast narrows the actual type of a union when it's used, yes.

I would certainly accept your example. Think about it: either the call frob(<number>some) is a coding error and can fail at runtime, or there is really no reason why the call on the next line frob(some) wouldn't work. So I say it would make perfect sense for the compiler to allow it.

Casting means "hey compiler I know better than you what type this variable really is in this location". Then why shouldn't the compiler learn from our hints?

As you noted, your second problem is unrelated to this suggestion. It is a limitation of the type analysis as it exists today.

[RyanCavanaugh]

Related #10421

It's not at all clear what the "scope of effect" of the assertion should be. For example let x = y ? a!.foo : b; -- is a still non-null on the next line? Maybe y was a discriminator for a's nullness, maybe it wasn't. Unless we had a very clear understanding of what made an assertion appear in the first place, it's not going to be good for us to guess what the scope of the assertion should be. And if we did understand, arguably we should make the compiler understand what introduced the assertion in the first place.

It's not clear there's a good answer here. Sort of unrelatedly, the OP was caused by a compiler bug (for / in should not introduce undefined into the iterator type), so that much is at least solved already. And there's always the const xx = x!; workaround.

[jods4]

ok maybe I didn't think this as thoroughly as you guys, but couldn't it use the same rules as the data flow analysis that's already in place?

In your example a!.foo is in a branch and there is no way to extrapolate that it is also valid on the main code path. As you said, y could very well have been key to the non-null aspect.

This is pretty much the same thing as an if: type guards inside the "then" block do not propagate outside of it.

BTW although the idea still makes sense to me, I have been using null-strict code for a while and lack of this hasn't really been an issue. Mostly because 99% if the time my code is correctly typed. So maybe it's not worth pursuing if you feel the benefits are not worth the effort.

[RyanCavanaugh]

I think the confusing part is that you might write code like this

function fn(x: string | undefined) {
  /* do something here that makes x necessarily not-null that the compiler can't detect */

  // lots more lines
  console.log(x!);
  // lots more lines
  return x.substr(3); // OK
}

Then you refactor a bit

function fn(x: string | undefined) {
  /* do something here that makes x necessarily not-null that the compiler can't detect */

  if (enableLogging) {
    // lots more lines
    console.log(x!);
    // lots more lines
  }
  return x.substr(3); // Error!
}

and it's really not clear why you're getting an error on the last line now, especially if the console.log(x!) had been some more complex expression where it was easy to miss that it was x! instead of x

If you're not running into this much in practice, that's great news. Hopefully you're only having to use ! in places where it's truly exogenous knowledge.

[jods4]

I agree all the inferred stuff can sometimes be a little obscure, this is already the case today. Especially when you encounter shortcomings of the typing engine.

In your example, yes there is a new error. There is a good reason for that, even though I agree it can be a little obscure. Reviewing the changes (hopefully people use version control), I think it is reasonably clear that moving a type assertion (the !) into a branch might cause that. And the fix is trivial: if it's still true that x is not null, add a !.
If there are so many lines that you can't see what you're doing, it's a different problem. Overly long functions are a code smell.

But again I'm not particularly concerned by this anymore. If you do something in this area, I hope it's something more general, like narrowing types in a block for casts as well, etc.

EDIT: The issue you linked #10421 is similar but avoids the "obscure" changes by introducing explicit syntax for block narrowing of types, maybe that's better. A no-op statement like assume x!; or assume <number>x; would achieve the same goals as this issue without the drawbacks you mentionned.

[pmoleri]

Just adding a use case. This would be nice for writing real assertions:

class Foo {
   prop: string | null;

   propLength() {
       assert(this.prop!);
       return this.prop.length;
  }
}

function assert(value: any) {
    if (!value) {
        throw new AssertError("Null or undefined");
    }
}

This way, if I use ! only inside asserts calls I make sure that it has been properly asserted.

[ChayimFriedman2]

@pmoleri TypeScript 3.7 introduced assertion functions exactly for this purpose, and they have clearer syntax and no need for redundant !, while your code suffers from huge in-readability because you use the non-null assertion where the operand can be null, and even expected to.