- Notifications
You must be signed in to change notification settings - Fork12.9k
Support naming tuple members#38234
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| ==== 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. |
There was a problem hiding this comment.
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 likeTuple member namedness must be homogeneous but that's maybe also bad.
DanielRosenwasserApr 28, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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<TypeNode>; | |||
| elements: NodeArray<TypeNode | NamedTupleMember>; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the end, I madeNamedTupleMember aTypeNode, which is inline withOptionalType andRestType, so I don'thave 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like the rename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This rename is missing in the API breaking changes for 4.0 -https://github.com/microsoft/TypeScript/wiki/API-Breaking-Changes
@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 Though admittedly, that's not directly related to this and could be a separate feature request! |
typescript-bot commentedApr 28, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the tarball bundle task on this PR atc80c84a. You can monitor the buildhere. |
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 */functionfoo(...args){let[a,b]=args;}foo()/** *@param args {[a: string, b: number]} the params */functionbar(...[a,b]){}bar() |
typescript-bot commentedApr 28, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hey@weswigham, I've packed this intoan installable tgz. You can install it for testing by referencing it in your and then running There is also a playgroundfor this build. |
I tried 3.9.1-rc on DevTools today and ran into that issue:#38242 Glad to see a fix in progress! |
weswigham commentedMay 1, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
From design meeting: Better tuple language service experiences across the board would be nice:
|
weswigham commentedMay 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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:
|
typescript-bot commentedMay 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the tarball bundle task on this PR atf0cf8d4. You can monitor the buildhere. |
typescript-bot commentedMay 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hey@weswigham, I've packed this intoan installable tgz. You can install it for testing by referencing it in your and then running There is also a playgroundfor this build. |
@sandersn@ahejlsberg@rbuckton@andrewbranch reviews and feedback whenever you're ready would be appreciated now ❤️ |
It shouldn't affect much, mostly just being a new syntax feature, but @typescript-bot test this |
typescript-bot commentedMay 14, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the perf test suite on this PR ata7aa566. You can monitor the buildhere. |
typescript-bot commentedMay 14, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the parallelized Definitely Typed test suite on this PR ata7aa566. You can monitor the buildhere. |
typescript-bot commentedMay 14, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the extended test suite on this PR ata7aa566. You can monitor the buildhere. |
typescript-bot commentedMay 14, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the parallelized community code test suite on this PR ata7aa566. You can monitor the buildhere. |
typescript-bot commentedMay 14, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hey@weswigham, I've packed this intoan installable tgz. You can install it for testing by referencing it in your and then running There is also a playgroundfor this build. |
The user suite test run you requested has finished andfailed. I've opened aPR with the baseline diff from master. |
| kind: SyntaxKind.NamedTupleMember; | ||
| dotDotDotToken?: Token<SyntaxKind.DotDotDotToken>; | ||
| name: Identifier; | ||
| questionToken?: Token<SyntaxKind.QuestionToken>; |
There was a problem hiding this comment.
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, likeImportTypeNode['isTypeOf']? Is this token actually used anywhere?
There was a problem hiding this comment.
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 asParameterDeclaration. In the case ofParameterDeclaration, 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; |
There was a problem hiding this comment.
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 useisOptionalType and friends in thewhile so as to avoid this type assertion?
There was a problem hiding this comment.
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 aisRestType and mixing and matching looked odd.
| } | ||
| const tupleTypeNode = createTupleTypeNode(tupleConstituentNodes); | ||
| const tupleTypeNode =setEmitFlags(createTupleTypeNode(tupleConstituentNodes), EmitFlags.SingleLine); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
src/compiler/checker.ts Outdated
| @@ -9520,27 +9541,36 @@ namespace ts { | |||
| return result; | |||
| } | |||
| function getExpandedParameters(sig: Signature): readonly Symbol[] { | |||
| function getExpandedParameters(sig: Signature, skipUnionExpanding?: boolean): readonly(readonlySymbol[])[] { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep.
weswigham commentedMay 14, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 Perf test died early with a crash in |
@typescript-bot run dt again since the 2.9 thing should be fixed now |
typescript-bot commentedMay 15, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the parallelized Definitely Typed test suite on this PR ate3a1cb1. You can monitor the buildhere. |
There was a problem hiding this comment.
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 theactual names ('0','1', etc.) and the element names.
src/compiler/checker.ts Outdated
| 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(",") : ""); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
src/compiler/checker.ts Outdated
| @@ -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 ||isTypeOperatorNode(host) && host.operator === SyntaxKind.ReadonlyKeyword) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/compiler/checker.ts Outdated
| @@ -25117,19 +25176,23 @@ namespace ts { | |||
| } | |||
| } | |||
| const types = []; | |||
| const names: (ParameterDeclaration | NamedTupleMember)[] = []; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 uparg_xxx names), but now we only collect names with an actual declaration associated?
There was a problem hiding this comment.
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 (likearg_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.
src/compiler/checker.ts Outdated
| seenNamedElement = true; | ||
| } | ||
| else if (seenNamedElement) { | ||
| grammarErrorOnNode(e, Diagnostics.Tuple_members_must_all_have_names_or_not_have_names); |
There was a problem hiding this comment.
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<TypeNode>; | |||
| elements: NodeArray<TypeNode | NamedTupleMember>; | |||
There was a problem hiding this comment.
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]'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm, theactual property name is'1', but it sure looks like it's name isz. Wonder if it is better to omit the names in some cases?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
@typescript-bot run dt |
typescript-bot commentedMay 19, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at05d398e. You can monitor the buildhere. |
* 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
ftzi commentedSep 24, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
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. |
ftzi commentedSep 24, 2020
Hmm, that's bad. Would be a really cool feature to allow "dynamic functions" |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#28259
This PR allows tuple members to have names like so:
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.