Uh oh!
There was an error while loading. Please reload this page.
Fix(52604): Provide Object member completions without comma; insert a comma - #52899
Merged
Conversation
navya9singh
requested review from
Andrew Branch (andrewbranch) and Gabriela Araujo Britto (gabritto)February 21, 2023 19:03
Andrew Branch (andrewbranch)
requested changes
Feb 21, 2023
Andrew Branch (andrewbranch)
left a comment
Member
There was a problem hiding this comment.
Nice work, this is 90% of the way there. Just a few things to clean up.
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.
Andrew Branch (andrewbranch)
requested changes
Feb 23, 2023
| /** Case completions for switch statements */ | ||
| SwitchCases = "SwitchCases/", | ||
| /** Completions for an Object literal expression */ | ||
| ObjectLiteralExpression = "ObjectLiteralExpression/", |
There was a problem hiding this comment.
The name of this should indicate that it’s for comma insertion. ObjectLiteralMemberWithComma or something
| this.replaceNode(sourceFile, oldNode, newNode, { suffix }); | ||
| } | ||
| public replacePropertyAssignmentOnSameLine(sourceFile: SourceFile, oldNode: PropertyAssignment, newNode: PropertyAssignment): void { |
There was a problem hiding this comment.
This is no longer used.
| }, | ||
| "Add missing comma for an object member completion '{0}'.": { | ||
| "category": "Message", | ||
| "code": 18052 |
There was a problem hiding this comment.
Although I don’t care as much about diagnostic message grouping as Jake does, it’s still nice to separate the Messages from the Errors. There should be a group that’s entirely Message category.
| if (source === CompletionSource.ObjectLiteralExpression && contextToken) { | ||
| const changes = textChanges.ChangeTracker.with( | ||
| { host, formatContext, preferences }, | ||
| tracker=>tracker.insertText(sourceFile, contextToken.end,",")); |
There was a problem hiding this comment.
Suggested change
| tracker=>tracker.insertText(sourceFile,contextToken.end,",")); | |
| tracker=>tracker.insertText(sourceFile,contextToken.end,",")); |
Andrew Branch (andrewbranch)
commented
Feb 23, 2023
Member
I wonder if this works on the playground 🤔 TypeScript Bot (@typescript-bot) pack this |
Contributor
Heya Andrew Branch (@andrewbranch), I've started to run the tarball bundle task on this PR at c8c1362. You can monitor the build here. |
TypeScript Bot (typescript-bot)
commented
Feb 23, 2023
Contributor
Hey Andrew Branch (@andrewbranch), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
Comment on lines
+1357
to
+1358
| if ((contextToken && isPropertyAssignment(contextToken.parent) && findNextToken(contextToken, contextToken?.parent, sourceFile)?.kind !== SyntaxKind.CommaToken && | ||
| completionKind === CompletionKind.ObjectPropertyDeclaration)) { |
There was a problem hiding this comment.
completionKind === CompletionKind.ObjectPropertyDeclaration is the cheapest of these conditions to check (along with contextToken), so it should be moved before the more expensive ones (particularly the findNextToken call) so the harder work can be short-circuited.
(Also, it looks like there’s an extra pair of parens around this whole expression.)
Member
I saw that Navya already commented about this on the other issue, but before shipping this we should really fix microsoft/vscode#174628 or at least update the code to distinguish between sources. |
Member
Another issue I found testing this: we don't do the comma insertion for object literal method snippet completions, only for the regular object literal completions: interfaceT{aaa: string;foo(): void;}constobj: T={aaa: ""/**/}If you select the completion that inserts just |
Member
Something else I noticed that doesn't work is that we don't offer object literal completions after a commaless method, or in fact after a property assignment with a more complex expression: interfaceT{aaa: string;bbb: number;foo(): void;}constobj: T={foo(){}/**/}constobj: T={bbb: 1*2/**/}You get the global completions here instead. |
Andrew Branch (andrewbranch)
commented
Feb 23, 2023
Member
Hm, good catch. The infrastructure we have now may not be conducive to combining the snippet and the comma insertion, since each has its own Inserting a comma after a method is probably a simple fix though. |
Jake Bailey (jakebailey)
commented
Feb 24, 2023
Member
TypeScript Bot (@typescript-bot) pack this |
Contributor
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at c8c1362. You can monitor the build here. |
Contributor
Hey Jake Bailey (@jakebailey), 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 |
Contributor
Heya Gabriela Araujo Britto (@gabritto), I've started to run the diff-based user code test suite (tsserver) on this PR at 2fff618. You can monitor the build here. Update: The results are in! |
Contributor
Heya Gabriela Araujo Britto (@gabritto), I've started to run the perf test suite on this PR at 2fff618. You can monitor the build here. Update: The results are in! |
Contributor
Hey Gabriela Araujo Britto (@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 |
| if (source === CompletionSource.ObjectLiteralMemberWithComma && contextToken) { | ||
| const changes = textChanges.ChangeTracker.with( | ||
| { host, formatContext, preferences }, | ||
| tracker => tracker.insertText(sourceFile, contextToken.end,",")); |
There was a problem hiding this comment.
Suggested change
| tracker=>tracker.insertText(sourceFile,contextToken.end,",")); | |
| tracker=>tracker.insertText(sourceFile,contextToken.end,","), | |
| ); |
| if (completionKind === CompletionKind.ObjectPropertyDeclaration && contextToken && | ||
| findPrecedingToken(contextToken.pos, sourceFile, contextToken)?.kind !== SyntaxKind.CommaToken && | ||
| (isMethodDeclaration(contextToken.parent.parent) || isSpreadAssignment(contextToken.parent) || findAncestor(contextToken.parent, (node: Node) => isPropertyAssignment(node))?.getLastToken() === contextToken || |
There was a problem hiding this comment.
- You don't need to annotate the type of
node,it'll be inferred (edit: but you can also just writeisPropertyAssignmentdirectly. - Feed through the
sourceFilewhen usingNodemethods so that they don't have to walk back up the tree.
Suggested change
| (isMethodDeclaration(contextToken.parent.parent)||isSpreadAssignment(contextToken.parent)||findAncestor(contextToken.parent,(node: Node)=>isPropertyAssignment(node))?.getLastToken()===contextToken|| | |
| (isMethodDeclaration(contextToken.parent.parent)||isSpreadAssignment(contextToken.parent)||findAncestor(contextToken.parent,node=>isPropertyAssignment(node))?.getLastToken(sourceFile)===contextToken|| |
There was a problem hiding this comment.
I'd also rather you just broke this into 2 nested ifs, where it looks like
if(completionKind===CompletionKind.ObjectPropertyDeclaration&&contextToken&&findPrecedingToken(contextToken.pos,sourceFile,contextToken)?.kind!==SyntaxKind.CommaToken){if(isMethodDeclaration(contextToken.parent.parent)||isSpreadAssignment(contextToken.parent)||
...){source=CompletionSource.ObjectLiteralMemberWithComma;hasAction=true;}}There was a problem hiding this comment.
Wait, you don't need node => isPropertyAssignment, just use isPropertyAssignment directly.
TypeScript Bot (typescript-bot)
commented
Jun 23, 2023
Contributor
Gabriela Araujo Britto (@gabritto) Here are the results of running the user test suite comparing Everything looks good! |
| hasAction = true; | ||
| } | ||
| if (completionKind === CompletionKind.ObjectPropertyDeclaration && contextToken && |
There was a problem hiding this comment.
Leave a comment above here with an example of what you're trying to capture.
| else { | ||
| if (isObjectLiteralExpression(contextToken.parent.parent) && | ||
| (isSpreadAssignment(contextToken.parent) || isShorthandPropertyAssignment(contextToken.parent) && | ||
| (getLineAndCharacterOfPosition(contextToken.getSourceFile(), contextToken.getEnd()).line !== getLineAndCharacterOfPosition(contextToken.getSourceFile(), position).line))) { |
There was a problem hiding this comment.
Pass through the sourceFile here too and reuse the same sourceFile
| (getLineAndCharacterOfPosition(contextToken.getSourceFile(), contextToken.getEnd()).line !== getLineAndCharacterOfPosition(contextToken.getSourceFile(), position).line))) { | ||
| return contextToken.parent.parent; | ||
| } | ||
| const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node)); |
There was a problem hiding this comment.
Suggested change
| constancestorNode=findAncestor(parent,(node: Node)=>isPropertyAssignment(node)); | |
| constancestorNode=findAncestor(parent,isPropertyAssignment); | |
| if (parent.parent && parent.parent.parent && isMethodDeclaration(parent.parent) && isObjectLiteralExpression(parent.parent.parent)) { | ||
| return parent.parent.parent; | ||
| } | ||
| const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node)); |
There was a problem hiding this comment.
Suggested change
| constancestorNode=findAncestor(parent,(node: Node)=>isPropertyAssignment(node)); | |
| constancestorNode=findAncestor(parent,isPropertyAssignment); |
| return parent.parent.parent; | ||
| } | ||
| const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node)); | ||
| if (contextToken.kind !== SyntaxKind.ColonToken && ancestorNode && ancestorNode.getLastToken() === contextToken && |
There was a problem hiding this comment.
Suggested change
| if(contextToken.kind!==SyntaxKind.ColonToken&&ancestorNode&&ancestorNode.getLastToken()===contextToken&& | |
| if(contextToken.kind!==SyntaxKind.ColonToken&&ancestorNode?.getLastToken()===contextToken&& |
| } | ||
| break; | ||
| default: | ||
| if (parent.parent && parent.parent.parent && isMethodDeclaration(parent.parent) && isObjectLiteralExpression(parent.parent.parent)) { |
There was a problem hiding this comment.
Suggested change
| if(parent.parent&&parent.parent.parent&&isMethodDeclaration(parent.parent)&&isObjectLiteralExpression(parent.parent.parent)){ | |
| if(parent.parent?.parent&&isMethodDeclaration(parent.parent)&&isObjectLiteralExpression(parent.parent.parent)){ |
| return contextToken.parent.parent; | ||
| } | ||
| const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node)); | ||
| if (ancestorNode && ancestorNode.getLastToken() === contextToken && isObjectLiteralExpression(ancestorNode.parent)) { |
There was a problem hiding this comment.
Suggested change
| if(ancestorNode&&ancestorNode.getLastToken()===contextToken&&isObjectLiteralExpression(ancestorNode.parent)){ | |
| if(ancestorNode?.getLastToken()===contextToken&&isObjectLiteralExpression(ancestorNode.parent)){ | |
| if (completionKind === CompletionKind.ObjectPropertyDeclaration && contextToken && | ||
| findPrecedingToken(contextToken.pos, sourceFile, contextToken)?.kind !== SyntaxKind.CommaToken && | ||
| (isMethodDeclaration(contextToken.parent.parent) || isSpreadAssignment(contextToken.parent) || findAncestor(contextToken.parent, (node: Node) => isPropertyAssignment(node))?.getLastToken() === contextToken || |
There was a problem hiding this comment.
I think the spread assignment case should be handled the same as the property assignment case, because the spread assignment can also contain any expression, like this:
constv: I={
...a.b.c.d}so I think the condition here should be something like:findAncestor(contextToken.parent, node => isPropertyAssignment(node) || isSpreadAssignment(node))?.getLastToken()
| (getLineAndCharacterOfPosition(contextToken.getSourceFile(), contextToken.getEnd()).line !== getLineAndCharacterOfPosition(contextToken.getSourceFile(), position).line))) { | ||
| return contextToken.parent.parent; | ||
| } | ||
| const ancestorNode = findAncestor(parent, (node: Node) => isPropertyAssignment(node)); |
There was a problem hiding this comment.
Same as my comment above regarding spread assignments, I think you can handle them together with property assignment:const ancestorNode = findAncestor(parent, node => isPropertyAssignment(node) || isSpreadAssignment(node));
TypeScript Bot (typescript-bot)
commented
Jun 23, 2023
Contributor
Gabriela Araujo Britto (@gabritto) Here they are:CompilerComparison Report - main..52899
System
Hosts
Scenarios
TSServerComparison Report - main..52899
System
Hosts
Scenarios
StartupComparison Report - main..52899
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
You should really be using exporttypeObjectLiteralElementLike=PropertyAssignment|ShorthandPropertyAssignment|SpreadAssignment|MethodDeclaration|AccessorDeclaration;But I've noticed you don't provide the comma for accessors. interfaceSomeType{first: number;second: number}exportletx: SomeType={getfirst(){return42}/**/} |
navya9singh
commented
Jun 23, 2023
MemberAuthor
Yeah, I've added a test for that now. |
TypeScript Bot (typescript-bot)
commented
Jun 24, 2023
Contributor
Gabriela Araujo Britto (@gabritto) Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
Daniel Rosenwasser (DanielRosenwasser)
approved these changes
Jun 26, 2023
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.
Andrew Branch (andrewbranch)
approved these changes
Jun 27, 2023
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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pr provides object member completion with a missing comma and inserts a comma as well. It inserts comma in cases when insertion is requested on the same line,
color: {primary: "red" /*$*/}or on different lines,and excludes cases which do not need a comma, like
const i: I = {/*$*/};orFixes#52604