Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Nested symbols hierarchy #121

Closed
laughedelic opened this issue Nov 14, 2017 · 9 comments
Closed

Nested symbols hierarchy #121

laughedelic opened this issue Nov 14, 2017 · 9 comments

Comments

@laughedelic
Copy link

laughedelic commented Nov 14, 2017

Description

Outline view symbols hierarchy doesn't get nested based on the containerName property of the symbol, but requires that location ranges are nested.

Citing the LSP spec about location field of SymbolInformation object:

The location of this symbol. The location's range is used by a tool
to reveal the location in the editor
. If the symbol is selected in the
tool the range's start information is used to position the cursor. So
the range usually spwans more then the actual symbol's name and does
normally include thinks like visibility modifiers.

The range doesn't have to denote a node range in the sense of a abstract
syntax tree
. It can therefore not be used to re-construct a hierarchy of
the symbols.

(my emphasis)

The point is that the outline tree structure should be based on the "parent" relation: i.e. containerName matching the name of the parent symbol.

Current Behavior

Following example response contains a chain of nested symbols located each on a separate line:

  1. File
  2. Module
  3. Namespace
  4. Package
  5. Class
  6. Method
An example JSON body of the response I sent
[
  {
    "name": "File", "kind": 1,
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 1, "character": 1 },
        "end":   { "line": 1, "character": 2 }
      }
    }
  },
  {
    "name": "Module", "kind": 2,
    "containerName": "File",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 2, "character": 1 },
        "end":   { "line": 2, "character": 2 }
      }
    }
  },
  {
    "name": "Namespace", "kind": 3,
    "containerName": "Module",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 3, "character": 1 },
        "end":   { "line": 3, "character": 2 }
      }
    }
  },
  {
    "name": "Package", "kind": 4,
    "containerName": "Namespace",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 4, "character": 1 },
        "end":   { "line": 4, "character": 2 }
      }
    }
  },
  {
    "name": "Class", "kind": 5,
    "containerName": "Package",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 5, "character": 1 },
        "end":   { "line": 5, "character": 2 }
      }
    }
  },
  {
    "name": "Method", "kind": 6,
    "containerName": "Class",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 6, "character": 1 },
        "end":   { "line": 6, "character": 2 }
      }
    }
  }
]

Here's how it looks in the outline view:

screen shot 2017-11-14 at 01 13 08

Expected Behavior

screen shot 2017-11-14 at 01 25 37 small

This result was achieved with the following response body
[
  {
    "name": "File", "kind": 1,
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 1, "character": 1 },
        "end":   { "line": 35, "character": 1 }
      }
    }
  },
  {
    "name": "Module", "kind": 2,
    "containerName": "File",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 2, "character": 1 },
        "end":   { "line": 34, "character": 1 }
      }
    }
  },
  {
    "name": "Namespace", "kind": 3,
    "containerName": "Module",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 3, "character": 1 },
        "end":   { "line": 33, "character": 1 }
      }
    }
  },
  {
    "name": "Package", "kind": 4,
    "containerName": "Namespace",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 4, "character": 1 },
        "end":   { "line": 32, "character": 1 }
      }
    }
  },
  {
    "name": "Class", "kind": 5,
    "containerName": "Package",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 5, "character": 1 },
        "end":   { "line": 31, "character": 1 }
      }
    }
  },
  {
    "name": "Method", "kind": 6,
    "containerName": "Class",
    "location": {
      "uri": "...",
      "range": {
        "start": { "line": 6, "character": 1 },
        "end":   { "line": 30, "character": 1 }
      }
    }
  }
]

In this case ranges are nested.


Versions

  • Atom: 1.22.0
  • Client OS: OS X 10.11.6
  • atom-ide-ui: 0.5.4

Additional Details

Installed packages (`apm ls --installed`)
Dev Packages (1)� /Users/laughedelic/.atom/dev/packages
└── [email protected]

Community Packages (70)� /Users/laughedelic/.atom/packages
├── [email protected]
├── [email protected] (disabled)
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] (disabled)
├── [email protected]
├── [email protected]
├── [email protected] (disabled)
├── [email protected] (disabled)
├── [email protected] (disabled)
├── [email protected] (disabled)
├── [email protected]
├── [email protected] (disabled)
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] (disabled)
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] (disabled)
├── [email protected] (disabled)
├── [email protected] (disabled)
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] (disabled)
├── [email protected]
├── [email protected] (disabled)
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] (disabled)
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
└── [email protected]
@damieng
Copy link

damieng commented Nov 14, 2017

The problem with not taking the range into consideration is that names are not unique. Without it results are often ambiguous.

@laughedelic
Copy link
Author

But what about taking order into account? Symbols can be listed in the declaration order (sort by location start), then children always follow the parent, it's safe even if there will be another parent with the same name later.

Apart from that, I think LSP specification should be improved to make it easier: adding fullName field, that's unambiguous and can be used in containerName to refer to the parent symbol, whereas name will be used to display symbol in the UI. I think that for many language servers returning correctly nested ranges is much harder than getting fully qualified names.

@laughedelic
Copy link
Author

To clarify my first paragraph:

  1. sort received symbols by their definition order (i.e. location start-range)
  2. if a symbol has non-empty containerName, search this name among the symbols above it starting from the closest, if it matches any, nest it under it.

@laughedelic
Copy link
Author

OK, I see that it won't work well on this example:

class Foo {
  class Foo {
    val buh
  }
  val bar
}

Both buh and bar will be listed under the second Foo 🙁

@damieng
Copy link

damieng commented Nov 14, 2017

Yeah if you check the history for the OutlineViewAdapter you can see the various attempts at handling this up to the point we have today (which works well with C#, Java, TypeScript)

Presumably the problem you're seeing is with ide-python?

The spec doesn't explain why they don't want people producing a tree structure - it is incredibly useful and I suspect like the rest of the LSP spec it's entirely influenced by VSCode - which in this case did not have an outline/tree view at the time)

I really want to keep the existing behavior but if we can fall back to something else if some information is missing we can do that. Alternatively an ide package can implement their own OutlineViewAdapter although I suspect given the limited token information it just won't ever look good.

@hansonw
Copy link
Contributor

hansonw commented Nov 14, 2017

Check out microsoft/language-server-protocol#132 for some related discussions. Anyway this is firmly in atom-languageclient + LSP territory so I'm closing it here :)

@hansonw hansonw closed this as completed Nov 14, 2017
@laughedelic
Copy link
Author

Presumably the problem you're seeing is with ide-python?

Nope, I noticed it working on the Scala language server (scalameta/metals#34), but it's still in progress, so I think will work it out with nested ranges.

And yes, in the linked issue there is a direct reply in microsoft/language-server-protocol#132 (comment):

spec the purpose of the request which is to get a flat list of document symbols. Neither the container name nor the positions should be used to re-infer a hierarchy. The container name's purpose was to render it in the UI as a qualification.

So it's a won't-fix from the LSP side, or at least not in this type of request.

Also some general comments from another thread:

Which makes sense..

@damieng
Copy link

damieng commented Nov 14, 2017

The reality is in fact that most LSP's are returning information that can be used to infer a hierarchy and it looks great when it does. It doesn't need a full AST to do so as we're only really rendering important details.

Let me know if you need any help getting the Scala ranges working.

@laughedelic
Copy link
Author

laughedelic commented Nov 15, 2017

@damieng I agree. There are some notions common for all languages, like fully-qualified names or explicit information about nesting. I believe that only simplicity based on generalization and abstraction is helpful and liberating while simplicity based on the target usage is only limiting.

And thanks for support 👍 I think I got it right: scalameta/metals#34 (comment) 😉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants