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

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

Open
craigphicks opened this issue Sep 7, 2021 · 13 comments
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript
Milestone

Comments

@craigphicks
Copy link

craigphicks commented Sep 7, 2021

Bug Report

🔎 Search Terms

🕗 Version & Regression Information

  • Spreading of Variadic Tuple types, as well as the ability to indicate final position optional parameters with labels, were introduced in ts 4.0. The noted behavior has been been present since then.

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

⏯ 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
Copy link
Contributor

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
Copy link
Author

@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 craigphicks changed the title exactOptionsParameters - still forcibly inserting undefined with optiona into Tuple when using spread operator on sub Tuple exactOptionsPropertyTypes - a number of anomolies detected with Tuples, options, and spreading. Sep 7, 2021
@craigphicks craigphicks changed the title exactOptionsPropertyTypes - a number of anomolies detected with Tuples, options, and spreading. exactOptionsPropertyTypes - anonolous behavior with Tuples, options, and spreading. Sep 7, 2021
@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Sep 7, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.5.0 milestone Sep 7, 2021
@andrewbranch andrewbranch added this to Agenda in Design Meeting Docket via automation Sep 7, 2021
@craigphicks
Copy link
Author

@andrewbranch thank you

@craigphicks craigphicks changed the title exactOptionsPropertyTypes - anonolous behavior with Tuples, options, and spreading. exactOptionsPropertyTypes - anomolous behavior with Tuples, options, and spreading. Sep 8, 2021
@andrewbranch
Copy link
Member

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
Copy link
Author

craigphicks commented Sep 8, 2021

@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
Copy link
Author

@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. r is for raw. c is the massaged and shortened form, which may be ignored.

@andrewbranch
Copy link
Member

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
Copy link
Author

craigphicks commented Sep 8, 2021

@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
Copy link
Member

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
Copy link
Author

craigphicks commented Sep 9, 2021

@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
Copy link
Contributor

Andarist commented Mar 2, 2023

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 RyanCavanaugh removed Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone labels Mar 2, 2023
@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Mar 2, 2023
@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 5.1.0 milestone Mar 2, 2023
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Experimentation Needed Someone needs to try this out to see what happens and removed Design Limitation Constraints of the existing architecture prevent this from being fixed labels Mar 2, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Mar 2, 2023
@RyanCavanaugh
Copy link
Member

... but that is an impossible type

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

  • Should tuples support middle optional elements?
  • If not, what should a spread do if doing one would produce one?

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript
Projects
Development

No branches or pull requests

5 participants