search

microsoft / TypeScript

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

exactOptionsPropertyTypes - anomolous behavior with Tuples, options, and spreading. #45764

Open craigphicks opened 3 years ago

craigphicks commented 3 years ago

Bug Report

🔎 Search Terms

🕗 Version & Regression Information

⏯ Playground Link

https://www.typescriptlang.org/play?exactOptionalPropertyTypes=true&ts=4.4.2#code/ATAuE8AcFNgFQIzALzANoA8BcA7ArgLYBG0ATgDTAYD8WRA9vQDbQCGOAugFAgDG9OAM6gwCBFkQp0CcnhwATaADMAljmjyOwAPTbgAUQAaAYX0AZM-oBycAIS3gAdxWgAFsGgZWvUAHlIoCoCrEwACqT0MKQQcFDQgligpHiwbiqCwCS8rASw6cDsHqQRpAB0wDzA-EIioAgATBJIqGgySSncfALCogDMTVKt3JUQMPD1gwDkAHqT5KULiOSTACSTnVXdtfXicBMtM3NtydDLaxwA3JXVPaD1jXtTs+Qycoqq6vJn6xc6ejj0MikYAAWmAxjMAEEAJIAWVBYFc+QAygAJXwAVTMABFMrBCkD6KRrlswPV+o8Ds8ZKsfn8isCwRCYfCwWkMmjMTjgFZfHA8QUcAyiRUgA

💻 Code

  type T1 = [x:number, x?:boolean]
  const t11:T1 = [1,undefined] // EXCELLENT!! with exactOptionalPropertyTypes:true this became is an error. 
  const t12:T1 = [1,true]
  const t13:T1 = [1]

  type T2 = ['^',...T1,'$']
  const t21:T2 = ['^',1,true,'$'];
  const t22:T2 = ['^',1,undefined,'$']; // noerr - CLAIM - this SHOULD be an error
  const t23:T2 = ['^',1,'$']; // err - CLAIM - this SHOULD NOT be an error

🙁 Actual behavior

In the above code, variable declaration for t22 is NOT an err. In the above code, variable declaration for t23 IS an err.

🙂 Expected behavior

In the above code, variable declaration for t22 IS an err. In the above code, variable declaration for t23 is NOT an err.

Even though with exactOptionalPropertyTypes , the T1 now no longer considers [number,undefined] to be a legal assignment, when T1 is spread into T2 the old behavior is used instead of the new behavior.

The documentation for exactOptionalPropertyTypes says

In TypeScript 4.4, the new flag --exactOptionalPropertyTypes specifies that optional property types should be interpreted exactly as written, meaning that | undefined is not added to the type:

MartinJohns commented 3 years ago

However, although there was hope it would be fixed with 4.4's exact exactOptionsParameters, it was not.

This is the first time I ever heard of this flag. It's not mentioned in the 4.4 release notes. Where did you find this?

craigphicks commented 3 years ago

@MartinJohns - exactOptionsPropertyNames.

Despite the name, it apparently applies to Tuples as well. From the link given in the 4.4 documentation to the related pull

In --strictOptionalProperties mode, optional elements in tuple types are strictly checked when they don't explicitly include undefined in their type. For example:

// Compile in --strictOptionalProperties mode
function f4(t: [string?]) {
let x = t[0];  // string | undefined
t[0] = 'hello';
t[0] = undefined;  // Error, 'undefined' not assignable to 'string'
}

It appears the name changed from strictOptionalProperties to exactOptionalPropertyTypes. (There never was a strictOptionalParameters.)

craigphicks commented 3 years ago

@andrewbranch thank you

andrewbranch commented 3 years ago

FWIW, that JSON dump of type info is not from our compiler API. I’ve never heard of properties like discTypeFlag and ctypes. It would be sort of helpful to me if this issue were focused on the tuple thing and the issue about what I assume is someone’s 3rd party compiler API were relocated somewhere else.

craigphicks commented 3 years ago

@andrewbranch - Sure - I'll take it out. Should I remove that whole final section "Tangent topic yet critical - Type API may have changed - is it still possible to determine behavior from exposed Type API ?"

It's not third party API or a compiler. It's from code I wrote displaying the information obtained via getTypeOfSymbolAtLocation and getDeclaredTypeOfSymbol, which are part of the (bleeding edge) typescript compiler API. That format you see is that information slightly massaged to take up less space, without losing anything critical. There is a verbose mode which outputs raw-ish information.

(... Tangential information describing motivation for using compiler API's getTypeOfSymbolAtLocation and getDeclaredTypeOfSymbol removed ... )

That final section of my post is about that compiler API - perhaps - no longer enough info to predict what will pass the compiler in some instances (optional vs. explicit undefined).

craigphicks commented 3 years ago

@andrewbranch - Here is a link to the rawish output. The discTypeFlag is still there but it is just taken from ts.Type['flags']. Sometimes (rarely) more than one flag bit is set, but an unambiguous value is required for discriminating types for processing.

In the out you see some places where r: and c:fields exist side by side.ris for raw. c is the massaged and shortened form, which may be ignored.

andrewbranch commented 3 years ago

Yeah, it would be better if an API request were split into a separate issue please. And it would be easier for me to evaluate without it being intercepted by “a generalized type explorer, which is a component of a typescript Type to schema translator.” If you’re not using the compiler API directly, I can’t tell whether the compiler API is actually deficient.

craigphicks commented 3 years ago

@andrewbranch Removed.

getTypeOfSymbolAtLocation and getDeclaredTypeOfSymbol are both member functions of checker, and they are both exposed as part of the compiler API. For example, getTypeOfSymbolAtLocation is introduced on the Typescript github page "Using the Compiler API".

But I think that is not the Compiler API you mean right now. You mean using the compiler API exposed either through tsc or ts.createProgram, ts.getXXXDiagnostics and program.emit, which is 99.9% of usage. I agree 100%.

andrewbranch commented 3 years ago

No, I’m familiar with the full checker API, and I’m sympathetic to ensuring it’s usable, it just doesn’t seem relevant to this issue and I want to evaluate it without it having been serialized into a different structure with different property names by some mysterious 3rd party tool.

craigphicks commented 3 years ago

@andrewbranch - You are absolutely correct that it is a separate issue and I think my judgement was poor. (I've already figure how to solve that separate API issue so don't worry).

About the problem at hand I made some more examples around the strange result.

type T1 = [x:number, x?:boolean]
  const t11:T1 = [1,undefined] // EXCELLENT!! with exactOptionalPropertyTypes:true this became is an error. 
  const t12:T1 = [1,true]
  const t13:T1 = [1]

 type T2a = [...T1];
  const t2a1:T2a = [1,true];
  const t2a2:T2a = [1,undefined]; // err - ok
  const t2a3:T2a = [1]; // noerr - ok 

 type T2b = ['^',...T1];
  const t2b1:T2b = ['^',1,true];
  const t2b2:T2b = ['^',1,undefined]; // err - ok
  const t2b3:T2b = ['^',1]; // noerr - ok 

  type T2 = ['^',...T1,'$']
  const t21:T2 = ['^',1,true,'$'];
  const t22:T2 = ['^',1,undefined,'$']; // noerr - CLAIM - this SHOULD be an error
  const t23:T2 = ['^',1,'$']; // err - CLAIM - this SHOULD NOT be an error 

  type T3 = [x:number, x:boolean]|[x:number]
  type T4 = ['^',...T3,'$']
  const t41:T4 = ['^',1,true,'$'];
  const t42:T4 = ['^',1,undefined,'$']; // err - ok
  const t43:T4 = ['^',1,'$']; // pass - ok 

type T4eq = ['^',...[x:number, x:boolean],'$'] | ['^',...[x:number],'$'] 
  const t41eq:T4eq = ['^',1,true,'$'];
  const t42eq:T4eq = ['^',1,undefined,'$']; // err - ok
  const t43eq:T4eq = ['^',1,'$']; // pass - ok

Out of T2a,T2b, T2, only T2 is failing.The difference with T2 is that it has an element after the variadic. Whether there is an another element before the variadic doesn't seem to make a difference.

The ones that work correctly (T2a,T2b) (and T4 and T4eq) all make unions at the highest level. T4eq is literally a union at the highest level, the others look like that internally.

playground

Andarist commented 1 year ago

The OP claims that the type produced by this spread is incorrect:

type T1 = [x: number, x?: boolean];
type T2 = ["^", ...T1, "$"]; // ["^", number, boolean | undefined, "$"]

The latter examples imply that the expected behavior would be to produce ["^", number, boolean?, "$"] but that is an impossible type (A required element cannot follow an optional element.(1257)).

When spreading a tuple without adding extra trailing elements the original optional flag is preserved (and not expanded into T | undefined). So I'm really not sure if there is anything actionable here. I think that the issue could be closed.

RyanCavanaugh commented 1 year ago

... but that is an impossible type

Yeah, there are a couple things wrapped up into one here

The first one is effectively a feature request, but not one we'd be likely to pick up since I think it'd have serious combinatorial explosion problems.

Re second one, the current behavior is maybe the best approximation of what we could do. I think there's an argument to be made that under EOPT, tuple spreads should behave as if you had written

  type T2 = ['^',...Required<T1>,'$'];
  const t21:T2 = ['^',1,true,'$'];
  const t22:T2 = ['^',1,undefined,'$']; // should be + is an error

Though if you want that behavior, you can write that today, but if you like today's behavior more and we switch, there wouldn't be a way to go back except to turn off EOPT -- but maybe that's what EOPT means, after all.

RyanCavanaugh commented 1 year ago

Having just read what I wrote, I'm maybe unsure now, since it's not like T2 acquires a wrong type here. It's maybe surprising, but the definition doesn't introduce a novel soundness hole or anything.