Complete `constructor` keyword after property declaration #43654

z0gSh1u · 2021-04-13T09:13:54Z

Fixes #43650
and more.

This is my first pull-request here. Help and suggestions are appreciated.

All these various situations are fixed, expanding the case in original issue:

implicit any property, property with type, property with initializer...
literal initializer, object initializer, array initializer, identifier initializer, function execution initializer...
fully present constructor, partly present constructor, no presence...
different semicolon styles...
w/ or w/o semicolon or newline...
and so on.

Please refer to the test case for detail:

tests/cases/fourslash/completionConstructorKeywordAfterPropertyDeclaration.ts

typescript-bot · 2021-04-13T09:14:00Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

ghost · 2021-04-13T09:14:06Z

All CLA requirements met.

DanielRosenwasser · 2021-04-13T18:34:26Z

This seems like an oddly-specific fix, and I'm not sure if it really identifies the root problem. I think we need to understand why we weren't offering up completions before.

z0gSh1u · 2021-04-14T02:38:51Z

In my opinion, this is a missing case of getting completions during class declaration.

The function tryGetObjectTypeDeclarationCompletionContainer checks the contextToken to give a completion.

TypeScript/src/services/completions.ts

Line 2835 in f74f9ca

switch (contextToken.kind) {

However, it doesn't consider the implicit any case like:

class Foo {
   bar // <- context token is identifier when w/o semicolon
   // bar; // <- context token is semicolon when with semicolon
   constructor/**/()
}

since there is no switch-case for contextToken as an Identifier. And the switch-case for SemicolonToken doesn't cover this situation either.

So it falls back to default, and checks the contextToken using

TypeScript/src/services/completions.ts

Line 2737 in f74f9ca

function isClassMemberCompletionKeyword(kind: SyntaxKind) {

This function doesn't consider the situation that contextToken is an Identifier either (but does for like AsyncKeyword), resulting no suggestion return for completions.

What's more, for case with a TypeReference like:

class Foo {
  bar: number // <- context token w/o semicolon
  consturctor/**/()
}

The completion is blocked beforehand by

TypeScript/src/services/completions.ts

Line 1816 in f74f9ca

isSolelyIdentifierDefinitionLocation(contextToken) ||

which is in fact a different problem from the previous one.

The fix for this is made referring to a similar issue:

TypeScript/src/services/completions.ts

Line 2377 in f74f9ca

    
           // Don't block completions if we're in `class C /**/`, because we're *past* the end of the identifier and might want to complete `extends`.

andrewbranch · 2021-04-20T17:03:11Z

You need to actually check for an ASI-triggering newline if you want to prevent a completion at

class C {
  blah /*| constructor*/
}

I don’t know how important that is, though.

z0gSh1u · 2021-04-21T04:23:29Z

Hi, I'm not very sure of what you mean. I'm basically allowing instead of preventing completions. And the behavior for cursor at that position should be unchanged. Do these changes affect ASI in some way?

andrewbranch · 2021-04-21T16:12:09Z

tests/cases/fourslash/completionConstructorKeywordAfterPropertyDeclaration.ts

+
+////class A {
+////  bla
+////  constructor/*1*/(props) {}


The presence of the full constructor keyword is changing the behavior of the completions. It seems like what you really want to test is

class A { bla /**/ }

and

class A { bla co/**/ }

right? Those currently don’t work.

We’ll have to revisit my comment about newlines after this issue is fixed—you’re right that the behavior is currently unchanged, but I think it’s because of this issue.

Got it. This is a more universal situation. And I've found that completions are not working either for snippet like this:

class C { blah = 123 // property with an immediate assignment constructor/**/ }

I'll take a deeper dive into these problems.

z0gSh1u · 2021-04-22T09:44:03Z

Updated. New fix available now.

andrewbranch · 2021-04-22T16:06:39Z

@typescript-bot pack this

typescript-bot · 2021-04-22T16:06:42Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 59756d3. You can monitor the build here.

typescript-bot · 2021-04-22T16:28:22Z

Hey @andrewbranch, 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/101548/artifacts?artifactName=tgz&fileId=0893DA32FEBAAB115A1A95CFDCF527ABB9460D922231E4A45C3D77411A8B80EE02&fileName=/typescript-4.3.0-insiders.20210422.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

andrewbranch · 2021-04-22T16:43:07Z

Ok, so this is what I meant about checking newlines: https://www.staging-typescript.org/play?ts=4.3.0-pr-43654-9#code/MYGwhgzhAEDC0G8BQ1oCNwAtrAPQCp9ckBfIA

If you trigger completions at the comment, you now get a completion for constructor which is invalid due to the lack of a semicolon or newline. Because of ASI, either a semicolon or a newline makes constructor a valid next token, but it’s not valid here.

z0gSh1u · 2021-04-23T10:03:30Z

I've adopted a validation using newline or semicolon. The completions for these two cases are blocked now as the original behavior in 4.3.0-beta:

class A {
  blah /*|con*/
}
class B {
  blah: number /*|con*/
}

And constructor will not be suggested for this:

class C {
  blah
  /**/
}

since

class C {
  blah
   = 123
}

is legal. Unless user types a c at that position.

During the debugging, I've found another issue of property with an initializer that might be controversial. In 4.3.0-beta, completions of private / protected / public / constructor can be triggered like this:

class C {
  blah = blah /*|p*/
}
// PLAYGROUND: https://www.staging-typescript.org/play?jsx=0&ts=4.3.0-beta#code/MYGwhgzhAEDC0G8BQ1oCNwAtoF51egAcB6AKlOKQF8g

It looks unlike an intended one. But I'm not sure and haven't fix this since it is guaranteed by some test cases like tests\cases\fourslash\completionsClassPropertiesAssignment.ts.

andrewbranch · 2021-04-23T15:54:48Z

src/services/completions.ts

+                return getLinesBetweenPositions(sourceFile, contextToken.end, previousToken.end) > 0 ||
+                    contextToken.kind === SyntaxKind.SemicolonToken;


Slightly cheaper:

Suggested change

return getLinesBetweenPositions(sourceFile, contextToken.end, previousToken.end) > 0 ||

contextToken.kind === SyntaxKind.SemicolonToken;

return contextToken.kind === SyntaxKind.SemicolonToken ||

positionsAreOnSameLine(contextToken.end, previousToken.end, sourceFile);

Also, can you move this function declaration outside of the flow of its parent function? Our loosely-enforced convention is to put helper functions at the bottom of their parent, but it looks like this one could move outside of isSolelyIdentifierDefinitionLocation entirely.

Got it.

+ return contextToken.kind === SyntaxKind.SemicolonToken || + !positionsAreOnSameLine(contextToken.end, previousToken.end, sourceFile); ^~~~~~

exactly.

andrewbranch · 2021-04-23T16:10:33Z

src/services/completions.ts

+            isPropertyBeforeConstructorInitalized(getAncestor(location, SyntaxKind.ClassDeclaration) as ClassDeclaration, location.parent as ClassElement) ||
+            contextToken.kind === SyntaxKind.SemicolonToken
+        )) {
+            return getAncestor(contextToken, SyntaxKind.ClassDeclaration) as ObjectTypeDeclaration;


I believe we will not produce a ConstructorKeyword outside a class (it will just be an Identifier if it appears), but the class node could be a ClassExpression, not only a ClassDeclaration.

Got it. Replaced using isClassLike to predicate.

andrewbranch · 2021-04-23T16:13:36Z

src/services/completions.ts

+        // class c { prop: number \n constructor| }
+        // class c { prop = somewhat \n constructor| }
+        // and also for `;` replacing `\n`
+        if (location.kind === SyntaxKind.ConstructorKeyword && (


It looks like this whole condition applies only once you’ve typed the entire constructor keyword, and stops applying as soon as you’ve typed the open parenthesis afterwards, which is not really a time when completions are likely to be triggered, or when they will be valuable. This might be technically correct, but I’m confused about what purpose it’s serving.

This is the fix for the most original issue. #43650

Manually triggered completion (Ctrl+Space in Windows) doesn't offer constructor for this:

class C { blah // w/ w/o semicolon, type, initializer constructor/**/ // must be fully present }

It looks like that the full presence of constructor is changing the behavior of completions in some way.

The cause seems to be a missing case in tryGetObjectTypeDeclarationCompletionContainer. Removing this will result in no suggestion. But the conditions here are unnecessarily complex. I'm trying to simplify it.

I think I disagree with the premise that #43650 is worth more than a line of code to fix. From the original issue opened against VS Code, it looks like someone was just hunting for inconsistencies, not experiencing a legitimate UX issue. I’m not against fixing it, but I am against fixing it if no one can explain why it matters and it takes a multiline if statement, a bunch of explanatory comments, and a dozen fourslash tests.

😆 Yep. The latest fix is of 2 LOC.

andrewbranch · 2021-04-23T16:17:32Z

src/services/completions.ts

+                && isClassLike(previousToken.parent.parent)
+                && isPropertyDeclaration(contextToken.parent)
+                // After considering different contextToken...
+                && (isIdentifier(contextToken) || isTypeKeyword(contextToken.kind))


Is this condition necessary/correct? If the previous property declaration has a type annotation or an initializer, there are lots of ways it might be something other than an identifier or type keyword. But those cases seem to work already... are we able to just drop this line?

Correct. This is already supported by some other code. Cases like

type SomeType = number class C { blah: SomeType; /**/ }

are already working in 4.3.0-beta.

andrewbranch · 2021-04-23T16:19:46Z

tests/cases/fourslash/completionConstructorKeywordAfterPropertyDeclaration.ts

+//// }
+//// class R {
+////   blah
+////   /*18*/ // dont complete since `blah \n = 123` is legal


But constructor is also legal here, so why not offer it?

That's right.

andrewbranch · 2021-04-23T16:22:01Z

tests/cases/fourslash/completionConstructorKeywordAfterPropertyDeclaration.ts

+//// class G {
+////   blah = 123; con/*7*/
+//// }
+//// // situations that `constructor` is fully present


All of these are fine to have, and correct, but as far as I understand, essentially pointless. I don’t think they’re particularly important to test, and I definitely think that little to no implementation code should be written to make these cases pass.

Kindly refer to #43654 (comment) maybe.

z0gSh1u · 2021-04-23T19:56:14Z

constructor should not but is now still suggested at:

class T {
  blah: number con/**/
}
const SomeConst = 123
class U {
  blah = SomeConst con/**/
}

I'm still working on this.

andrewbranch · 2021-04-23T20:01:26Z

The simplifications are looking great. Let me know when you’re ready for another review. Thanks!

z0gSh1u · 2021-04-24T16:43:21Z

The fix should be well-rounded and reliable enough now. Please refer to the test case:

https://github.com/microsoft/TypeScript/blob/70adf96ed0b9445ec0468ff038588247f87a7c81/tests/cases/fourslash/completionConstructorKeywordAfterPropertyDeclaration.ts

for detail.

What's more, we've got a byproduct that, for 15-21 in the test case of "situations that constructor should not be suggested", completions for private / public / protected are also blocked, which should be more reasonable comparing to 4.3.0-beta. (See #43654 (comment))

At last, kindly ping @andrewbranch .

andrewbranch · 2021-04-26T16:26:56Z

src/services/completions.ts

@@ -2370,6 +2370,31 @@ namespace ts.Completions {
                    return isPropertyDeclaration(contextToken.parent);
            }

+            // If `constructor` is totally not present, but we request a completion manually at a space...
+            if (contextToken === previousToken && isPreviousPropertyDeclarationTerminated(contextToken, position)) {
+                return false; // Don't block completions.


Do we even know that the current location is in a class right here?

Agree. Narrowing this fix to inside a ClassLike should be more reliable. I've edited the code.

andrewbranch

Thanks!

z0gSh1u · 2021-04-30T00:14:48Z

Thanks for your help. Have a nice day!

Complete constructor keyword after property declaration.

0865bd4

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 13, 2021

typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 13, 2021

z0gSh1u changed the title ~~Complete constructor keyword after property declaration~~ [WIP] Complete constructor keyword after property declaration Apr 13, 2021

Fix logical errors.

6edfc26

z0gSh1u changed the title ~~[WIP] Complete constructor keyword after property declaration~~ Complete constructor keyword after property declaration Apr 13, 2021

DanielRosenwasser requested review from sheetalkamat and andrewbranch April 14, 2021 20:07

andrewbranch requested changes Apr 21, 2021

View reviewed changes

Fix for more universal situations.

59756d3

Only provide completions if property declaration is terminated.

d824629

andrewbranch requested changes Apr 23, 2021

View reviewed changes

Simplify many logical conditions.

dcadad3

Make the fix more reliable.

70adf96

andrewbranch reviewed Apr 26, 2021

View reviewed changes

Narrowing the fix.

939285c

sandersn assigned andrewbranch Apr 29, 2021

andrewbranch approved these changes Apr 29, 2021

View reviewed changes

andrewbranch merged commit 4ecb563 into microsoft:master Apr 29, 2021

		return getLinesBetweenPositions(sourceFile, contextToken.end, previousToken.end) > 0 \|\|
		contextToken.kind === SyntaxKind.SemicolonToken;

Complete constructor keyword after property declaration #43654

Complete constructor keyword after property declaration #43654

Uh oh!

Conversation

z0gSh1u commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Apr 13, 2021

Uh oh!

ghost commented Apr 13, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielRosenwasser commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

z0gSh1u commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewbranch commented Apr 20, 2021

Uh oh!

z0gSh1u commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

z0gSh1u commented Apr 22, 2021

Uh oh!

andrewbranch commented Apr 22, 2021

Uh oh!

typescript-bot commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewbranch commented Apr 22, 2021

Uh oh!

z0gSh1u commented Apr 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

z0gSh1u Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

z0gSh1u Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

z0gSh1u Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

z0gSh1u Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Complete `constructor` keyword after property declaration #43654

Complete `constructor` keyword after property declaration #43654

z0gSh1u commented Apr 13, 2021 •

edited

Loading

ghost commented Apr 13, 2021 •

edited by ghost

Loading

DanielRosenwasser commented Apr 13, 2021 •

edited

Loading

z0gSh1u commented Apr 14, 2021 •

edited

Loading

z0gSh1u commented Apr 21, 2021 •

edited

Loading

typescript-bot commented Apr 22, 2021 •

edited

Loading

typescript-bot commented Apr 22, 2021 •

edited

Loading

z0gSh1u Apr 23, 2021 •

edited

Loading

z0gSh1u Apr 23, 2021 •

edited

Loading

z0gSh1u Apr 23, 2021 •

edited

Loading

z0gSh1u Apr 23, 2021 •

edited

Loading

z0gSh1u commented Apr 24, 2021 •

edited

Loading

z0gSh1u Apr 27, 2021 •

edited

Loading