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

[Suggestion] Disallow literal types to be asserted to a different literal type of the same widened type #14156

Closed
gcnew opened this issue Feb 17, 2017 · 10 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@gcnew
Copy link
Contributor

gcnew commented Feb 17, 2017

TypeScript Version: nightly (2.3.0-dev.20170217)

The problem

Currently it is possible to assert an expression of one literal type to be of a different literal type.

const True = false as true;
const one = 2 as 1;
const str = 'hello' as 'str';

This is confusing and obviously incorrect. It also diverges from the behaviour exhibited when checking literal properties and array elements. The following cases raise errors as expected.

['hello'] as ['str'];          // Type "hello" is not comparable to type "str"
({a: 'hello'} as {a: 'str'});  // Type "hello" is not comparable to type "str"

It's been written many times that assertions provide a mechanism to either upcast or downcast, however in the specific case of mismatching literal types, the operation is actually coercion because they are on the same level.

Proposal

No literal widening should be carried out prior to the assertion compatibility check iff the expression type is a string, number or boolean literal and the assertion type is

  • a literal of the same base type
  • or, a union or intersection containing a constituent of the same base type

Examples

false as (false | string); // OK
false as (true | string);  // Error
false as (boolean | string);  // OK

'hello' as 'str'; // Error
'hello' as 'hello'; // OK
'hello' as ('str' | 123); // Error
'hello' as ('hello' | 123); // OK
'hello' as (1 | 2 | string); // OK
'hello' as ('str' & { _brand: any }); // Error
'hello' as ('hello' & { _brand: any }); // OK
'hello' as ('str' & { _brand: any } | 1); // Error
'hello' as ('hello' & { _brand: any } | 1); // OK
@aaronbeall
Copy link

aaronbeall commented Mar 23, 2017

Just ran into this specifically with string literals and was about to log an issue. I was just trying to useas to keep a value passed to a string argument (from external library) to match a known set of values used throughout my code (its an error otherwise):

type MyValues = "a" | "b" | "c";

declare function externalUtil(s: string): string;

externalUtil("a" as MyValues);
externalUtil("A" as MyValues); // expected validation, got none

Upvoted. :)

@gcnew
Copy link
Contributor Author

gcnew commented Mar 23, 2017

I've always been worried that I might make an error while typing the discrimination string twice in ad-hoc unions. However, the turning point for me was the realisation that we can easily achieve compile-time nameof with a simple nameof type operator that just creates a string literal type, while the actual runtime expression is hand-coded by the programmer.

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Mar 23, 2017
@RyanCavanaugh
Copy link
Member

This is allowed because we widen the type of the expression before testing if it's comparable to the asserted type. Widening is important so that null / undefined can be asserted to other types but it's not clear that it's the right thing to do for literal types. We could potentially just not widen literals when checking an assertion.

@gcnew
Copy link
Contributor Author

gcnew commented Apr 13, 2017

Just for clarification: null and undefined cannot be asserted to other types without a double cast or a "not null" assertion. It's the current behaviour - no changes there.

null as 1;        // error in strictNullChecks
null! as 1;       // OK, even in strictNullChecks
null as any as 1; // also OK

Widening is sometimes beneficial and should be applied, e.g. when faking nominal types with brands.

'/dev/null' as (string & { _pathBrand: any }); // currently OK and should be OK

Widening is counter intuitive when asserting two distinct literal types, if both types have the same base type.

'hello' as 'goodbye'; // currently allowed, but it shouldn't!
123 as true;          // different base types - error in before and after

@tikotzky
Copy link

asserting false as true is currently useful for getting the return type of a function.
see http://www.johnfn.com/ts-best-line.html for an explanation.

@gcnew
Copy link
Contributor Author

gcnew commented Apr 20, 2017

You can use a double cast when you are intentionally cheating false as any as true.

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug and removed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels May 8, 2017
@RyanCavanaugh
Copy link
Member

Discussed and overall we think this behavior is preferable to the alternative. In general we allow "plausible" type assertions and a literal-to-literal cast within the same primitive type qualifies under that metric. Somewhat frequently you have to do this when a side effect unknown to the compiler changes a previously-guarded symbol, e.g.

var x = true;
// ...
mutateXfromAfar();
// hit some specific overload
var j = doSomething(x as false);

Double casting is always an option but just seems too cumbersome here; I think as much as possible you should always be able to write non-suspicious code without ever having to double-cast.

@gcnew
Copy link
Contributor Author

gcnew commented May 8, 2017

Casting to a different literal definitely falls in the category of suspicios, imo. Casting to base - sure. Calling apples oranges can't be right.

@forivall
Copy link

If someone knows of a tslint rule that checks for this, that would be valuable, and it would be appreciated if it was linked here!

@ajafff
Copy link
Contributor

ajafff commented Apr 10, 2018

I have an initial implementation of a lint rule to checks exactly what is described in this issue.
Please have a look at this PR fimbullinter/wotan#179 and leave some feedback. The interesting parts are the test baselines which are the the first 2 files in the change list.

Be aware that this is not a TSLint rule. It's a rule for the Fimbullinter project (which might supersede TSLint in the future). So you need to use yet another linting tool to detect some real bugs.
You won't regret giving it a try as there are even more really useful rules available and it can also execute TSLint rules or emulate TSLint.

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants