[regression 4.8] type narrowed too far; valid cases considered unreachable

[seansfkelley]

Bug Report

🔎 Search Terms

type narrowing assertNever never unreachable

🕗 Version & Regression Information

• This changed between versions 4.7.4 and 4.8.x

⏯ Playground Link

Playground Link: Provided

💻 Code

enum PropertyType {
  String = 'string',
  Number = 'number',
  Unknown = 'unknown',
}

interface Property<Type extends PropertyType, Value extends unknown = unknown> {
  type: Type;
  value: Value;
}

type StringProperty = Property<PropertyType.String, string>;
type NumberProperty = Property<PropertyType.Number, number>;
type UnknownProperty = Property<PropertyType.Unknown, unknown>;

type PropertyObject =
  | StringProperty
  | NumberProperty
  | UnknownProperty

type PropertyObjectOrEmpty = PropertyObject | { type: PropertyType; value: undefined };

interface Edit<VT extends PropertyObject = PropertyObject> {
  type: 'edit';
  property: VT;
}

interface Add<VT extends PropertyObjectOrEmpty = PropertyObjectOrEmpty> {
  type: 'add';
  property: VT;
}

function assertNever(n: never): never {
  throw new Error('nope');
}

function doStuff(action: Edit | Add) {
  if (action.property.value == null) {
    // do something
  } else if (action.property.type === PropertyType.Unknown) {
    // do something else
  } else {
    // this line is reachable, but compiles without error!
    assertNever(action.property);
  }
}

doStuff({
  type: 'edit',
  property: {
    type: PropertyType.String,
    value: 'hello, world'
  }
})

Output

"use strict";
var PropertyType;
(function (PropertyType) {
    PropertyType["String"] = "string";
    PropertyType["Number"] = "number";
    PropertyType["Unknown"] = "unknown";
})(PropertyType || (PropertyType = {}));
function assertNever(n) {
    throw new Error('nope');
}
function doStuff(action) {
    if (action.property.value == null) {
        // do something
    }
    else if (action.property.type === PropertyType.Unknown) {
        // do something else
    }
    else {
        // this line is reachable, but compiles without error!
        assertNever(action.property);
    }
}
doStuff({
    type: 'edit',
    property: {
        type: PropertyType.String,
        value: 'hello, world'
    }
});

Compiler Options

{
  "compilerOptions": {
    "strict": true,
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "alwaysStrict": true,
    "esModuleInterop": true,
    "declaration": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "target": "ES2017",
    "jsx": "react",
    "module": "ESNext",
    "moduleResolution": "node"
  }
}

🙁 Actual behavior

Code compiles without error, but assertNever is hit at runtime.

🙂 Expected behavior

Compilation error: the type of action.property at the point it's passed to assertNever is not never. (4.7 does this.)

[whzx5byb]

A shorter repo:

// @strict: true

type S = 
| { type: 'string', value: string } 
| { type: 'number', value: number } 
| { type: 'unknown', value: unknown }
| { value: undefined };

declare var s: S

if (s.value !== undefined) {
  s;
//^?
// 4.7: var a: S;
// 4.8: var a: { type: 'unknown', value: unknown };
}

4.7 doesn't narrow the type at all, while 4.8 narrows it incorrectly.

[typescript-bot]

👋 Hi, I'm the Repro bot. I can help narrow down and track compiler bugs across releases! This comment reflects the current state of this repro running against the nightly TypeScript.

---

Comment by @whzx5byb

⚠️ Assertions:

• var s: { type: 'unknown'; value: unknown; }

Historical Information

Version	Reproduction Outputs
4.8.2	⚠️ Assertions: var s: { type: 'unknown'; value: unknown; }
4.4.2, 4.5.2, 4.6.2, 4.7.2	⚠️ Assertions: var s: S

[typescript-bot]

The change between 8002369 and v4.8.2 occurred at bdb8514.

[andrewbranch]

@typescript-bot bisect good 8002369 bad v4.8.2

[fatcerberus]

The bot still gets hung up on 5aa0053... it looks like that's going to be an issue if the good is anything prior to that commit forevermore...

[andrewbranch]

I think the test runner can work around it by treating its array of root files as immutable. I’ll do that sometime this week.

#49736 is the alleged new culprit 😮

[andrewbranch]

At a glance, that makes no sense whatsoever

[jakebailey]

I'm not sure what the bot picked up on; when I do a bisect by hand, I get #49119 (which makes a lot of sense). @ahejlsberg

[berickson1]

I think I have a similar repro for this.
Code

// @strict: true

interface SuperType {
  foo: number;
}

type First = Omit<SuperType, 'foo'> & {
  a: string;
}

type Second = Omit<SuperType, 'foo'> & {
  a: string;
  foo: string;
}

function myFunction(props: First): void;
function myFunction(props: Second): void;
function myFunction(props: First|Second): void {
  if (hasProperty(props, 'foo')) {
    //            ^?
    // Props should be narrowed to Second at this point
    // TS 4.8.2 claims props is Second | (Omit<SuperType, "foo"> & { a: string; } & Record<"foo", unknown>) | (Omit<SuperType, "foo"> & { a: string; foo: string; } & Record<...>)
    // TS 4.8.0-beta claims props is Second
    // TS 4.7.4 claims props is Second
    const second: Second = props; // TSS 4.8.2 Error: Types of property 'foo' are incompatible. Type 'unknown' is not assignable to type 'string'.
    console.log(second);
    return;
  }

  // Props should be narrowed to First at this point
  const first: First = props; // TS 4.8.2 claims this is First
    console.log(first);
}

// This function is used for type-narrowing to determine if an arbitrary property exists on an object (and type it as such)
export function hasProperty<T, K extends string | number | symbol>(obj: T, prop: K): obj is T & Record<K, unknown> {
  return obj !== undefined && obj !== null && Object.prototype.hasOwnProperty.call(obj, prop);
}

Playground Link

Tracking issue status for this to drop in nightly

[andrewbranch]

@berickson1 I’m not sure exactly where that changed, but the new behavior is correct. hasProperty(props, 'foo') does not prove that props is a Second. It could be a First & { foo: unknown }.