fix(41259) : JS autocomplete doesn't work for object literal shorthands

[orange4glace]

Fixes #41259

Still have to work with failed testcase completionListAtIdentifierDefinitionLocations_destructuring

[andrewbranch]

@typescript-bot pack this

[typescript-bot]

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

[orange4glace]

It seems that all tests should be passed to pack. I've just updated the failing one to fit the current build.

[andrewbranch]

Oh, that's annoying. I thought it just had to build cleanly. Thanks!

@typescript-bot pack this again pls

[typescript-bot]

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

[typescript-bot]

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/89408/artifacts?artifactName=tgz&fileId=C85F15B03B8CC0441C24B7DB52C54BD7D244F91A68C45B0E7ED2D9222E880CAA02&fileName=/typescript-4.2.0-insiders.20201116.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[andrewbranch]

A couple notes:

1. The failing test case is a little weird after all; the globals only show up because of the cumulative parsing weirdness that comes from the several preceding lines. My impression looking at that test was that every line was intended to be a clean parsing context. It’s valuable to ensure that a half-written statement has good completions as you’re writing it, but less so that the fourth half-written statement in a row behaves well. I would actually just split that test file up into a file per line.

2. Having an any or undefined contextual type is a little too restrictive. You also want to account for object, unknown, and possibly string index signatures:

   declare function f1(obj: unknown): void;
   declare function f2(obj: object): void;
   declare function f3(obj: Record<string, any>): void;
   
   declare var foo;
   f1({ /**/ });
   f2({ /**/ });
   f3({ /**/ });

   But the index signature case brings up the question of whether the types need to match up:

   declare const foo: string;
   const obj: { [key: string]: number } = { /* `foo` here? It would error because string is not assignable to number */ };

   On one hand, we don’t do any kind of filtering of member completions in the presence of a contextual type, but that’s more justifiable because you might dot off of one and continue the expression, resulting in a different type. More practically, it’s going to be too expensive to check the assignability of every identifier in the completions list, so I think the choice is between offering all the completions, even wrong ones, or none.

   You also want to test cases where the contextual type is a type parameter:

   declare function foo<T>(obj: T): T; // and another where `T extends object`, maybe `T extends {}`
   var blah: string;
   foo({ /**/ })

   I think this might Just Work once you consider unknown and object contextual types, but it’s worth testing.

Thanks for working on this!

[typescript-bot]

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src'. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src'.

[orange4glace]

Thanks for the much detailed feedback! It was very helpful.

1. As you've mentioned, I've modified the code so the autocompletion are also given to the unknown, object, {}, Record<string, any> , an object having String index type, <T extends object>, <T extends {}>.

2. More practically, it’s going to be too expensive to check the assignability of every identifier in the completions list, so I think the choice is between offering all the completions, even wrong ones, or none.

IMHO, offering all the completions would be better even if some of are wrong. The difference between the contextual type case is that dot can't be followed in shorthand completion so there's no chance that ending up with different type. Even though, as you mentioned, since checking all the assignability of the list is expensive, just offering them all rather than nothing could give more good user experience, I think.

3. If those types are good to go, I think types with Object and Object | { a: string}(Union type including empty object type) could be also good to go. In addition, the code below has no error with using foo inside of the object literal so it seems reasonable to suggest auto completion.

declare const foo: number;
interface Empty {}
interface Typed { typed: number; }

declare function f10<T extends Object>(obj: T): void;
declare function f13<T extends (Empty | Object | Typed)>(obj: T): void;

f10({foo});
f13({foo});

Playground, v4.0.5
The last committed code contains implementation of it.
I'm expecting that you can test it on the playground once you pack it.
Please give a thoughts about this suggestion!

4. While I'm implementing this, I needed to check if the object is empty object type(ex: {}) or not.
   Since it seems that there's no such function in completions.ts, I made some extra exports from checker.ts to use in completions.ts which are,

• isTypeSubsetOf()
• isEmptyObjectType()
• globalObjectType (@internal getGlobalObjectType())

I'm not sure if it is legal to do this only for my intention.
Please give an advice whether it is okay anyway? (also, looks like typescript-bot is complaining about the lib update and what can I do this for it :o)

5. I made a split the mentioned test file into multiple files using // @Filename directive.

I would actually just split that test file up into a file per line.

Thanks !

[andrewbranch]

While I'm implementing this, I needed to check if the object is empty object type(ex: {}) or not.
Since it seems that there's no such function in completions.ts, I made some extra exports from checker.ts

I wonder if you can avoid all this by seeing if checker.getPropertiesOfType(completionsType) returns an empty array. It would return false for non-reduced unions like object | { a: string }, but I actually think that’s ok—it seems reasonable to treat the property a in this case as an infinitely more preferable completion than random identifiers in scope. In general, I think we should start by being very cautious about flooding new completions into positions where some completions are already offered. Offering new completions where none are currently offered is much more clearly a win.

Edit: To clarify, I’m not 100% confident that checker.getPropertiesOfType(completionsType) is the best strategy, just brainstorming. I would like to find a way not to use those very low-level checker functions.

[orange4glace]

it seems reasonable to treat the property a in this case as an infinitely more preferable completion than random identifiers in scope. In general, I think we should start by being very cautious about flooding new completions into positions where some completions are already offered.

Oh actually I didn't think about that! I agree with that.
I just tried checker.getPropertiesOfType(completionsType) and it generally works as expected including not offering global completions with union types. But also, Object(with capital 'O') is not getting completions while Record<number, any> and { [key: number]: number } are getting them. (Actually I'm not very confident with Object should get completions or not?)
I'll investigate it more what I can do to filter them out :)
Thanks!

[orange4glace]

I realized that there's already a code which is getting properties of the type.

Still, one thing that blocks me from implementing it is distinguishing the Object type since the Object type has some properties such as constructor, toString(), hasOwnProperty().
I have compared the completion type with globalObjectType to achieve this which ends up with exposing globalObjectType from checker.ts.
I think that this is the simplest way to implement it even though it introduces new internal API (and maybe also a low-level function as you said?)

[andrewbranch]

I don’t think we should do anything special for capital-O Object since it is pretty much always a mistake for people to use it. (They mean lowercase-o object almost 100% of the time.)

[orange4glace]

That actually makes a lot easier to me ;) Just fixed it!

[andrewbranch]

If I can nitpick the format of this test a little bit, could we rename/reorder the function declarations such that they’re obviously grouped by expected behavior? So you can quickly scan that e.g. f1–f10 should show foo in completions, then there’s a blank line, and the next group doesn’t show foo, etc... We also want to add a case where any is the contextual type, and a case where there is no contextual type, like const x = { /** }.

[orange4glace]

Actually I had also considered about this and I think you're right. any and non-contextual type are also added. Thanks!

[andrewbranch]

@typescript-bot pack this again please and thank you

[typescript-bot]

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

[typescript-bot]

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/89565/artifacts?artifactName=tgz&fileId=CFB9251FBB14D8399537B75E78771CFA306BB45A75BDE889B383C8953E9579A702&fileName=/typescript-4.2.0-insiders.20201119.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[andrewbranch]

Thanks for your work on this, @orange4glace. If you’re ready for review, push the button and I’ll get some other team members to experiment with it on the playground.

[orange4glace]

I'm much appreciated that I can contribute to the one of my favorite language with your lots of help :)

[andrewbranch]

We very much appreciate the contribution! Lots of people are on vacation this week, so it might not get another review until next week. This should be on track for 4.2 regardless though.

[sandersn]

I just played with this:

1. I literally thought the lack of this feature was a bug last week. I love this feature.
2. I couldn't get completions for the following examples (in TS, I didn't try JS):

Given

const variable1 = 1
const variable2 = 2

// nothing after a missing comma
const o = {
  variable1
  va/**/
}

// nothing in an unclosed literal
const p = {  var/**/
interface A {
 // ...
}

I didn't read the original bug too closely so I'm not sure if fixing that is feature creep or not.
Edit: I don't have my editor (emacs) set up to insert matching brackets, so it is quite common for my buffer to be in the example states.

[orange4glace]

Thanks for the feedback @sandersn!

// nothing in an unclosed literal

Actually it was my mistake. I was checking NodeFlags.ThisNodeHasError with non-contextual type which is not necessary.
(You can check const o: any = { var/**/ works which has a contextual type in the playground)
I've applied it to the latest commit.

// nothing after a missing comma

I agree that it should be worked either since autocomplete also works in the case which is,

const o = {
    variable1: variable1
    variable2: var/**/
}

I've also applied this to the latest commit.

Please leave comments for any thoughts for the code or other things!
Thanks.

[andrewbranch]

It is a bit of a pain, but can we try to keep using exact in all these tests? Hopefully in this case it’s just exact: completion.globals. exact is useful in some of these other tests for ensuring that locals get sorted before globals, which is important to this feature. I’m almost on the fence about whether globals should show up here at all (who wants to declare something like const obj = { Object, Date, setTimeout }?), but I think as long as they get sorted below locals, it’s fine.

[orange4glace]

Sure! I'll fix it after I find out how I can do this elegantly. :)

I’m almost on the fence about whether globals should show up here at all

I agree with that by the way.

[orange4glace]

Fixed! except it uses globalsPlus for this case (#41731)

707e026#diff-758264b55071844e413a511bae1269991faea7f964e4d03cebb9a021ff42eb50R12

[andrewbranch]

It’s probably worth discussing the broader design point of whether globals should be offered as shorthand properties, or just locals:

const salamander = 0;

const o = { s/**/ };
// Completions:
//  - salamander 👍
//  - setTimeout 🤔 

I think it’s a bit odd for globals to show up, but I’m not sure it’s harmful, because

• None of these show up if we have any actual property suggestions from a contextual type; these only appear when we previously would have shown no completions
• Locals are sorted above globals

These two points mean that the globals are not crowding out better suggestions, even if they’re rarely useful themselves. Open to hearing other takes, though.

[andrewbranch]

I just thought of something else—assuming we decide to keep globals in the completions here, can we add a test that ensures that we don’t trigger auto-imports here? I think it’d be over the top to get shorthand property completions for things that aren’t even currently in scope at all.

Something like this

// @module: esnext

// @Filename: /a.ts
//// export const exportedConstant = 0;

// @Filename: /b.ts
//// const obj = { exp/**/

verify.completions({
  marker: "",
  exact: completion.globals, // I think?
  preferences: { includeCompletionsForModuleExports: true }
});

[orange4glace]

I think it’d be over the top to get shorthand property completions for things that aren’t even currently in scope at all.

I agree that offering auto-imports to completions might be annoying 👍
While I have implemented it, I realized that autocompletion includes its container variable, for example,

const obj = {
    o/**/ 
     ~~~~ obj
}

It is obviously a mistake to use its container variable (and which finally emits an error Block-scoped variable 'obj' used before its declaration.) so technically, I think we should get rid of it from completions.
On the hand, property value assignment also auto-completes its container variable like,

const obj = {
    value: o/**/
            ~~~~ obj
}

Playground

I don't know whether this is intended or not (like implementing it is kinda cumbersome? 🤔)

It’s probably worth discussing the broader design point of whether globals should be offered as shorthand properties, or just locals:

👍 for not offering global symbols since as you said, literally no one wants to re-declare such symbols which can be accessed globally.

[andrewbranch]

I don't know whether this is intended or not (like implementing it is kinda cumbersome? 🤔)

I don’t think it should be difficult to fix; I’m guessing it has just gone unnoticed. But I don’t think you need to worry about it in this PR since it appears unrelated. I can open a new bug for it.

[andrewbranch]

@sandersn @DanielRosenwasser either of you want to weigh in on whether these completions should include globals (#41539 (comment))? I don’t feel super strongly about it for the reasons I outlined in that comment. But I want to make a decision and get this merged soon.

[andrewbranch]

Thanks @orange4glace!

[orange4glace]

Oh, I haven't noticed that this merged.
Thanks @andrewbranch! I couldn't get this done if there wasn't your help. ;)