Fix #38608

[jack-williams]

Fixes #38608

Narrowing an object to never using in when the value was not declared as a union seems like undesirable behaviour.

[jack-williams]

@typescript-bot test this
@typescript-bot user test this

[typescript-bot]

Heya @jack-williams, I've started to run the extended test suite on this PR at 2609521. You can monitor the build here.

[typescript-bot]

Heya @jack-williams, I've started to run the parallelized community code test suite on this PR at 2609521. You can monitor the build here.

[typescript-bot]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[jack-williams]

Baseline changes:

VSCode fails on:

function improveError(err: Error): Error {
    if ('errno' in err && err['errno'] === 'ENOENT' && 'path' in err && typeof err['path'] === 'string') {
        return new Error(nls.localize('ext.term.app.not.found', "can't find terminal application '{0}'", err['path']));
    }
    return err;
}

Previously err would narrow to never and stuff just 'worked'. Now err stays as Error and we cannot index using 'errno'. Proposed fix:

function improveError(err: Error & { errno?: unknown, path?: unknown }): Error {
// ....

Other diff looks unrelated.

[weswigham]

Rather than the declared type, shouldn't this just be looking at the input type? If we look at the declared type, it precludes things like

declare var x:  unknown;
if (isAOrB(x)) {
  if ("aProp" in x) {
    x; // A
  }
  else if ("bProp" in x) {
    x; // B
  }
  else if ("cProp" in x) {
    x; // never
  }
}

since I'm pretty sure we just merged a fix to a lot of cases like that.

[jack-williams]

The fix that just went in was for discriminant property narrowing, which solved examples that are similar to yours, but it's a disjoint code path. Changing declaredType to type would revert to the original behaviour, but also have the correct behaviour here.

I've made a slight modification that narrows singletons if the current type is not the same as the declared type. This is abit of a hack, but in general working out the intent does not seem easy.

I added some discussion to the linked issue. The best thing might be to make no change at all.

[sandersn]

@weswigham what do you think? should we take this or not?

[weswigham]

Sure, this looks like both a pretty small change, and a pretty agreeable one now.

[jack-williams]

I'm happy to refresh this - just wondering whether to look into Anders' suggestion here? Maybe merge this and review that option later?

[weswigham]

🤷 I don't think we'll take this for the 4.1 RC this week, so there a few weeks you could experiment during until we're ready to start merging things for 4.2.

[sandersn]

Master is open for 4.2 PRs now.

[weswigham]

so @jack-williams you feel strongly one way or another with this now?

[jack-williams]

@weswigham I merged master on this and opened another PR that tried the & Record<..> approach. Are the bot user tests down, or did my change wreck everything? Other PR is here #41478.

Perf looked ok - some changes in the binder but I think they are unrelated.
EDIT: User test baselines seem ok with & Record<..> (haven't inspected them too much)

My preference for in narrowing would be to use & Record<..> - I think it is more useful whilst this is really a patch.

There are some subtleties which arise because in narrowing is always done using the current type, rather than with the declared type like discriminants.

As it is now, the following behave the same (with and without my changes):

type AOrB = { aProp: number } | { bProp: number };
declare function isAOrB(x: unknown): x is AOrB;

declare var x: unknown;
declare var y: AOrB;
if (isAOrB(x)) {
    if ("aProp" in x) {
        x.aProp;
    }
    else if ("bProp" in x) {
        x.bProp;
    }
    else if ("cProp" in x) {
        const _never: never = x;
    }
}


if ("aProp" in y) {
    y.aProp;
}
else if ("bProp" in y) {
    y.bProp;
}
else if ("cProp" in y) {
    const _never: never = y;
}

But it is not the case that similar discrimant code behaves the same:

type A = { kind: 'a' }
type B = { kind: 'b' }
type AorB = A | B;

declare const x: unknown;
declare const y: AorB;
declare function isAorB(x: any): x is AorB;

if(isAorB(x)) {
    if(x.kind === "a") {

    }
    else if (x.kind === "b") {
        x;
    }
    else {
        x // B;
    }
}

if(y.kind === "a") {

}
else if (y.kind === "b") {

}
else {
    y; // never;
}

When you consider having in use & Record you get different behavior when ordering clauses:

declare const w: AOrB;
if ("aProp" in w) {
    w // A
}
else if ("bProp" in w) {
    w // B
}
// w is never from here
else if ("cProp" in w) {
    w
}
else {
    w
}

but check cProp earlier and:

declare const w: AOrB;
if ("aProp" in w) {
    w // A
}
else if ("cProp" in w) {
    w // w is { bProp: number } & Record<"cProp", unknown>
}
else if ("bProp" in w) {
    w // B
}
else {
    w // never
}

• We could make the order not matter by narrowing from the declared type (for unions), but that might be a subtle breaking change for in.
• We could say the order does matter and just deal with the odd cases with the recommendation to check non-mentioned properties first.
• We could never narrow to never, but this would remove exhaustiveness checking for in, which I think could be quite a break?

I think the second option is reasonable but I haven't been able to check user tests yet.

[weswigham]

Narrowing for in is a little... Ideals-based? anyway, so I think whichever pattern matches more real world intent with usage like this is probably better. How we I can't say I've seen many in checks in the real world to build up an intuition of what one might "expect" these checks to imply, so I'm not sure I can be a great judge of which is better. Hm.

[jack-williams]

Would this be a way forward?

The current PR is ready and the other PR (#41478) is nearly ready but needs some tests, to scan through the user tests, and to write up a description of the implemented behavior. I can do that work this week. At that point, it probably comes down to a discussion within the team to see if they want to consider moving forward with #41478, otherwise this can be merged.

Merging this would not preclude #41478 at a later date, I think. In most cases this PR just changes some never's into original types, and #41478 would extend those types with intersections for new properties.

[sandersn]

I agree, let's merge this one now. I'll add #41478 to the design meeting agenda.