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

Report errors if union subtype reduction results in fewer types than were explicitly written #3862

Closed
danquirk opened this issue Jul 14, 2015 · 6 comments
Labels
Suggestion An idea for TypeScript

Comments

@danquirk
Copy link
Member

// A function that accepts strings or non-primitive types
function foo(x: Object | string) { ... }
foo('hello'); // ok
foo({ a: 3 }); // ok
foo(1); // ok, meant for this to be an error

Based on examples seen in DefinitelyTyped. Someone misunderstood what Object means, thinking it meant non-primitive types (not an uncommon source of confusion), so they union it with the one primitive type they do want to allow. The compiler performs subtype reduction and ends up with only Object. This is now allowing code the user intended to disallow. At this point if the compiler knows it has reduced an explicitly written union type to a set of types smaller than what was originally written it should just issue an error to alert the user that some part of their type annotation is not having the effect they expect.

@danquirk danquirk added the Bug A bug in TypeScript label Jul 14, 2015
@RyanCavanaugh
Copy link
Member

We should probably do #1809 first so that this code has somewhere reasonable to migrate to?

@danquirk
Copy link
Member Author

The fix here would encapsulate more types of errors than just conflating Objects and primitives so I wouldn't say it's entirely necessary. In an ideal world if this is the majority of cases we see then yeah it might make sense to wait.

On the other hand this would be a breaking change so the longer we wait the more impactful the break becomes.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 12, 2015

@danquirk what is the proposal here, if not #1809?

@danquirk
Copy link
Member Author

That gives people who want this pattern somewhere to go, but it doesn't get them there. If we implement #1809 tomorrow all the places where anyone wrote Object | string are still valid and still unknown bugs. Making this pattern an error would then alert them to their bug, and we could offer a better alternative.

In addition, any case where one union constituent completely captures the set of values meant to be specified by another union constituent are still relevant. A contrived example:

function foo(x: Base | Derived) { ... }
foo(new Derived2());

What was the developer's intention in saying | Derived if it's completely redundant in a union with Base as one of its constituents?

@RyanCavanaugh
Copy link
Member

What was the developer's intention in saying | Derived if it's completely redundant in a union with Base as one of its constituents?

I think that's clear in some contexts, but unclear in others. In the compiler we have some functions like (made up example):

function isExportLike(node: CallExpression|Declaration) { ... }

Then later I made CallExpression extend Declaration. It's not obvious that means isExportLike is now an error.

Similarly, I wanted to write a function:

function isDefineCall(node: Expression|CallExpression) {
  // Pretend we have tagged types for a moment
  if(node.kind === SyntaxKind.CallExpression) {
    // Maybe return true
  }
  return false;
}

This is probably an abuse of union types, but conveys well the intent of the function ("You can pass me anything, but I'm only going to possibly return true for CallExpression") and simplifies its implementation.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Bug A bug in TypeScript labels Oct 6, 2015
@RyanCavanaugh RyanCavanaugh removed the In Discussion Not yet reached consensus label Nov 3, 2015
@RyanCavanaugh
Copy link
Member

Moot now that we don't immediately reduce union types.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants