Skip to content
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.

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

Enable method signature completions for object literal that is implementing an interface #46590

Closed
mjbvz opened this issue Oct 29, 2021 · 10 comments �� Fixed by #48168 or microsoft/vscode#145941
Assignees
Labels
Domain: Completion Lists The issue relates to showing completion lists in an editor Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Oct 29, 2021

Bug Report

🔎 Search Terms

  • Method signature completions
  • suggest / suggestions
  • completions

🕗 Version & Regression Information

4.5.0-dev.20211029

💻 Code

For the code:

interface IFoo {
    bar(x: number): void;
}

const obj: IFoo = {
    |
}

Trigger suggestions inside the object literal

🙁 Actual behavior

We get a suggestion for bar but accepting it only completes bar

🙂 Expected behavior

Accepting the completion for bar should also insert the method signature:

interface IFoo {
    bar(x: number): void;
}

const obj: IFoo = {
    bar(x: number): void {
        
    }
}
@andrewbranch
Copy link
Member

This was a known limitation of the PR for 4.5, but probably won't be difficult to add in 4.6.

@andrewbranch andrewbranch added Domain: Completion Lists The issue relates to showing completion lists in an editor Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript labels Oct 29, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.6.0 milestone Oct 29, 2021
@a-tarasyuk
Copy link
Contributor

@andrewbranch Should the completion of object literal elements be allowed with the includeCompletionsWithClassMemberSnippets option, or should a new option be added?

@andrewbranch
Copy link
Member

@gabritto and I briefly discussed this when coming up with the name at the time. IIRC we didn’t really feel great about any options and mostly just lamented that we don’t have a way of presenting hierarchical preferences. One related thought is that for object literals we might want to give an option for whether to always use arrow function properties or to match the declaring interface—I personally tend to use arrow functions for object literal members even when the member is written as a method signature on the interface. If we do want to make that configurable, I think we would need a separate option. @gabritto @DanielRosenwasser @mjbvz what do you think?

@mjbvz
Copy link
Contributor Author

mjbvz commented Nov 12, 2021

@andrewbranch I think we can surface these settings sort of hierarchically in VS Code if we want. Right now have

javascript.suggest.includeCompletionsWithClassMemberSnippets

but we could expand this to something like:

javascript.suggest.classMemberSnippets.enabled
javascript.suggest.classMemberSnippets.methodStyle: "arrow"

We'd keep two distinct settings on the TS side, this would just be for the VS Code settings UI

@mjbvz
Copy link
Contributor Author

mjbvz commented Nov 12, 2021

Let's make sure we decide on on this before 4.5 goes so I can rename the VS Code setting if needed

@andrewbranch
Copy link
Member

@mjbvz that only affects the presentation of the setting title in the UI, right?

image

This is better than nothing, but it still appears as a flat list and is kind of hard to navigate. But, properly organizing the data leaves room for UI improvements later. I like your suggestion for the setting name, though I’m now having stronger doubts about using the term “class” to control something for object literals as well. Maybe at that point we can do javascript.suggest.objectMemberSnippets.*.

@andrewbranch
Copy link
Member

I think that change wouldn’t require any changes on the TS side right now; the new name can still map to the existing TS preference name.

@gabritto
Copy link
Member

@andrewbranch I think we can surface these settings sort of hierarchically in VS Code if we want. Right now have

javascript.suggest.includeCompletionsWithClassMemberSnippets

but we could expand this to something like:

javascript.suggest.classMemberSnippets.enabled
javascript.suggest.classMemberSnippets.methodStyle: "arrow"

We'd keep two distinct settings on the TS side, this would just be for the VS Code settings UI

I agree with @andrewbranch, I think this is better than what we have now, but I think we want to have something more structured for these options in the future.

Also, a few other things on my mind:

  • Not sure if it's relevant or if it's just a personal nit, but I'm still concerned that there's a javascript option when we don't yet support this feature in JS and don't have concrete plans for it (yet -- I opened an issue: Support class member snippet completions in JS files #46738).
  • This is just an intuition for now, but I sense it's likely we'd want to separately enable completions for objects vs classes. They are similar scenarios at first glance, but the differences tend to add up quickly. For instance, the arrow vs function preference @andrewbranch brought up, but also there might be type inference edge cases for objects that are not there for classes, and completions for class properties is easier to deal with than for object properties (which inevitably brings up the question of what value to assign to the property in the completion, or to put an empty placeholder).
    I might be wrong though. And if we'd like to have structured/hierarchical settings for this in the future, I could imagine something like suggest.methods and grouped under that, suggest.methods.classes and suggest.methods.objects (illustrative names).
@andrewbranch andrewbranch removed this from the TypeScript 4.6.0 milestone Nov 15, 2021
@andrewbranch
Copy link
Member

we'd want to separately enable completions for objects vs classes

Agreed. And you’re right, there are other significant differences. Parameters are contextually typed in object literals so there’s no need to write their type annotations. Also, in a class member position you can be fairly sure that a user starting to type a method name intends to type a whole method declaration. Not so in object literals, where it is possible to write a property assignment to an existing function instead:

import * as ts from "typescript";

const host: ts.LanguageServiceHost = {
  fileExis/*|*/
};

At this location we have no heuristics that would indicate whether the user intends to write a function literal / method declaration or a property assignment:

const host: ts.LanguageServiceHost = {
  fileExists: ts.sys.fileExists,
};

so we cannot replace the completion containing just the fileExists name. We would need to offer both options, and the two completion items need to be easy to disambiguate in the list. We need to be careful that users don’t find themselves automatically getting whole method declarations when they meant to only get a property name. It sounds like this feature needs a bit of careful UX design work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Completion Lists The issue relates to showing completion lists in an editor Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
7 participants