Uh oh!
There was an error while loading. Please reload this page.
Exhaustive case completion for switch statements - #50996
Conversation
| * @param text a valid bigint string excluding a trailing `n`, but including a possible prefix `-`. Use `isValidBigIntString(text, roundTripOnly)` before calling this function. | ||
| */ | ||
| function parseBigIntLiteralType(text: string) { | ||
| const negative = text.startsWith("-"); |
There was a problem hiding this comment.
Moved this into a new function in utilities.
| if (isSingleOrDoubleQuote(firstChar) && some(symbol.declarations, hasNonGlobalAugmentationExternalModuleSymbol)) { | ||
| return factory.createStringLiteral(getSpecifierForModuleSymbol(symbol, context)); | ||
| } | ||
| const canUsePropertyAccess = firstChar === CharacterCodes.hash ? |
There was a problem hiding this comment.
Also moved this into a new function in utilities.
Heya @gabritto, I've started to run the perf test suite on this PR at 97dcf69. You can monitor the build here. Update: The results are in! |
typescript-bot
commented
Nov 11, 2022
@gabritto Here they are:CompilerComparison Report - main..50996
System
Hosts
Scenarios
TSServerComparison Report - main..50996
System
Hosts
Scenarios
StartupComparison Report - main..50996
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewbranch
left a comment
There was a problem hiding this comment.
Nice work! The code looks great. A few thoughts from trying it out:
It would be nice to prevent global completions where only case can go—I think I should only see the case keyword and your snippet here:
VS Code handles indentation of multi-line snippets—we seem to send them with no indentation—which is great, but it doesn’t indent the inline preview text. @mjbvz is this something that can be fixed on VS Code’s side?
I wonder if it would make sense to offer the snippet with a default case when the reduced type is a union where some of the constituents are unit types:
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| insertText: | ||
| `case E.A: | ||
| case E.B: | ||
| case 1:`, |
There was a problem hiding this comment.
I feel like I would expect snippet tab stops after each :; what do you think?
There was a problem hiding this comment.
I haven't considered that before. I guess I'll add this and try it out and ask others to try it to see if it's annoying or not.
gabritto
commented
Nov 14, 2022
I'll need to go looking at switch statement examples again to see if that possibly makes sense. |
amcasey
left a comment
There was a problem hiding this comment.
LGTM. I'll let you and @andrewbranch decide how much experience polish is in scope.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| interface CaseClauseTracker { | ||
| addClause(clause: CaseClause): void; | ||
| addValue(value: string | number): void; |
There was a problem hiding this comment.
You can check for the presence of a bigint but not add one? Maybe they get converted to numbers?
There was a problem hiding this comment.
As it is today, we don't yet need to add a BigInt with addValue, because we add them when processing the existing case clauses. We only use addValue so far for adding enum member values, and those can't be BigInts. I figured I'd only implement what I'm currently using.
There was a problem hiding this comment.
I guess on second thought, I could refactor things to always use addValue. I think I'll do that.
| interface CaseClauseTracker { | ||
| addClause(clause: CaseClause): void; | ||
| addValue(value: string | number): void; | ||
| hasValue(value: string | number | PseudoBigInt): boolean; |
There was a problem hiding this comment.
Personally, I'm a fan of the TryAdd pattern (too C#?), which lets you add if absent and skip otherwise.
| existingNumbers.add(parseInt(expression.text)); | ||
| break; | ||
| case SyntaxKind.BigIntLiteral: | ||
| const parsedBigInt = parseBigInt(endsWith(expression.text, "n") ? expression.text.slice(0, -1) : expression.text); |
There was a problem hiding this comment.
Remind me why we need to parse and then re-stringify bigints?
There was a problem hiding this comment.
We need to use a standard representation of big ints for our sets, so that big ints that are written differently but are the same end up with the same representation. Since we can't use actual big ints because they're not supported in all runtimes (that might change eventually?), we use "pseudo big ints" in our codebase to represent big ints, but then that's not something we can put into a set, therefore I need to stringify the pseudo big ints.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gabritto
commented
Nov 22, 2022
@typescript-bot pack this |
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Ok, I just pushed a commit that adds tab stops after each case clause. I briefly tried using that, and I have a question... switch(foo){case1: {blah;}}if the tab stop is immediately after the case clause, in the same line, then editing the tab stop works, because you can write However, if you want to write your case clauses like this: switch(foo){case1:
blah;}then when you accept the completion snippet and are starting to write stuff into a tab stop, you need to first go to a new line by pressing ENTER (because the tab stop is in the same line as the |
Something else that's maybe worth mentioning: the completion entry kind is 'unknown', because I couldn't find an appropriate kind among the existing ones. |
As per offline discussions, seems we're going with yes tab stops, and putting those on the same line as the case clauses. Some things about snippet editing mode can make this annoying, so we'll be watching for feedback. The possible pain points brought up so far are:
On the other hand, as pointed out by @andrewbranch, users can easily exit snippet mode by pressing |
Update: I noticed there were spaces between the case clause and the tab stops, and I've now removed them, so currently the completion looks like this when accepted: The way tab stops are emitted by the emitter makes it so they're considered a statement, and therefore the emitter adds the spacing between |
andrewbranch
left a comment
There was a problem hiding this comment.
Very nice work 🌟 I’m excited to use this! I’ve been sad every time I’ve started switching over an enum since you started working on it 😄
heroboy
commented
Dec 29, 2022
Great work. My extension can finally be retired. |
ion1
commented
Mar 16, 2023
To make the type checker verify that my code handles all the cases, I tend to use boilerplate such as this: switch(foo){case"a": // ...case"b": // ...case"c": // ...default:
constimpossible: never=foo;// handle the error}I would benefit from that being included in a completion more often than not. |
JonasKru
commented
Mar 17, 2023
Can confirm, works in neovim. 👍 |
This PR aims to add an exhaustive case completion in the completions provided inside a switch statement.
The exhaustive case completion aims to provide all the possible case clauses that are not already present in the switch statement.
The completion is also only provided if the switch expression has a type that is a union of literal types, because that's when it's likely a user might want an exhaustive case list in their switch statement, and that's when we can offer the list of cases based on the type.
Basic examples
becomes:
when the case completion is accepted, and
becomes
Open items
The preview text on vscode shows the wrong indentation. Is this something vscode can fix? Or should we fix it? How does it work in other editors (e.g. vim, emacs)?
Tab stops: should we add tab stops after each case clause? If so, should the tab stops be on the same line as the case, or on the next line?