Support naming tuple members by weswigham · Pull Request #38234 · microsoft/TypeScript (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation57 Commits14 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes #28259
This PR allows tuple members to have names like so:
type Segment = [length: number, count: number]
The optional marker and variadic markers (? and ...) are expected in the same places as the parameter lists. I already have code in place to gracefully parse a ? or ... in the type and issue an error. Parameter lists automatically make named tuples (in fact, there is no new checker machinery for trafficking the names for named tuples, since it was already in place for parameter lists - mostly just new parser/emitter code). Names do not affect assignability in any way, and just exist for documentation and quickinfo.
j-oliveras, brainkim, peterblazejewicz, strelga, Bnaya, dragomirtitian, acid-chicken, millsp, cherryblossom000, fuafa, and 19 more reacted with thumbs up emoji brainkim, j-oliveras, Bnaya, fuafa, streamich, fvilante, and Juninhoww2 reacted with laugh emoji j-oliveras, brainkim, peterblazejewicz, Bnaya, millsp, fuafa, streamich, doumr, Bartunek, mminer, and 7 more reacted with hooray emoji ikokostya, Bnaya, and streamich reacted with confused emoji j-oliveras, alexrock, brainkim, peterblazejewicz, markusjohnsson, Bnaya, fuafa, JackCA, elektronik2k5, donaldpipowitch, and 8 more reacted with heart emoji brainkim, j-oliveras, AviVahl, strelga, Bnaya, streamich, AwesomeObserver, fvilante, ExE-Boss, jamesrwaugh, and bakkdoor reacted with rocket emoji brainkim, j-oliveras, Bnaya, streamich, and fvilante reacted with eyes emoji
A fork with an experimental idea which might not make it into master
label
| ==== tests/cases/conformance/types/tuple/named/namedTupleMembersErrors.ts (5 errors) ==== |
|---|
| export type Segment1 = [length: number, number]; // partially named, disallowed |
| ~~~~~~ |
| !!! error TS5084: Tuple members must all have names or not have names. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sandersn @DanielRosenwasser 🚲 🏚️ - I rewrote this message like 3 times and they all sound awkward. I want to say something like Tuple member namedness must be homogeneous but that's maybe also bad.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tuple members must all be named or anonymous.
or
All members of a tuple must either be named or anonymous.
or
Named tuple members cannot be mixed with anonymous tuple members.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you might have to play where you issue the error depending on which one you pick. Related errors can help too as a learning device for what an "anonymous" member is.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Either all or no tuple member can be named"
or
"Naming of tuples is only supported if all members are named."
or
"Either all or no tuple members need to be named"
| @@ -1273,7 +1275,15 @@ namespace ts { |
|---|
| export interface TupleTypeNode extends TypeNode { |
| kind: SyntaxKind.TupleType; |
| elementTypes: NodeArray; |
| elements: NodeArray<TypeNode | NamedTupleMember>; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, I made NamedTupleMember a TypeNode, which is inline with OptionalType and RestType, so I don't have to rename this field (or even change the type). If anyone feels strongly about it, I can change it back. However renaming it was very useful for figuring out where I needed to adjust/handle the new node; so "breaking" the API because of the new child kind might be worthwhile for other consumers, too. Depends on how strongly we want to maintain AST compatibility I suppose.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the rename.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@typescript-bot pack this
@orta @DanielRosenwasser can either of you think of any extra language service features that these names might need hooking up to? I haven't implemented it yet, but I figure trafficking doc comments along with the names might also be useful for quick info. So, eg, when you have
if you tupleize it, you can retain the same quickinfo experience, rather than it being missing, since today it is:
Though admittedly, that's not directly related to this and could be a separate feature request!
Heya @weswigham, I've started to run the tarball bundle task on this PR at c80c84a. You can monitor the build here.
It'd be interesting to see how JSDoc plays into a lot of this. Not sure what tests you want, but quick info and signature help on these examples might be useful.
/**
- @param args {[a: string, b: number]} the params */ function foo(...args) { let [a, b] = args; }
foo()
/**
- @param args {[a: string, b: number]} the params */ function bar(...[a, b]) { }
bar()
Hey @weswigham, 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/72444/artifacts?artifactName=tgz&fileId=2D62822DA042EC2DA13BEE472478F4D5575F9C36062F7718D8375C60BB11C2B002&fileName=/typescript-4.0.0-insiders.20200428.tgz"
}
}
and then running npm install.
There is also a playground for this build.
Incidentally, this also fixes a small bug do do with jsdoc variadic types and declaration emit (which is as far as I can tell, was unreported, but was reflected in our type baselines, which I noticed while I was adjusting tuple whitespace emit to support preserving comments/multiline-ness).
I tried 3.9.1-rc on DevTools today and ran into that issue: #38242 Glad to see a fix in progress!
From design meeting: Better tuple language service experiences across the board would be nice:
- Element completions (that mention tuple name) eg:
0 (property) first(also relevant to non-labeled tuple members) - Teasing apart unions of rest tuples into separate signature help overloads
- Quickfix for grammar errors on
...and?in name - Refactoring for extracting a parameter list as a tuple type/set of overload parameter lists as a tuple type
- Try to preserve doc comments with the tuple on the best effort basis
@typescript-bot pack this
The syntax has been swapped to the parameter-like one, and I've got most of what we want in-place now (so should be usable to test). Still todo:
- Teasing apart unions of rest tuples into separate signature help overloads
- More tests for the refactoring
- Better parameter comment preservation for the refactoring
- More tests in general, just for increased coverage (codifying the behavior of recursive aliases, generics, and such, while I know it works, should probably be in a test, and it's probably worth sanity checking the assignability relationships involving tuples with labels, even though I didn't really touch the existing implementation)
Heya @weswigham, I've started to run the tarball bundle task on this PR at f0cf8d4. You can monitor the build here.
Hey @weswigham, 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/73632/artifacts?artifactName=tgz&fileId=6E6817DF35333947E853CF063D39FB4BE556988D7B92F809B8CD718CE706059602&fileName=/typescript-4.0.0-insiders.20200512.tgz"
}
}
and then running npm install.
There is also a playground for this build.
…played as seperate entries now
Heya @weswigham, I've started to run the perf test suite on this PR at a7aa566. You can monitor the build here.
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at a7aa566. You can monitor the build here.
Heya @weswigham, I've started to run the extended test suite on this PR at a7aa566. You can monitor the build here.
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at a7aa566. You can monitor the build here.
Hey @weswigham, 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/73966/artifacts?artifactName=tgz&fileId=31DDF907A9FA5808E09BD99C8A1AC55236B525EFE19354549F6E4ECF150B754F02&fileName=/typescript-4.0.0-insiders.20200514.tgz"
}
}
and then running npm install.
There is also a playground for this build.
| kind: SyntaxKind.NamedTupleMember; |
|---|
| dotDotDotToken?: Token<SyntaxKind.DotDotDotToken>; |
| name: Identifier; |
| questionToken?: Token<SyntaxKind.QuestionToken>; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General design question: how do you decide between saving a reference to a token like this vs. saving a boolean property that indicates optionality, like ImportTypeNode['isTypeOf']? Is this token actually used anywhere?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, mostly so the members of the same name are the same type as ParameterDeclaration. In the case of ParameterDeclaration, so comments can more easily be collected on the intervening tokens (iirc). (Types don't normally care about comment preservation on intervening tokens)
| else if (unwrappedType.kind === SyntaxKind.RestType) { | |
|---|---|
| sawRest = true; | |
| } | |
| unwrappedType = (unwrappedType as OptionalTypeNode | RestTypeNode | ParenthesizedTypeNode).type; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit / nagging question: any reason not to use isOptionalType and friends in the while so as to avoid this type assertion?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh, mostly just because iirc one of them doesn't exist. Yeah, I think there isn't a isRestType and mixing and matching looked odd.
| } |
|---|
| const tupleTypeNode = createTupleTypeNode(tupleConstituentNodes); |
| const tupleTypeNode = setEmitFlags(createTupleTypeNode(tupleConstituentNodes), EmitFlags.SingleLine); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know single-line is a reasonable choice from here? It almost seems like single-line ought to remain the default for tuples, but I guess that would have meant introducing a new EmitFlag (which we’re running out of space for).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly that. Unlike object types, tuples have hisorically only been pretty printed single line, however we only have a flag for single line. So in many cases, I now detect if the input is single line and add the single line flag, so I can use it's absence as the multiline case. In this location specifically, I have no reason to change it to multiline (which is now the "default", lacking any emit flags), so it stays single line.
| @@ -9520,27 +9541,36 @@ namespace ts { |
|---|
| return result; |
| } |
| function getExpandedParameters(sig: Signature): readonly Symbol[] { |
| function getExpandedParameters(sig: Signature, skipUnionExpanding?: boolean): readonly (readonly Symbol[])[] { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this change on its surface, but I don’t immediately understand why it was necessary. Can you point me to the test case(s) where this matters?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns of array of arrays so it can return, to the language service, a list of "psuedooverloads" - one for each rest union tuple member. The flag exists to disable that behavior for node serialization, where we'd like to keep it as the single overload.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, so this only comes into play in signature help (and possibly quick info or whatever)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
Fixed a crash the user suite found (a parameter without name is apparently a thing which can happen, even though our types disallow it?), the rest of the user suite looks pretty good - just named tuples in error messages. RWC diff is just a tuple in declaration emit being multiline (since now we can preserve that). DT diff is currently filled with failures due to dropping 2.9 support, but the few failures not related to that are places where tuples acquired named and thus no longer match ExpectType calls, so are also good (though should probably be preemptively updated to accept the new labeled printback, once we can get a full list).
Perf test died early with a crash in npm itself... looks like it's running an old version of npm. @rbuckton is it safe to update the npm version in use on the perf testing machine (and if so, would you be able to)? It's on 6.7.0, while hosted workers are working fine on 6.14.4.
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at e3a1cb1. You can monitor the build here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The one thing I wonder about is whether it is better to omit tuple element names in certain error cases (see my review comment). I worry we'll confuse people between the actual names ('0', '1', etc.) and the element names.
| function getTupleTypeOfArity(arity: number, minLength: number, hasRestElement: boolean, readonly: boolean, associatedNames?: __String[]): GenericType { |
|---|
| const key = arity + (hasRestElement ? "+" : ",") + minLength + (readonly ? "R" : "") + (associatedNames && associatedNames.length ? "," + associatedNames.join(",") : ""); |
| function getTupleTypeOfArity(arity: number, minLength: number, hasRestElement: boolean, readonly: boolean, namedMemberDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[]): GenericType { |
| const key = arity + (hasRestElement ? "+" : ",") + minLength + (readonly ? "R" : "") + (namedMemberDeclarations && namedMemberDeclarations.length ? "," + map(namedMemberDeclarations, getNodeId).join(",") : ""); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like using node IDs here, though it will increase the number of unique tuple target types we generate.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Using node id here brings tuples in line with normal anonymous object types. It gets us good name and documentation preservation, though!
| @@ -13531,7 +13573,7 @@ namespace ts { | |
|---|---|
| function getAliasSymbolForTypeNode(node: Node) { | |
| let host = node.parent; | |
| while (isParenthesizedTypeNode(host) | | isTypeOperatorNode(host) && host.operator === SyntaxKind.ReadonlyKeyword) { |
| while (isParenthesizedTypeNode(host) | | host.kind === SyntaxKind.NamedTupleMember |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't think of when a named tuple member would occur in the parent chain?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly a leftover from an initial stage where I considered labels a kind of parenthesized declaration. I must have missed removing this in the cleanup.
| @@ -25117,19 +25176,23 @@ namespace ts { |
|---|
| } |
| } |
| const types = []; |
| const names: (ParameterDeclaration | NamedTupleMember)[] = []; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the scenario for capturing names here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you concatenate one tuple to another via spread, this allows us to concatenate the name list, too.
| for (let i = pos; i < nonRestCount; i++) { |
|---|
| types.push(getTypeAtPosition(source, i)); |
| names.push(getParameterNameAtPosition(source, i)); |
| const name = getNameableDeclarationAtPosition(source, i); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So previously we'd always collect names (even made up arg_xxx names), but now we only collect names with an actual declaration associated?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct! That does mean that generated names can still be shoved into parameter lists (like arg_0) but will disappear when copied into a tuple. I actually think this is a good thing, since the generated names were just positional anyway.
| seenNamedElement = true; |
|---|
| } |
| else if (seenNamedElement) { |
| grammarErrorOnNode(e, Diagnostics.Tuple_members_must_all_have_names_or_not_have_names); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Tuple members must all have names or all not have names". As it is now, you can read it as all members must have a name or not, which obviously is always true.
| @@ -1273,7 +1275,15 @@ namespace ts { |
|---|
| export interface TupleTypeNode extends TypeNode { |
| kind: SyntaxKind.TupleType; |
| elementTypes: NodeArray; |
| elements: NodeArray<TypeNode | NamedTupleMember>; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the rename.
| Type '[string] | [number, boolean]' is not assignable to type '[number, boolean]'. |
|---|
| Property '1' is missing in type '[string]' but required in type '[number, boolean]'. |
| Type '[string] | [number, boolean]' is not assignable to type '[y: number, z: boolean]'. |
| Property '1' is missing in type '[string]' but required in type '[y: number, z: boolean]'. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the actual property name is '1', but it sure looks like it's name is z. Wonder if it is better to omit the names in some cases?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh... I think it'd be kinda odd to sometimes display the same type with labels and sometimes without. Plus, this error is somewhat rare; I believe we usually try to report tuple length errors as an arity mismatch (ie, on length), but fail to do so here because of the union.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
Type '[string] | [number, boolean]' is not assignable to type '[y: number, z: boolean]'.
Property '1' (labeled as 'z') is missing in type '[string]' but required in type '[y: number, z: boolean]'.
Sorry for the late question, I was super excited for this feature as I want to use it now (and get the benefit in future as Angular updates it's TS dependency)
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 05d398e. You can monitor the build here.
cangSDARM added a commit to cangSDARM/TypeScript that referenced this pull request
- upstream/master: Support naming tuple members (microsoft#38234) LEGO: check in for master to temporary branch. fix: extract const in jsx (microsoft#37912) No contextual types from circular mapped type properties (microsoft#38653) Ensure formatter can always get a newline character (microsoft#38579) Fix debug command for Node debugging Remove mentions of runtests-browser in CONTRIBUTING.md fix(33233): add outlining for comments before property access expression regression(38485): allow using rawText property in processing a tagged template
sheetalkamat added a commit to microsoft/TypeScript-TmLanguage that referenced this pull request
This new feature looks like is just what I needed right now. But, I do not know how to connect the dots:
In summary, I have a readonly array of const strings, like readonly ['aaa', 'bbb', 'ccc']. I can't find a way (if there is one) to transform this into a generic variadic function parameter type, like [aaa: string | number, bbb: string | number, ccc: string | number].
There is no such functionality in this PR (or, as far as I know, planned). Tuple member names are purely documentation... they don't interact with the rest of the type system in any way.
Hmm, that's bad. Would be a really cool feature to allow "dynamic functions"
5 tasks