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

Allowed tuples to have both named and anonymous members #53356

Merged
merged 18 commits into from
Jun 26, 2023

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Mar 19, 2023

Fixes #52853

Previously, the labeledElementDeclarations property on TupleTypes was all-or-nothing for having named vs. anonymous members. Now that it's possible for them to mix-and-match, this PR pulls logic from getParameterNameAtPosition into getTupleElementLabel for filling in a name (`${restParameterName}_${index}`).

Co-authored-by: Mateusz Burzyński mateuszburzynski@gmail.com

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Mar 19, 2023
@@ -6399,7 +6399,7 @@ export interface TupleType extends GenericType {
hasRestElement: boolean;
combinedFlags: ElementFlags;
readonly: boolean;
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[];
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration | undefined)[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Breaking API Change] Is this the right choice to make? One alternative could be to rename it to elementDeclarations with a type like readonly (ParameterDeclaration | TypeNode). My hunch is that that would be preferable, but I'm going with this smaller change for now to make reviewing easier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming it doesn't feel like a good idea compared to this; I assume that this array is indexed such that there are empty spaces where there are no names?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this array is indexed such that there are empty spaces where there are no names?

To the best of my knowledge - yes. There are a couple of places in this PR where it seems to be handled this way

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the alternative be if not an array with missing names?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that most implementations that would include keeping labeledElementDeclarations with partial labels could be considered a breaking change. Even if you somehow omit undefined positions you still end up with a .length mismatch between those and other properties and that would be rather disastrous.

I think the current structure of this object makes the most sense but if you consider it to be too big of a breaking change then I think a different property should be added when mixed labels are involved and this one should be kept as undefined in such situations. That will be inconvenient to work with but if maintaining a backward compatibility is the goal then I don't see any other straightforward way of achieving this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in, this array seems to be the only one in the interface which actually describes each element; if we were to keep this array without making elements undefined, then the only thing I can think is that all of the named members gets smooshed up together, except now their indexes are lost.

So, it seems to me that the change in the PR is the least breaky option when we're now saying that something can potentially be undefined, because otherwise someone may iterate over this list thinking they're seeing everything, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mean "what would the alternative be if not an array which can contain undefined?"

I was responding to exactly that but perhaps I wasn't super clear. Should I try to rephrase my original answer?

if we were to keep this array without making elements undefined, then the only thing I can think is that all of the named members gets smooshed up together, except now their indexes are lost.

yeah, that would be IMHO bad - I mentioned that here: "Even if you somehow omit undefined positions you still end up with a .length mismatch between those and other properties and that would be rather disastrous.". But right now one alternative option comes to my mind - the missing labels could be prefilled with some defaults. You already generate such things for function parameters and stuff at times, like arg_0, etc, or something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we generate synthetic nodes for names for stuff like declaration emit, but do we commonly store them in the types? (I genuinely don't know the answer!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that either and perhaps you don't ever do that right now. I prefer to leave those optional members as undefined, that's just an idea that would help you to minimize the impact of this change if you think that this is a strong requirement for this PR to land.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're agreeing? I think I too prefer to make them undefined just like this PR does; I just wanted to make sure that was the best choice given the break. Reasonably no matter what, if downstream users care about this particular bit, they're going to have to fix their code.

src/compiler/checker.ts Outdated Show resolved Hide resolved
JoshuaKGoldberg and others added 2 commits March 23, 2023 12:09
Co-authored-by: Jake Bailey <jabaile@microsoft.com>
src/compiler/checker.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review March 23, 2023 16:48
Co-authored-by: Jake Bailey <jabaile@microsoft.com>
@JoshuaKGoldberg JoshuaKGoldberg requested review from jakebailey and Andarist and removed request for jakebailey March 24, 2023 12:14
@Andarist
Copy link
Contributor

I think that it would be nice to get some code coverage for generic mapped types with array constraints. With them named and anonymous elements should be preserved in the same way. Similarly, it should be possible to add new named/anonymous elements using generic conditional types.

@sandersn sandersn added this to Not started in PR Backlog Mar 27, 2023
@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 6a7fdd1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2023

Heya @jakebailey, I've started to run the extended test suite on this PR at 6a7fdd1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2023

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 6a7fdd1. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2023

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 6a7fdd1. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2023

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 6a7fdd1. You can monitor the build here.

Update: The results are in!

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/155492/artifacts?artifactName=tgz&fileId=34ECE4E0DBA7A648A8E18CCCC5BCD760BD4EEAB6ABD100E36CC69554625B304302&fileName=/typescript-5.2.0-insiders.20230615.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-53356-21".;

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..53356

Metric main 53356 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 365,530k (± 0.01%) 365,555k (± 0.01%) ~ 365,508k 365,595k p=0.298 n=6
Parse Time 3.56s (± 0.49%) 3.54s (± 0.40%) ~ 3.52s 3.56s p=0.121 n=6
Bind Time 1.18s (± 0.44%) 1.18s (± 0.46%) +0.01s (+ 0.71%) 1.18s 1.19s p=0.038 n=6
Check Time 9.62s (± 0.40%) 9.64s (± 0.26%) ~ 9.61s 9.68s p=0.369 n=6
Emit Time 7.95s (± 0.86%) 7.97s (± 0.86%) ~ 7.84s 8.05s p=0.568 n=6
Total Time 22.30s (± 0.47%) 22.33s (± 0.43%) ~ 22.17s 22.47s p=0.572 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 192,572k (± 0.03%) 193,739k (± 0.90%) ~ 192,533k 196,014k p=0.173 n=6
Parse Time 1.59s (± 0.76%) 1.60s (± 1.58%) ~ 1.55s 1.62s p=0.112 n=6
Bind Time 0.83s (± 1.01%) 0.82s (± 0.77%) ~ 0.81s 0.83s p=0.226 n=6
Check Time 10.15s (± 0.93%) 10.17s (± 0.97%) ~ 10.04s 10.33s p=0.873 n=6
Emit Time 3.00s (± 1.02%) 3.01s (± 0.42%) ~ 2.99s 3.02s p=0.935 n=6
Total Time 15.58s (± 0.62%) 15.60s (± 0.72%) ~ 15.48s 15.79s p=0.630 n=6
Monaco - node (v16.17.1, x64)
Memory used 345,877k (± 0.00%) 345,887k (± 0.01%) ~ 345,859k 345,913k p=0.470 n=6
Parse Time 2.73s (± 0.67%) 2.74s (± 0.36%) ~ 2.73s 2.76s p=0.251 n=6
Bind Time 1.09s (± 0.47%) 1.09s (± 0.37%) ~ 1.08s 1.09s p=0.595 n=6
Check Time 7.84s (± 0.49%) 7.88s (± 0.83%) ~ 7.83s 8.00s p=0.571 n=6
Emit Time 4.44s (± 0.54%) 4.46s (± 0.31%) ~ 4.45s 4.48s p=0.105 n=6
Total Time 16.10s (± 0.41%) 16.18s (± 0.42%) ~ 16.10s 16.28s p=0.077 n=6
TFS - node (v16.17.1, x64)
Memory used 299,992k (± 0.01%) 299,984k (± 0.01%) ~ 299,962k 300,014k p=0.378 n=6
Parse Time 2.16s (± 0.59%) 2.17s (± 1.26%) ~ 2.14s 2.20s p=0.864 n=6
Bind Time 1.24s (± 0.61%) 1.24s (± 1.29%) ~ 1.22s 1.26s p=1.000 n=6
Check Time 7.30s (± 0.27%) 7.31s (± 0.44%) ~ 7.28s 7.37s p=0.871 n=6
Emit Time 4.31s (± 0.46%) 4.35s (± 0.78%) ~ 4.31s 4.39s p=0.050 n=6
Total Time 15.01s (± 0.24%) 15.06s (± 0.60%) ~ 14.97s 15.19s p=0.422 n=6
material-ui - node (v16.17.1, x64)
Memory used 480,361k (± 0.01%) 480,346k (± 0.00%) ~ 480,303k 480,360k p=0.688 n=6
Parse Time 3.26s (± 0.39%) 3.25s (± 0.37%) ~ 3.24s 3.27s p=0.362 n=6
Bind Time 0.93s (± 0.59%) 0.94s (± 0.00%) ~ 0.94s 0.94s p=0.071 n=6
Check Time 17.90s (± 0.91%) 17.91s (± 0.73%) ~ 17.75s 18.12s p=0.810 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.10s (± 0.77%) 22.10s (± 0.56%) ~ 21.96s 22.31s p=0.810 n=6
xstate - node (v16.17.1, x64)
Memory used 560,644k (± 0.02%) 560,617k (± 0.01%) ~ 560,535k 560,691k p=0.336 n=6
Parse Time 3.99s (± 0.49%) 4.00s (± 0.54%) ~ 3.98s 4.04s p=0.220 n=6
Bind Time 1.76s (± 0.43%) 1.76s (± 0.59%) ~ 1.75s 1.78s p=0.273 n=6
Check Time 3.06s (± 0.64%) 3.07s (± 0.34%) ~ 3.06s 3.09s p=0.332 n=6
Emit Time 0.09s (± 0.00%) 0.09s (± 0.00%) ~ 0.09s 0.09s p=1.000 n=6
Total Time 8.90s (± 0.37%) 8.94s (± 0.18%) ~ 8.91s 8.95s p=0.077 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-148-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 53356 6
Baseline main 6

Developer Information:

Download Benchmark

@jakebailey
Copy link
Member

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2023

Heya @jakebailey, I've started to run the extended test suite on this PR at 6a7fdd1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/53356/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

  • [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-53356/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/53356/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

1 similar comment
@typescript-bot
Copy link
Collaborator

Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@jakebailey jakebailey added the Breaking Change Would introduce errors in existing code label Jun 22, 2023
@jakebailey
Copy link
Member

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2023

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 6a7fdd1. You can monitor the build here.

Update: The results are in!

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think this looks fine, and I think the break is probably also fine because it's introducing a new feature that downstream API users will have to handle (and existing code will not contain undefined and so will keep working with existing tooling).

Maybe someone else has an opinion here; @DanielRosenwasser?

PR Backlog automation moved this from Waiting on reviewers to Needs merge Jun 22, 2023
@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

Co-authored-by: Daniel Rosenwasser <drosenwasser@microsoft.com>
@DanielRosenwasser DanielRosenwasser merged commit 5b85e94 into microsoft:main Jun 26, 2023
17 checks passed
PR Backlog automation moved this from Needs merge to Done Jun 26, 2023
@JoshuaKGoldberg JoshuaKGoldberg deleted the mixed-tuple-naming branch June 26, 2023 17:42
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.2.0 milestone Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Labeled variadic tuples should not require labeling spreads for tuples that are already labeled
5 participants