Only report isDefinition when FAR is triggered on a definition

[amcasey]

In practice, isDefinition is used to exclude occurrences from the CodeLens references count in VS Code. Rather than try to generalize the meaning to everywhere FAR can be invoked, we stop reporting isDefinition for any search not triggered from a declaration location.

As a bonus, we get to skip a bunch of work.

Caveats:

1. No changes were made to the logic aggregating across projects. I claim that (a) this doesn't make it worse and (b) that scenario is encountered sufficiently rarely that it can be addressed in a follow-up PR (already drafted).
2. Technically, isDefinition is also used when there are exactly two references to a symbol to check whether exactly one is the definition (and jump to the other). After this change, no reference will be marked as a definition if the search is triggered from a non-declaration and, consequently, a list of references will be displayed (vs the current behavior of jumping to the beginning of the current token). Given that the editor behavior was specifically not coded as "just go to the other one", I claim that this asymmetry is actually more in line with the editor's intentions.

Given that we didn't hear an outcry last time we completely changed the (undocumented) meaning of isDefinition, I don't believe other editors depend on the exact behavior.

Edit: because an absent property won't "beat" a later, present property, the multi-project logic does need a small tweak.
Edit 2: "last time we completely changed" #45920

[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

[amcasey]

The new isDefinition_ tests are probably interesting. The modified tests are probably not.

The baseline changes are in their own commit if you want to review around them.

The protocol change probably warrants some discussion, since this PR makes a non-optional property optional. I believe that's okay since only a very rigidly coded client will have trouble treating an absent property as falsy.

[sheetalkamat]

can you put some screen shots of how the search use to look and how it looks now in editor for better clarity.

[sheetalkamat]

can you put some screen shots of how the search use to look and how it looks now in editor for better clarity.

[DanielRosenwasser]

Here's the current behavior that everyone gets

Here's what this PR proposes

[andrewbranch]

@DanielRosenwasser huh? Your two gifs are running two different commands 😄

[DanielRosenwasser]

Not only are they running different commands, the second one was clearly not exhibiting any of the behaviors @amcasey described. I've posted a new GIF.

[andrewbranch]

I don’t have a strong opinion on the way isDefinition impacts the go-to-the-other-one vs. peek behavior Daniel showed, but it does seem like an alternative would be for it to always jump to the non-requesting location when there are exactly two results. This is similar to what happens when you run Go To Definition on something with exactly one definition and one non-definition reference: when Go To Def returns only the same location as you requested, it runs Find All Refs instead (and if there’s only one reference, it jumps there). Consequently, you can jump back and forth between the singular definition and singular reference using Go To Definition alone.

[amcasey]

I don’t have a strong opinion on the way isDefinition impacts the go-to-the-other-one vs. peek behavior Daniel showed, but it does seem like an alternative would be for it to always jump to the non-requesting location when there are exactly two results. This is similar to what happens when you run Go To Definition on something with exactly one definition and one non-definition reference: when Go To Def returns only the same location as you requested, it runs Find All Refs instead (and if there’s only one reference, it jumps there). Consequently, you can jump back and forth between the singular definition and singular reference using Go To Definition alone.

I actually planned to make a PR to change this in VS Code, but discovered that it's actually in the core and applies to all languages. As I mentioned above, the code actually does extra work to not just jump to the other location, so I assumed it was intentional. We could file a bug, but I suspect there would be muscle memory issues.

[DanielRosenwasser]

To me there's really 3 things to discuss:

1. How does the behavior of an editor differ based on isDefinition today? Seems like apart from display, there's a difference in the N = 2 case for VS Code. That seems like something that could be in place regardless of isDefinition.
2. How does this break consumers of the API? Will we break any pedantic deserializers?
3. How intuitive is the behavior of this property for an editor? A reference ceases to be a definition as soon as you ask for a reference on the definition? Kind of odd.

[amcasey]

Sorry, I've been offline. Thanks for sharing those screenshots, @DanielRosenwasser!

Seems like apart from display

What did you mean by that? I wasn't expecting the CodeLens experience to change at all.

[amcasey]

Oh, I think @DanielRosenwasser might just have been asking where isDefinition is consumed. I wrote that up here: #47938 (comment)

Sorry for not bringing it forward.

[amcasey]

Looks like isDefinition became non-optional in 2.0: 792b23e

[amcasey]

3. A reference ceases to be a definition as soon as you ask for a reference on the definition? Kind of odd.

Possibly I'm misunderstanding, but I believe it's the other way around - if you ask on the definition, you get a value for every reference (including the definition); if you ask on a reference, you get nothing.

[DanielRosenwasser]

Not quite sure if there's an issue with the delete but otherwise this seems reasonable since the property used to be optional anyway. Maybe we can reach out to tide and typescript-language-server to give them a heads up since I see they leverage isDefinition.

[DanielRosenwasser]

CC @ananthakumaran, @rchl

[ananthakumaran]

I haven't looked at the changes in detail, but it might potentially break the feature introduced by ananthakumaran/tide#56

[amcasey]

@ananthakumaran In the example in ananthakumaran/tide#56, the search from (1) will still include isDefinition, but the search from (2) will not. This should be enough information to jump to the only reference. If your preferred approach is to simply jump to the "other" usage, then, as @andrewbranch suggested above, you shouldn't need to consume isDefinition at all.

Also note that the behavior before this PR already had a lot of weird corner cases. For example, if you exported a function in one file and imported it in another, there would be exactly two usages, but each would return isDefinition true at the point where the search began and false at the other usage.

[rchl]

Thanks for the heads-up Daniel.

One place in typescript-language-server that refers to isDefinition is obviously the textDocument/references handler. The other place is the custom "calls" request that I don't really care much about as it's not that well defined what it should do.

Neither of those places will have issues with the isDefinition becoming optional.

As far as the behavior change, the discussion here is a bit confusing because it explains in terms of VSCode behavior which then makes it unclear whether change is due to VSCode's logic itself or the protocol change. I wanted to know specifically how this change affects the results returned by the typescript server so checked with similar example as posted before:

/*1*/function foo() {}
/*2*/foo();

textDocument/references on line (1) now returns:

[
  {
    "file": "/usr/local/workspace/github/typescript-language-server/src/test.ts",
    "start": {
      "line": 1,
      "offset": 15
    },
    "end": {
      "line": 1,
      "offset": 18
    },
    "contextStart": {
      "line": 1,
      "offset": 6
    },
    "contextEnd": {
      "line": 1,
      "offset": 23
    },
    "lineText": "/*1*/function foo() {}",
    "isWriteAccess": true,
    "isDefinition": true
  },
  {
    "file": "/usr/local/workspace/github/typescript-language-server/src/test.ts",
    "start": {
      "line": 2,
      "offset": 6
    },
    "end": {
      "line": 2,
      "offset": 9
    },
    "lineText": "/*2*/foo();",
    "isWriteAccess": false,
    "isDefinition": false
  }
]

While on line (2) returns:

[
  {
    "file": "/usr/local/workspace/github/typescript-language-server/src/test.ts",
    "start": {
      "line": 1,
      "offset": 15
    },
    "end": {
      "line": 1,
      "offset": 18
    },
    "contextStart": {
      "line": 1,
      "offset": 6
    },
    "contextEnd": {
      "line": 1,
      "offset": 23
    },
    "lineText": "/*1*/function foo() {}",
    "isWriteAccess": true
  },
  {
    "file": "/usr/local/workspace/github/typescript-language-server/src/test.ts",
    "start": {
      "line": 2,
      "offset": 6
    },
    "end": {
      "line": 2,
      "offset": 9
    },
    "lineText": "/*2*/foo();",
    "isWriteAccess": false
  }
]

Since by default (in Sublime Text at least) the textDocument/references request is triggered with includeDeclaration: false, the first case will return only one (non-definition) item while the second case will return both.

So in fact the same behavior as shown in the VSCode gifs earlier.

If that's the intended new behavior then I'm fine with it too.

[amcasey]

@rchl Sorry for not being clearer. Yes, the expected change in the server response is that the invocation at (2) will no longer include isDefinition. The behavior shown in the gifs was not the goal of the change, but it is an expected consequence of the change and seems intuitively reasonable to us.

In a perfect world, we'd probably have a separate isDeclaration property for implementing textDocument/references and, ironically, it would probably have the value that isDefinition had before #45920 (which, I believe, was changed on the basis that it was ill-defined and had few consumers).

[ananthakumaran]

the search from (1) will still include isDefinition, but the search from (2) will not

I just tested it against master and the feature still works as earlier. references command from (2) showed all references earlier as well, the user would use goto definition if they want to jump to definition. LGTM