Smart Select language service API

[andrewbranch]

Closes #29071

Protocol for VS & VS Code

export interface Location {
    line: number;
    offset: number;
}

export interface TextSpan {
    start: Location;
    end: Location;
}

export interface SelectionRangeRequest extends FileRequest {
    command: CommandTypes.SelectionRange; // 'selectionRange'
    arguments: SelectionRangeRequestArgs;
}

export interface SelectionRangeRequestArgs extends FileRequestArgs {
    locations: Location[];
}

export interface SelectionRangeResponse extends Response {
    body?: SelectionRange[];
}

export interface SelectionRange {
    textSpan: TextSpan;
    parent?: SelectionRange;
}

Also exposes a selectionRange-full tsserver command which accepts the same request format but responds with the language service response unchanged, which is the same except that TextSpan is a ts.TextSpan:

export interface TextSpan {
    start: number;
    length: number;
}

Implementation notes

Selection ranges walk up the AST, with a few different types of exceptions to the rule.

Adding artificial depth to flat nodes

Some TypeScript nodes feel like they have subtrees but are actually quite flat. Keeping them flat results in a jump from one selection to the next that feels too big. To remedy this, when asking for the children of these nodes, we synthesize SyntaxLists as intermediate nodes that group children together more intuitively, sometimes multiple levels deep. The most complex example is for mapped types. Consider

type M = { -readonly [K in keyof any]-?: any }

The actual tree for the type in this declaration looks like

MappedType
  ├ OpenBraceToken
  ├ MinusToken
  ├ ReadonlyToken
  ├ OpenBracketToken
  ├ TypeParameter
  │  ├ Identifier
  │  ├ InKeyword
  │  └ TypeOperator
  │      ├ KeyofKeyword
  │      └ AnyKeyword
  ├ CloseBracketToken
  ├ MinusToken
  ├ QuestionToken
  ├ ColonToken
  ├ AnyKeyword
  └ CloseBraceToken

But this belies the intuition of the way things are grouped. The two tokens in -readonly go together, the two in -? go together, the square brackets go with their contents, etc. The tree we actually want for selection purposes would look more like

MappedType
  ├ OpenBraceToken
  ├ SyntaxList
  │  ├ SyntaxList
  │  │  ├ MinusToken
  │  │  └ ReadonlyToken
  │  ├ SyntaxList
  │  │  ├ OpenBracketToken
  │  │  ├ TypeParameter
  │  │  │  ├ Identifier
  │  │  │  ├ InKeyword
  │  │  │  └ TypeOperator
  │  │  │     ├ KeyofKeyword
  │  │  │     └ AnyKeyword
  │  │  └ CloseBracketToken
  │  ├ SyntaxList
  │  │  ├ QuestionToken
  │  │  └ ColonToken
  │  └ AnyKeyword
  └ CloseBraceToken

This results in a better selection experience, but it's totally fake which also makes it totally subjective. I used two main strategies to make decisions that are ultimately opinions:

1. As someone who writes a lot of TypeScript in VS Code myself, I put myself in the shoes of a user selecting some of my own code and thought about the common edits I might want to make around a particular selection, and tried to add selection stops that facilitated those edits with minimal additional keystrokes.
2. I gauged some of my own opinions and expectations that weren’t reflected in the AST against @mjbvz’s. Where we agreed, I felt confident that synthesizing a node was justified.

Skipping redundant or uninteresting nodes

Identical selection ranges are deduped, but some others vary only in whitespace or semicolons and seemed to add little value. These nodes are skipped.

Adding JSDoc comments

An extra selection is added to nodes with JSDoc annotations so that the node can be selected both alone and with its comment.

Special handling for string literals

String literals don't separate their quotes from their contents in the AST, which is desirable for selection, so there’s special logic there. Template strings are especially unintuitive and have special handling to make sure you can get the whole string, just inside the quotes, around the outside of a substitution (${x}), and on the inside of a substitution.

[typescript-bot]

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

[mjbvz]

Protocol changes look good. I'll give it another quick test locally and let you know if I run into an big issues

[mjbvz]

Looks great. Just found a few odd results that cause VS Code to error:

---

export function loadRepos() {
  return {
    type: /**/LOAD_REPOS,
  };
}

Top level returned text span is:

        "textSpan": {
            "start": {
                "line": 1,
                "offset": 27
            },
            "end": {
                "line": 1,
                "offset": 27
            }
        },

---

describe('thing', () => {
    it('/**/', () => {});
});

Returned span is:

        "textSpan": {
            "start": {
                "line": 1,
                "offset": 20
            },
            "end": {
                "line": 1,
                "offset": 20
            }
        },

---

class HomePage {
    componentDidMount() {
        if (this.props.username/**/) { }
    }
}

Returned top level span is

 "textSpan": {
            "start": {
                "line": 2,
                "offset": 23
            },
            "end": {
                "line": 2,
                "offset": 23
            }
        },

---

[andrewbranch]

Hm, I can see why the empty string one is doing that, but the others seem odd. I'll check and see what it thinks it's doing, but it looks like I just need to add a filter to reject zero-width spans. Will update first thing tomorrow. Thanks for testing!

[andrewbranch]

Ohhh. Those really strange ones were caused by a possible bug in ts.positionBelongsToNode that @RyanCavanaugh and @rbuckton and I were discussing yesterday. I've since switched away from using that API, and just added a filter to ignore empty spans.

I can no longer repro the bugs you gave me, but I thought I stopped using ts.positionBelongsToNode before you tried it yesterday... I just added unit tests for those cases but I'll try the integration with your custom VS Code build too.

[RyanCavanaugh]

I agree the fourslash format looks like a disaster.

The motivation, though, is that in general position-based tests end up being a lot less readable and maintainable than DSL-based formats, so we try to avoid the former as much as possible.

It's impossible to look at these tests right now in the diff view, for example, and understand what the expected behavior is, and if something changes in the wire format, there's a bunch of updating that will have to happen. Updating the text is also very difficult - adding a single character in the middle of an existing test is going to require a lot of tedious math.

Having a one-off DSL has generally paid for itself in terms of long-term test maintenance. For example, we could imagine having something like this (bikeshed a bunch):

const test = `const { x, y: a, ...zs = {} } = {};`;
const expected = [
    `const { x, y: a, ...zs = {} } = {};`,
    `             [a]                    `,
    `          [y: a]                   `,
    `     [{ x, y: a, ...zs = {} }]     `,
];

Alternatively, this might be an appropriate place to use baselines that just render a human-readable format of what happened. Writing a one-off renderer to make an inspectable text file for what the selection spans were would be pretty easy, and lends itself to reviewers being able to see what the behavior is.

[andrewbranch]

👍🏽 I like the idea of a baseline. The assertions are difficult to write no matter what, but there are formats that are easy to read—eliminating the writing step seems like the best approach.

[andrewbranch]

Ryan just gave me a physical thumbs up on this in the room because his computer currently has a blue screen of death 😄

[sheetalkamat]

@andrewbranch yey on first big feature merge 👍

[mihailik]

External observer question: why is selectionRange-full is needed?

   export interface TextSpan {
      start: Location;
      end: Location;
  }

. . .

Also exposes a selectionRange-full tsserver command which accepts the same request format but responds with the language service response unchanged, which is the same except that TextSpan is a ts.TextSpan:

 export interface TextSpan {
     start: number;
     length: number;
 }

Given general brevity of the docs on ts APIs, adding odd-shaped nearly-identical calls can easily lead people astray.

[andrewbranch]

Good question. Almost every TSServer command has a -full counterpart that gets used by Visual Studio. I believe Visual Studio internally uses 0-based lines and columns, while I know VS Code uses 1-based lines and columns.

I actually wrote up something about this for the wiki, I just haven’t added it yet.

[mihailik]

Sorry, I didn't realise it's already consistent with other calls. Makes total sense!

[amcasey]

I believe the primary difference between the -full and regular versions is that -full uses a character offset from the beginning of the file, rather than line and column.

[andrewbranch]

Yep, that’s right, and that’s the same way that the compiler itself typically represents positions in files. Maybe Visual Studio doesn't use lines and columns much internally at all ¯_(ツ)_/¯