microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.84k stars 12.46k forks source link

Assignment operators allow assigning invalid values to literal types #47027

Open MartinJohns opened 2 years ago

MartinJohns commented 2 years ago

Bug Report

πŸ”Ž Search Terms

πŸ•— Version & Regression Information

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

type Data = { value: 'literal' }

const data: Data = { value: 'literal' }
data.value += 'foo' // Appending a string is possible, despite it being a string literal type.

// data is still of type Data and can be passed along,
// even when the value has the wrong type.
useData(data);

function useData(_: Data): void {}

The same applies to any literal type (e.g. number). See #48857 for more examples.

πŸ™ Actual behavior

Appending a random string to the property typed as the literal "literal" is possible.

πŸ™‚ Expected behavior

I would expect an error, saying that the the type "literalfoo" is not assignable to "literal".


Ping @S0AndS0

S0AndS0 commented 2 years ago

Here's yet another related example of odd string literal type behavior;

let example_one = 'first' as const;
example_one += ' append';
//> No error?!

From what I understand the act of _let example_one = 'first' as const;_ is similar to defining a custom type, eg.

type name = 'second';
let example_two: name = 'second';
example_two += ' append';

Because when I inspect the TypeScript type of `example`_ on assignment lines they're string literals;

let example_one = 'first' as const;
//> example_one: "first"

let example_two: name = 'second';
//> example_two: "second"

... But inspecting TypeScript types after +=, the types of `example_ are transmuted tostring`

example_one += ' append';
//> example_one: string

example_two += ' append';
//> example_two: string
tshinnic commented 2 years ago

Much confusion here shown in initial report.

I'm sure there are more specific doc mentions, but regarding objects this mentions:

When you initialize a variable with an object, TypeScript assumes that the properties of that object might change values later.

That is, something like this is not protected and produces no errors:

  const req1 = { url: "https://example.com", method: "GET" };
  req1.url += 'toobad'

Same doc mentions later that a strange construct like this will protect against property modification:

  const req2 = { url: "https://example.com", method: "GET" } as const;
  req2.url += 'toobad'
  // Cannot assign to 'url' because it is a read-only property.

But there it is the entire object literal that is being declared constant (or actually the properties 'readonly')

In the second comment I was struck by:

  let example_one = 'first' as const;
  example_one   += 'hiya'

Consider the difference between that and

  let example_one = ('first' as const);
  example_one   += 'hiya'

There is no difference. Neither generates an error.

You have marked the literal string 'first' as constant. And every string literal is constant. You have said nothing about the variable example_one. Changing that to:

  const example_one = ('first' as const);
  example_one   += 'hiya'
  //  Cannot assign to 'example_one' because it is a constant.

fails as expected because now the variable is marked as constant.

I'm afraid I see nothing surprising here (except the { url: "https://example.com", method: "GET" } as const; wow).

Please consult the docs again. Each time more things make sense.

MartinJohns commented 2 years ago

@tshinnic You missed the point of the issue by a staggering degree. There's absolutely no confusion present, and I'm very aware that the object is mutable. The point is about the += operator allowing to assign incompatible types. Perhaps you should read the documentation about literal types.

You also don't seem to know what const contexts (aka as const) do. The as const will narrow the type of the value, so the type will be "first", and not string. Here's an example that will hopefully illustrate the issue more:

// No difference between these two:
let a = 'abc' as const;
let b: 'abc' = 'abc';

// This is an error.
a = 'foo'

// This is not an error, but IMO should be:
a += 'foo'

Or to make a more elaborate demonstration with objects using your example:

type MyRequest = { url: string, method: 'GET' | 'POST' }
const req: MyRequest = { url: '...', method: 'GET' }

// This is okay, the assigned value matches the type:
req.method = 'POST'

// This is an error, the assigned value does not match the type:
req.method = 'EXAMPLE'

// Why is this not an error? The assigned value does not match the type.
req.method += 'EXAMPLE'
tshinnic commented 2 years ago

Oh good, a teachable moment awaits.

S0AndS0 commented 2 years ago

Personally I'd expect to have to leverage something like & to allow for appending to a literal, for example something like;

type appendable<P, T> = P & T;

interface Data_One {
  value: appendable<'prefix', string>;
}

const data_one: Data_One = { value: 'prefix' };

data_one.value += ' append';

... And for TypeScript to throw an error for previously posted examples.

But at this time even appending types such as numbers are allowed, eg.

type Data_Two = { value: 'prefix' }

const data_two: Data_Two = { value: 'prefix' };

data_two.value += 1;
MartinJohns commented 2 years ago

@S0AndS0 That would be `prefix-${string}`:

type MyString = `prefix-${string}`

let test: MyString
test = "prefix-abc"
test = "prefix-foo"
test = "error"
RyanCavanaugh commented 2 years ago

See also https://github.com/microsoft/TypeScript/issues/14745#issuecomment-459881335

This is a bit of a weird spot. Mutation of a value of a single literal type is clearly wrong, but also unlikely to be something you do accidently, so it's a low-value error despite high confidence. However, mutation of a value of a union of literal types is likely to be correct in a way that we can't statically verify, so it's a false positive.

The natural way to split that would be to say that mutation of non-union literals is disallowed but mutation of union literals is "presumed OK", but this is a subtyping violation because an operation that's allowed on T | U should also be valid on a T or a U.

This code smells really bad, though, and it'd be nice to find a more reasonable solution.

OT: To whomever it may concern, please take my personal ensurances that OP understands what they are talking about.

MartinJohns commented 2 years ago

See also #14745 (comment)

I just knew I have seen this issue before, but just couldn't find it. increment was the missing keyword here.

fatcerberus commented 2 years ago

However, mutation of a value of a union of literal types is likely to be correct in a way that we can't statically verify, so it's a false positive.

I would personally be very suspicious of any code that switched between members of a finite set of known string literals by way of mutation rather than direct assignment and wouldn’t mind the (hypothetical) error that disallowed it.

My opinion of this pattern for number literals is admittedly more fuzzy though, given that TS doesn’t have range types.

fwienber commented 2 years ago

I would like to contribute a few more examples that illustrate that assignment operators and increment/decrement operators (that is, all modifying operators) are currently typed inconsistently in TypeScript. I also put them in a TypeScript Playground. The first two examples are taken from my own attempt at a bug report about assignment operator typing (#48857), which I closed to continue the discussion here.

Example 1: Assignment Operators Bypass Range (Union) Type Check

The first example shows a typical way to define and check an integer range type, but the attempt to enforce calling the coercion function is undermined by using assignment operators.

type ZeroToThree = 0 | 1 | 2 | 3;
// a function that coerces any given number to the integer range 0..3:
const ZeroToThree = (value: number): ZeroToThree => (value >>> 0) % 4 as ZeroToThree;

let z: ZeroToThree = 3;
const y: ZeroToThree = z + 1; // error, good, because + always results in a 'number', not necessarily in range of ZeroToThree
const x: ZeroToThree = ZeroToThree(z + 1); // type-safe _and_ run-time-safe!
const w: ZeroToThree = z - 1 as ZeroToThree; // if you know what you're doing and want to avoid run-time overhead, use 'as'
z += 1; // no error, bad, because a is now out of range (4)
z *= 2; // any numeric operator can be used to bypass the range check
console.log(z); // z is now 8 :-(

Example 2: Increment/Decrement Operators Bypass Type Check

The second example is based on the pattern to implement "tagged" number types, here an "unsigned integer" type.

type uint = number & {__uint__: never};
// a function that coerces any given number into an unsigned 32 bit integer:
const uint = (value: number): uint => (value >>> 0) as uint;

let a = uint(0);
const b: uint = a - 1; // type error; good: you are forced to use the coercion function!
const c: uint = uint(a - 1); // good: no error when using coercion function (leads to MAX_UINT = 4294967295)
let d = uint(1); // good: type inference works: hover over 'd' shows 'const d: uint'
d -= 1; // here, TypeScript correctly complains "Type 'number' is not assignable to type 'uint'." 
--d; // bad: no type error, although it behaves the same as the line above
d--; // bad: no type error, too: 'd' is an uint, but becomes -2 :-(
const e: uint = --d; // good: type error, the _result_ of --d is correctly typed as 'number'
const f: uint = d++;  // bad: type error, although the result of post-increment is the old value, declared as 'uint'

What is really strange is that d -= 1 and --d, which is literally the same thing, behave differently. Furthermore, the last line shows that postfix operators are typed incorrectly. It could be argued whether d++ should be allowed at all, but the error message TypeScript gives, "Type 'number' is not assignable to type 'uint'. (2322)" is clearly wrong, because the result of d++ is the old value of d and thus has the type of d, which is uint, not number.

Example 3: No Type Narrowing When Using +=

The third example shows that assignment operators, unlike their expanded equivalent syntax, do not support type narrowing:

function works(param: string | number): string {
  param = param + "a";
  return param; // accepted by type checker, because 'param' is narrowed to 'string'
}

function shouldAlsoWorkButDoesNot(param: string | number): string {
  param += "a";
  return param; // type error because 'param' is _not_ narrowed to 'string'
}

This makes things ever weirder, because unlike the previous examples, where incorrect assignments were accepted, here, the correct return expression is rejected.

Conclusion

The examples show that the issue is not about literal types, union types, or as const, but about typing assignment operators and increment/decrement operators consistently with their expanded syntax.

My suggestion would be to type-check assignment operators and increment/decrement operators exactly according to their expanded syntax/semantics:

The special treatment of post-increment/decrement actually makes a difference, as can be seen in the last line of example 2.

fwienber commented 2 years ago

Consequences: The suggested stricter typing would of course lead to most usages of assignment operators, except on numbers and += on strings (which, then again, taken together hopefully cover most usages), being reported as type errors. You would have to change your code to the expanded syntax and add a type assertion or a conversion / coercion function call. For example, using the uint declaration from example 2 above, in

for (let i = 0 as uint; i < 10; ++i) { ... }

++i would now be a type error, so it would have to be changed into

for (let i = 0 as uint; i < 10; i = uint(i + 1)) { ... }

Because you know that i will never leave the uint range and to save run-time overhead, you may want to use a type assertion instead. Unfortunately, ++i as uint would only add a type assertion for the result of the expression, not for the value that is assigned to i, and so, according to the new type checks, would still result in an error. But you can use

for (let i = 0 as uint; i < 10; i = i + 1 as uint) { ... }

Since changing your code like that can result in quite some work, like for other breaking changes in TypeScript, new compiler flags should be introduced to (de)activate the stricter checks, something like strictAssignmentOperatorTypes and strictUpdateExpressionTypes (still looking for a good name that would include both, something like strictModifyingExpressionTypes).

fwienber commented 2 years ago

A few corrections of my suggested new typing rules:

  • ++i [...] treated like i = i + 1

This is not precise, as in ECMAScript, the increment operator does not use the overloaded + operator, but always returns a number (or bigint):

var foo = "a";
++foo; // the expression result as well as the new value of foo is NaN, not "a1"
var bar = "b";
bar += 1; // the expression result as well as the new value of bar is "b1"

So in this case, ++ and += 1 really behave differently!

  • i++ [...] treated like (() => { const result = i; i = i + 1; return result; })()

This is also not correct, as in ECMAScript, the coercion to number (or bigint) is also applied on the expression result (see for example ecma262, 13.4.2.1):

var foo = "a";
foo++;
// foo is NaN, but the expression result is also NaN, not the old value "a"!

So the fix would be:

  • i++ [...] treated like (() => { const result = Number(i); i = result + 1; return result; })()

But since additionally, TypeScript does not allow usage of arithmetic operators on anything non-numeric, the two rules about increment/decrement operators can be corrected and combined to

Let's have a look at the special case that the operand has type any. The resulting type of the increment/decrement expression is known to always be number | bigint:

(value: any)  => {
  const a: number | bigint = ++value;
  const b: number = ++value;
  const c: bigint = ++value;
}

In the example, TypeScript currently shows a type error only for the last line, because it infers the type of ++value to be number, not number | bigint. The expected behavior would be that both the second and the third line show a type error.

fwienber commented 2 years ago

After adding several examples for inconsistent / undesirable behavior of assignment/increment/decrement operators and suggesting an approach for a possible solution, I'd be glad to hear some feedback by the TypeScript community and/or maintainers, if you think this is valid and viable...

fwienber commented 2 years ago

Btw., the "uint" example was inspired by https://spin.atomicobject.com/2018/11/05/using-an-int-type-in-typescript/ .

fwienber commented 2 years ago

After learning details about ECMAScript Update Expressions (prefix / postfix increment / decrement operator), it seems to me they are quite a different kind of beast, so I would like to pull out the corresponding TypeScript issues I found into a separate issue. I think this would be a good separation of concerns and allow to return focus to Assignment Operators in this issue, as the title and original example suggest. Please let me know if you think that this is a bad idea, ideally before I take the effort to write the new issue.

fwienber commented 2 years ago

@ahejlsberg https://github.com/microsoft/TypeScript/issues/14745#issuecomment-459881335

I don't think it is feasible to just error here--after all, it is perfectly fine to increment a value of type 0 | 1 | 2.

If this is so, then why does

let x: 0 | 1 | 2 = 0;
x = x + 1;

result in a type error, namely Type 'number' is not assignable to type '0 | 1 | 2'. (2322)?

I strongly suggest assignment operators and update expression should result in the same type errors as semantically equivalent expressions.

Rather, in order to do better we would have to support "type math", [...] We have previously decided not to do this because the added complexity (conceptual and in implementation) doesn't justify the marginal gains in type checking. I think this decision still holds, so I'm not sure we really want to do anything here.

I don't want any "type math" or added complexity, I just want consistency, and for that, have the option to handle assignment operators and update expressions in a strict way, i.e. be treated just like semantically equivalent expressions.

fwienber commented 2 years ago

Here is another TypeScript playground with consolidated, concise examples of everything that goes wrong with assignment operators and update expressions. All lines that correctly produce errors are commented, so that it is obvious that running "green" code results in unexpected, contradictory output. If you want to see those correct type errors, just un-comment the corresponding lines (but of course these change the output when running the code).

fwienber commented 2 years ago

Is there no interest in this topic, or just no attention because this issue is not new? Or is it just that at the moment, the TypeScript team is under release stress (4.7)? My first attempt to "revive" this issue is now 18 days old, and there has not been a single reaction by anyone since then. Does anybody have tips how to raise attention ("bump" issue)? I really think there are serious flaws in the way TypeScript currently handles assignment operators and update expressions, however these seem like low-hanging fruit to fix to me and I already sketched a possible solution.

fwienber commented 2 years ago

I think with all the examples of inconsistent behavior provided in this issue, it at least deserves a "Bug" label.

fwienber commented 2 years ago

Since the label "Suggestion" seems to cause this issue to stay under the radar, I extracted one of the most obvious, concrete bugs into a new "Bug" issue: #49558

MartinJohns commented 2 years ago

Bug means it's supposed to work a specific way, but it doesn't. This is not the case here. This behaviour was never supposed to work this way, it was never implemented. It unsound behaviour, yes, but not a bug.

This issue doesn't get a lot of traction because it's a corner-case. Hardly anyone ever stumbles upon this problem. The team has limited resources that are better spent on more common features and issues. And while this issue is not nice, having a sound type system is explicitly a non-goal.

fwienber commented 2 years ago

That's why instead of insisting on labelling this issue a "Bug", I created a new issue with one of the examples where the type system is not only non-sound, but downright contradictory and clearly wrong/outdated with respect to ECMAScript semantics. Regarding #49558, TypeScript does not ignore the topic, but explicitly implements a rule that says "applying update expressions on any results in a number", which once was true, but with the addition of bigint to ECMAScript no longer is. TypeScript claims to support bigint since version 3.2, so I consider this outdated, incomplete implementation a bug. You are definitely right this is a corner case, but then again, bigint and ++/--/- are built-in, well-defined, basic features of ECMAScript I'd expect to be fully supported by TypeScript, so I was quite surprised to encounter such strange behavior.