Skip to content

Adds support for class completions after ASI inserted class property definition #32243

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 15, 2019
Merged

Adds support for class completions after ASI inserted class property definition #32243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e9d098
Adds support for completions after ASI inserted expressions
Jul 3, 2019
e55f97e
Updates the baselines for the typo fixes
orta Jul 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1538,7 +1538,7 @@ namespace ts.Completions {
* Relevant symbols are stored in the captured 'symbols' variable.
*/
function tryGetClassLikeCompletionSymbols(): GlobalsSearch {
const decl = tryGetObjectTypeDeclarationCompletionContainer(sourceFile, contextToken, location);
const decl = tryGetObjectTypeDeclarationCompletionContainer(sourceFile, contextToken, location, position);
if (!decl) return GlobalsSearch.Continue;

// We're looking up possible property names from parent type.
Expand Down Expand Up @@ -2155,7 +2155,7 @@ namespace ts.Completions {
* Returns the immediate owning class declaration of a context token,
* on the condition that one exists and that the context implies completion should be given.
*/
function tryGetObjectTypeDeclarationCompletionContainer(sourceFile: SourceFile, contextToken: Node | undefined, location: Node): ObjectTypeDeclaration | undefined {
function tryGetObjectTypeDeclarationCompletionContainer(sourceFile: SourceFile, contextToken: Node | undefined, location: Node, position: number): ObjectTypeDeclaration | undefined {
// class c { method() { } | method2() { } }
switch (location.kind) {
case SyntaxKind.SyntaxList:
Expand All @@ -2165,9 +2165,15 @@ namespace ts.Completions {
if (cls && !findChildOfKind(cls, SyntaxKind.CloseBraceToken, sourceFile)) {
return cls;
}
break;
case SyntaxKind.Identifier: // class c extends React.Component { a: () => 1\n compon| }
if (isFromObjectTypeDeclaration(location)) {
return findAncestor(location, isObjectTypeDeclaration);
}
}

if (!contextToken) return undefined;

switch (contextToken.kind) {
case SyntaxKind.SemicolonToken: // class c {getValue(): number; | }
case SyntaxKind.CloseBraceToken: // class c { method() { } | }
Expand All @@ -2179,7 +2185,13 @@ namespace ts.Completions {
case SyntaxKind.CommaToken: // class c {getValue(): number, | }
return tryCast(contextToken.parent, isObjectTypeDeclaration);
default:
if (!isFromObjectTypeDeclaration(contextToken)) return undefined;
if (!isFromObjectTypeDeclaration(contextToken)) {
// class c extends React.Component { a: () => 1\n| }
if (getLineAndCharacterOfPosition(sourceFile, contextToken.getEnd()).line !== getLineAndCharacterOfPosition(sourceFile, position).line && isObjectTypeDeclaration(location)) {
Copy link
Member

Choose a reason for hiding this comment

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

Adding the isObjectTypeDeclaration(location) miiiight have been overly conservative, but I suggested it to err on the side of caution—the smallest reasonable net I could think of that would catch the repros provided.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please reverse the order of checks here (that is checking isObjectTypeDeclaration(location) before line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching the if ordering changes the behavior from what it previously was, and starts to trigger behavior which I think we don't want.

This fourslash test fails:

/// <reference path="fourslash.ts" />

////function /*a*/ ;
////function* /*b*/ ;
////interface I {
////    abstract baseMethod(): Iterable<number>;
////}
////class C implements I {
////    */*c*/ ;
////    public */*d*/
////}
////const o: I = {
////    */*e*/
////};
////1 * /*f*/

verify.completions(
    { marker: ["a", "b"], exact: undefined, isNewIdentifierLocation: true },
    { marker: ["c", "d"], exact: ["baseMethod"], isNewIdentifierLocation: true },
    { marker: "e", exact: ["baseMethod"] },
    { marker: "f", includes: [{ name: "Number", sortText: completion.SortText.GlobalsOrKeywords }] },
);

With

  1) fourslash tests
       tests/cases/fourslash/completionsGeneratorFunctions.ts
         fourslash test completionsGeneratorFunctions.ts runs correctly:
     Error: At f: Expected 'isNewIdentifierLocation' to be false, got true

Which is returning an empty array of completions

return location;
}
return undefined;
}
const isValidKeyword = isClassLike(contextToken.parent.parent) ? isClassMemberCompletionKeyword : isInterfaceOrTypeLiteralCompletionKeyword;
return (isValidKeyword(contextToken.kind) || contextToken.kind === SyntaxKind.AsteriskToken || isIdentifier(contextToken) && isValidKeyword(stringToToken(contextToken.text)!)) // TODO: GH#18217
? contextToken.parent.parent as ObjectTypeDeclaration : undefined;
Expand Down
2 changes: 1 addition & 1 deletion src/services/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ namespace ts {
}

export interface CompletionInfo {
/** Not true for all glboal completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
/** Not true for all global completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
isGlobalCompletion: boolean;
isMemberCompletion: boolean;

Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5398,7 +5398,7 @@ declare namespace ts {
argumentCount: number;
}
interface CompletionInfo {
/** Not true for all glboal completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
/** Not true for all global completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
isGlobalCompletion: boolean;
isMemberCompletion: boolean;
/**
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5398,7 +5398,7 @@ declare namespace ts {
argumentCount: number;
}
interface CompletionInfo {
/** Not true for all glboal completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
/** Not true for all global completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */
isGlobalCompletion: boolean;
isMemberCompletion: boolean;
/**
Expand Down
21 changes: 21 additions & 0 deletions tests/cases/fourslash/completionEntryAfterASIExpressionInClass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference path='fourslash.ts'/>

//// class Parent {
//// protected shouldWork() {
//// console.log();
//// }
//// }
////
//// class Child extends Parent {
//// // this assumes ASI, but on next line wants to
//// x = () => 1
//// shoul/*insideid*/
//// }
////
//// class ChildTwo extends Parent {
//// // this assumes ASI, but on next line wants to
//// x = () => 1
//// /*root*/ //nothing
//// }

verify.completions({ marker: ["insideid", "root"], includes: "shouldWork", isNewIdentifierLocation: true });