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

Union Types And Strings #9489

Closed
bolismauro opened this issue Jul 2, 2016 · 15 comments
Closed

Union Types And Strings #9489

bolismauro opened this issue Jul 2, 2016 · 15 comments
Assignees
Labels
Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@bolismauro
Copy link

bolismauro commented Jul 2, 2016

TypeScript Version: 2.0.0-dev.20160702

Code

const t = (a: "a"|"b") => a;
const b = "a";
t(b);

Expected behavior:
No errors

Actual behavior:

src/index.tsx(3,3): error TS2345: Argument of type 'string' is not assignable to parameter of type '"a" | "b"'.

This is just a toy example, and can be fixed with enums or other approaches.
I'm actually having this issue, where tsc gives me a lot of error in the CSS.
For example

src/index.tsx(33,13): error TS2322: Type '{ flex: number; justifyContent: string; alignItems: string; backgroundColor: string; }' is not assignable to type 'ViewStyle'.
  Types of property 'alignItems' are incompatible.
    Type 'string' is not assignable to type '"flex-start" | "flex-end" | "center" | "stretch"'.

Please also note that

t("a");

doesn't give any error, but I can't really use it to solve the react issue.

I'm not sure if there is another issue related to the same problem. I tried to search in the opened issues but I haven't found anything.

@ahejlsberg
Copy link
Member

You need a type annotation to give b a literal type:

const t = (a: "a"|"b") => a;
const b: "a" = "a";
t(b);

I think it is reasonable to require a type annotation to get a literal type for a var or let since you might later mutate the variable, but it does seem a bit silly not to just infer the literal type for a const.

@DanielRosenwasser @mhegazy @RyanCavanaugh I recall discussing this before, perhaps we should consider making this change.

@ahejlsberg
Copy link
Member

Specifically, I think we should add this to #9407 by considering the initializer of a const variable declaration a literal type location.

@bolismauro
Copy link
Author

bolismauro commented Jul 3, 2016

Not sure if this can help, but this is a snippet of the original error

import * as React from 'react';
const { Component } = React;

import {
  AppRegistry,
  StyleSheet,
  Text,
  View
} from 'react-native';

const styles = {
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  welcome: {
    fontSize: 20,
    textAlign: 'center',
    margin: 10,
  },
  instructions: {
    textAlign: 'center',
    color: '#333333',
    marginBottom: 5,
  },
};

class Test extends Component<any, any> {
  render() {
    return (
      <View style={styles.container}>
        <Text style={styles.welcome}>
          Welcome to React Native!
        </Text>
        <Text style={styles.instructions}>
          To get started, edit index.ios.js
        </Text>
        <Text style={styles.instructions}>
          Press Cmd+R to reload,{'\n'}
          Cmd+D or shake for dev menu
        </Text>
      </View>
    );
  }
}

export default Test;

I receive errors when I assign the various styles. Basically every css property that is described as union generates an error like the one in the open post

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Breaking Change Would introduce errors in existing code Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined labels Jul 3, 2016
@DanielRosenwasser
Copy link
Member

The only reason I didn't do this is because it's a breaking change. Either way, someone is going to have to write a type annotation. Given the types of programming const is used for, the ergonomics might win out here, but we should discuss it.

@ahejlsberg
Copy link
Member

ahejlsberg commented Jul 3, 2016

@bolismauro I'm not sure you'll be able to avoid a type annotation. It works if you do the following:

type Styles = {
  container: ViewStyle,
  welcome: TextStyle,
  instructions: TextStyle
};

const styles: Styles = {
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  welcome: {
    fontSize: 20,
    textAlign: 'center',
    margin: 10,
  },
  instructions: {
    textAlign: 'center',
    color: '#333333',
    marginBottom: 5,
  },
};

Once you add this type annotation everything works because we have a contextual type for each of the properties. (Plus of course you get completion lists when writing the object literal and errors if you write the wrong values.)

Even with the proposed change to consider the initializer for a const a literal type location, we couldn't trasitively to the same for properties in an object initializer because there is no indication that they are const. Specifically, you might want to assign a new value to styles.container.alignItems, but you wouldn't be able to if we inferred type "center" for it.

@ahejlsberg
Copy link
Member

@bolismauro Alternatively you could use the as type assertion operator:

const styles = {
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  } as ViewStyle,
  welcome: {
    fontSize: 20,
    textAlign: 'center',
    margin: 10,
  } as TextStyle,
  instructions: {
    textAlign: 'center',
    color: '#333333',
    marginBottom: 5,
  } as TextStyle,
};

@bolismauro
Copy link
Author

@ahejlsberg your explanation makes absolutely sense.
I'll use the as operator to fix the problem, thanks!

@DanielRosenwasser
Copy link
Member

Should we close this?

@joewood
Copy link

joewood commented Aug 29, 2016

@ahejlsberg your comment #9489 (comment) - was this ever discussed in more detail? Is there a reason why const types aren't narrowed to their literal type?

@DanielRosenwasser
Copy link
Member

@joewood mainly because it breaks this

const x = "a";
let y = x;
y = "b"; // error! "b" not assignable to "a"

But it's sort of been on the table ever since we first discussed it.

@joewood
Copy link

joewood commented Aug 29, 2016

Thanks @DanielRosenwasser, wasn't the original rules set out to widen when used to assignment to let/var as per your #6167 (comment)? I understand this one is tricky though and I'm still reading through all comments....

@DanielRosenwasser
Copy link
Member

The thing is that much of the time you'd end up needing to annotate types anyway. For instance, a string literal in a property declaration would be widened to string:

interface Subscribe { actionType: "Subscribe"; email: string}
// ...
type Action = Subscribe | ChangeEmail | Unsubscribe
declare function processAction(a: Action): void;

const x = {
    actionType: "Subscribe",
    email: "[email protected]",
}

// Oops! x has type '{ actionType: string; email: string }'
processAction(x);

So it was arguable how much better it was, because that is clearly one of the cases we really cared about anyway.

It was also incredibly complex, subtle, and sort of ad hoc. Imagine trying to explain the rule set to users of why their literal types disappeared in this one place but not another.

I also dug up some notes on a gist from way back about this: https://gist.github.com/DanielRosenwasser/57f111bcb5f52457cc6d

@joewood
Copy link

joewood commented Aug 29, 2016

But, in your example above, the const only applies to x and not actionType. So I would argue that the compiler is right to error against this, actionType can be changed to anything. Some control flow analysis may enable some narrowing on this, but I would be happy to force a cast, changing "Subscribe" to "Subscribe" as "Subscribe".

Realize this problem is multi-faceted, but I think making it more consistent makes it simpler to explain. From your gist, problem 1 seems to be solved by the widen on mutable binding rule, and problem 2 seems to be solved by the use the most wide comparison type rule.

@ahejlsberg
Copy link
Member

Possible fix for this issue in #10676.

@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Breaking Change Would introduce errors in existing code labels Sep 20, 2016
@mhegazy mhegazy added this to the TypeScript 2.1 milestone Sep 20, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Sep 20, 2016

Original issue should be addressed by #10676

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants