Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Narrowing on assignment does not remove weak types #26405

Closed
mattmccutchen opened this issue Aug 13, 2018 · 5 comments
Closed

Narrowing on assignment does not remove weak types #26405

mattmccutchen opened this issue Aug 13, 2018 · 5 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@mattmccutchen
Copy link
Contributor

Filing an issue to track the regression from #26143 described in #26143 (comment).

TypeScript Version: master (fe387cc)

Search Terms: narrow assignment weak type assignable comparable

Code

type AOrArrA<T> = T | T[];
const arr: AOrArrA<{x?: "ok"}> = [{ x: "ok" }]; // weak type
arr.push({ x: "ok" });  // Error: Property 'push' does not exist on type 'AOrArrA<{ x?: "ok"; }>'.

Expected behavior: No error.

Actual behavior: Error as indicated above.

Playground Link: N/A, playground is not affected

Related Issues: #26143

@RyanCavanaugh
Copy link
Member

@ahejlsberg tentatively tagging Bug but we'll figure out what we really want to do

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Aug 13, 2018
@mattmccutchen
Copy link
Contributor Author

Continuing the work toward a solution from #26143 (comment):

I tried:

  • Narrowed type = intersection of declared and assigned types: Fails with let foo: Foo[] = [], where the narrowed type is never[] even after a push.
  • Narrowed type = minimal sub-union of declared type to which assigned type is assignable: Picks an arbitrary union constituent when the assigned type is any. Even if we fixed that somehow, let foo: {a: number} | {b: number} = {a: 1, b: 2} would arbitrarily pick one of the two union constituents, an undesirable loss of symmetry.

So I think it's most practical to just fix weak types. To recap, we want to (1) decompose unions on the source side, (2) consider the constraints of indexed access types on the source side and decompose unions again after doing so, and (3) check for weak types. The old implementation did (1) and (3) but not (2); the one from #26143 does (1) and (2) but not (3). ISTM it's going to be awkward to get (2) without building on the existing checkTypeRelatedTo code. Thus, I think the best way we can get (1), (2), and (3) is to add a new relation that is the same as the comparable relation but checks for weak types; I propose to name it the "narrowable" relation. I'll submit a PR to do this. I hesitate to add a new relation, but it's just a matter of replacing each of the ~10 occurrences of relation === comparableRelation with relation === comparableRelation || relation === narrowableRelation except for the one for weak types.

mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Aug 13, 2018
types.

Fixes microsoft#26405.

Also add a debug assert to catch any future cases like microsoft#26130 in which
the target type of a valid assignment is narrowed to something to which
the source type is no longer assignable.
@mattmccutchen
Copy link
Contributor Author

Proposed fix is at #26425.

@RyanCavanaugh
Copy link
Member

Per #26518, we'll revert the changes in #26143

mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Aug 18, 2018
…microsoft#26130 by

skipping narrowing if the old algorithm produces a type to which the
assigned type is not assignable.

This also means we'll no longer narrow for erroneous assignments where
the assigned type is not assignable to the declared type.  This is the
reason for the numericLiteralTypes3 baseline change.

Fixes microsoft#26405.
@mattmccutchen
Copy link
Contributor Author

mattmccutchen commented Aug 18, 2018

Wait, I have another idea: use the old algorithm but if it messes up and produces a narrowed type to which the assigned type is not assignable (as in #26130), then skip narrowing and use the declared type. I updated #26425.

By the way, another example that confuses the old algorithm (playground):

// Enable strictNullChecks
function test<T extends number | string>(arg: null | T) { 
  let x: null | number | string = arg;
  if (x == null)
    return;
  x;  // never
}

@RyanCavanaugh RyanCavanaugh added the Fixed A PR has been merged for this issue label Aug 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

3 participants