Rls: LSP: textDocument/documentSymbol lacks containerName

[chergert]

When providing the symbol results for textDocument/documentSymbol, there is an optional field named containerName which provides the name of the parent symbol. It would be nice if rls supported this so that we can build a proper symbol tree in GNOME Builder.

[nrc]

The relevant part of the spec: https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#document-symbols-request

AIUI, we have this info coming out of the compiler, but we don't record it in rls-analysis and so it is non-trivial (but not exactly difficult) to surface from the RLS.

Historicaly, this has also been a bit of a tricky thing to get right - the compiler has multiple different ideas of what the container symbol is (e.g., whether anonymous blocks get counted, whether an impl counts, or should instead use the concrete type or trait, etc.), so I'm not 100% convinced a single one-size-fits-all container symbol will work (especially as we can only provide a name, not an id, so we can't use impls here).

[bruno-medeiros]

Why is the compiler and rls-analysis info necessary for this? The https://github.com/RustDT/Rainicorn tool provides this functionality using only the Rust parser. The protocol format mapping is easier here, because it provides the symbols (aka structure elements) in a hierarchical model already, thus the container info is already included implicitly. However, the problem of mapping hierarchical model to flat one is a different problem. In any case, it seems only the rust parser would be needed.

[bruno-medeiros]

BTW, I'm not sure we can create a tree model reliably in a client using containerName. The thing is, from what I understand from VS Code, the containerName is used by the "Go to Symbol in File" functionality, which uses containerName just for _display purposes_ (https://code.visualstudio.com/images/cpp_workspacesearch.png). This means it doesn't have to worry about duplicates, etc.

But to use containerName to build a tree model, we would have to ensure those names are unique, otherwise you run into problems with code like:

struct Foo {
}
impl Foo {
  fn func() { }
}

if the func symbol is sent with containerName=Foo, which container is that?

I guess we could specify containerName in a modified way that actually is unique (for example assigning id's like 1:Foo for struct Foo , 2:Foo for impl Foo, etc, but this would be an LSP extension that clients would have to know how to handle. Point being, to implement this reliably, it seems we need need an LSP extension.

[nrc]

Why is the compiler and rls-analysis info necessary for this?

Depends how you define it, if you only ever want the syntactic container, then you don't, if you want the semantic container, e.g., Foo containing func in your example above, then you do.

[chergert]

It would be nice to have a cross-unit id extension to the symbol JSON node for more complex use cases. Then allow the tooling to choose how precise they want to be with the tree.

[bruno-medeiros]

Depends how you define it, if you only ever want the syntactic container, then you don't, if you want the semantic container, e.g., Foo containing func in your example above, then you do.

I think the way this is typically used, is with syntactic container only.

[bruno-medeiros]

Actually, (as was pointed out here: https://github.com/Microsoft/language-server-protocol/issues/112#issuecomment-262169335 ), the source range information (if correct) should be enough to build a tree of the symbols, so container name is not actually needed.

[chergert]

I must be missing something, because doesn't that still require a language parser client side to know the boundary of scopes (which would defeat the purpose of IDE side langserv provider being language agnostic). For languages where classes can be embedded inside of classes, or multiple classes per file, how do you know if the class was nested or simply after the previous?

Edit: (I guess that is what you meant by reliable range information, so you'd have an end position). Nevermind, feel free to disregard this :)

[chergert]

Okay, so I implemented support for building the tree using the provided range in GNOME Builder https://github.com/chergert/gnome-builder/commit/7d6ae63bd31f5aa3e5fe7d7b59c0b100d7099357 but it looks like that rustls does not provide source ranges that map to the entire semantic range of the AST node but rather the keyword.

So once rustls adjusts the range to be accurate, symbol trees should just start working for us.

[bruno-medeiros]

So once rustls adjusts the range to be accurate, symbol trees should just start working for us.

That's a good point, I hadn't checked what kind of range rls returned. However, it might might that the intention of LSP is that the range of the symbol is meant to be only the range of the identifier. I've asked for a clarification: https://github.com/Microsoft/language-server-protocol/issues/132

[alexheretic]

Atom users would appreciate this too, see https://github.com/mehcode/atom-ide-rust/issues/74.

Rls providing a containerName for symbols is perfectly valid in the spec, so I can't see anything blocking it's implementation _(other than someone to do it)_. The client's use/misuse for creating a tree is up to the client, but in any case that ship has sailed and no alternative for clients seems to exist yet.

[laughedelic]

Rls providing a containerName for symbols is perfectly valid in the spec, so I can't see anything blocking it's implementation (other than someone to do it). The client's use/misuse for creating a tree is up to the client, but in any case that ship has sailed and no alternative for clients seems to exist yet.

@alexheretic containerName is not needed for Atom to show the tree view.

See discussion in https://github.com/facebook-atom/atom-ide-ui/issues/121 and https://github.com/facebook-atom/atom-ide-ui/issues/121#issuecomment-344145808 in particular for the summary.

TL;DR: LSP doesn't allow you to rely on containerName for the hierarchy. But you can use nested location ranges to nest the tree nodes. It works in Atom IDE UI and it works with the VS Code outline extension.

[alexheretic]

Right thanks @laughedelic for redrawing my attention, to:

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.

So iiuc you suggest we ignore containerName and instead extend symbol ranges to cover their scopes?

[alexheretic]

Ok with a little investigation we'll need both containerName _and_ nested location ranges.

Here I manually extended fun1's location range to cover its scope, and added it as fun2's containerName.

Another issue is impl Temp {} is not currently a symbol. We need it as a symbol in order to nest things under it.

[nrc]

impls don't have very good info at the moment, although impl Temp in that example should have OK info.

We need something like this to improve the symbols view in cargo src too.

[alexheretic]

For a bit more completeness here is a more complex example

textDocument/documentSymbol response

[{
  "name": "",
  "kind": 2,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 1,
        "character": 0
      },
      "end": {
        "line": 58,
        "character": 1
      }
    }
  },
  "containerName": null
}, {
  "name": "stuff",
  "kind": 2,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 7,
        "character": 4
      },
      "end": {
        "line": 7,
        "character": 9
      }
    }
  },
  "containerName": ""
}, {
  "name": "speaks",
  "kind": 2,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 8,
        "character": 8
      },
      "end": {
        "line": 8,
        "character": 14
      }
    }
  },
  "containerName": "stuff"
}, {
  "name": "Speaks",
  "kind": 11,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 9,
        "character": 18
      },
      "end": {
        "line": 9,
        "character": 24
      }
    }
  },
  "containerName": "speaks"
}, {
  "name": "speak",
  "kind": 6,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 10,
        "character": 15
      },
      "end": {
        "line": 10,
        "character": 20
      }
    }
  },
  "containerName": "Speaks"
}, {
  "name": "NUM_A",
  "kind": 14,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 14,
        "character": 10
      },
      "end": {
        "line": 14,
        "character": 15
      }
    }
  },
  "containerName": "stuff"
}, {
  "name": "Temp",
  "kind": 5,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 18,
        "character": 11
      },
      "end": {
        "line": 18,
        "character": 15
      }
    }
  },
  "containerName": "stuff"
}, {
  "name": "field",
  "kind": 8,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 19,
        "character": 8
      },
      "end": {
        "line": 19,
        "character": 13
      }
    }
  },
  "containerName": "Temp"
}, {
  "name": "Another",
  "kind": 5,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 21,
        "character": 11
      },
      "end": {
        "line": 21,
        "character": 18
      }
    }
  },
  "containerName": "stuff"
}, {
  "name": "AnEnum",
  "kind": 10,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 23,
        "character": 20
      },
      "end": {
        "line": 23,
        "character": 26
      }
    }
  },
  "containerName": "stuff"
}, {
  "name": "One",
  "kind": 8,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 24,
        "character": 8
      },
      "end": {
        "line": 24,
        "character": 11
      }
    }
  },
  "containerName": "AnEnum"
}, {
  "name": "Two",
  "kind": 8,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 25,
        "character": 8
      },
      "end": {
        "line": 25,
        "character": 11
      }
    }
  },
  "containerName": "AnEnum"
}, {
  "name": "name",
  "kind": 8,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 25,
        "character": 14
      },
      "end": {
        "line": 25,
        "character": 18
      }
    }
  },
  "containerName": "Two"
}, {
  "name": "Three",
  "kind": 8,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 26,
        "character": 8
      },
      "end": {
        "line": 26,
        "character": 13
      }
    }
  },
  "containerName": "AnEnum"
}, {
  "name": "something",
  "kind": 6,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 29,
        "character": 11
      },
      "end": {
        "line": 29,
        "character": 20
      }
    }
  },
  "containerName": null
}, {
  "name": "speak",
  "kind": 6,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 34,
        "character": 11
      },
      "end": {
        "line": 34,
        "character": 16
      }
    }
  },
  "containerName": "Speaks"
}, {
  "name": "self",
  "kind": 13,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 34,
        "character": 18
      },
      "end": {
        "line": 34,
        "character": 22
      }
    }
  },
  "containerName": null
}, {
  "name": "speak",
  "kind": 6,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 39,
        "character": 11
      },
      "end": {
        "line": 39,
        "character": 16
      }
    }
  },
  "containerName": "Speaks"
}, {
  "name": "self",
  "kind": 13,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 39,
        "character": 18
      },
      "end": {
        "line": 39,
        "character": 22
      }
    }
  },
  "containerName": null
}, {
  "name": "fun1",
  "kind": 12,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 43,
        "character": 7
      },
      "end": {
        "line": 43,
        "character": 11
      }
    }
  },
  "containerName": "stuff"
}, {
  "name": "fun2",
  "kind": 12,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 44,
        "character": 11
      },
      "end": {
        "line": 44,
        "character": 15
      }
    }
  },
  "containerName": null
}, {
  "name": "m",
  "kind": 13,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 50,
        "character": 16
      },
      "end": {
        "line": 50,
        "character": 17
      }
    }
  },
  "containerName": null
}, {
  "name": "main",
  "kind": 12,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 56,
        "character": 3
      },
      "end": {
        "line": 56,
        "character": 7
      }
    }
  },
  "containerName": ""
}, {
  "name": "a",
  "kind": 13,
  "location": {
    "uri": "file:///home/alex/project/rusttmp/src/main.rs",
    "range": {
      "start": {
        "line": 57,
        "character": 8
      },
      "end": {
        "line": 57,
        "character": 9
      }
    }
  },
  "containerName": null
}]

So we can see that we _do_ already set containerName in a few cases. However, without nested location ranges atom can't construct the proper syntax tree outline & can't connect the containerName with the actual symbol name, so both appear separately.

So it looks like we should make the following improvements, that may need enhancements to rls-analysis:

• [ ] Construct test case using above example.
• [ ] Extend all location end positions to the end of the owning scope _if applicable_.
  _So for example the mod stuff location should change from [7:4, 7:9] -> [7:4, 56:1]_
• [ ] Add missing symbols, _ie impl Temp_ _(probably just use Unknown to describe this symbol kind)_
• [ ] Add missing containerNames, _ie fn fun1() { fn fun2() {} }_
• [ ] Once we have the symbol, rename impl Speaks for Temp _"Speaks"_ -> _"Speaks for Temp"_ (remove generics if they exist though?)
• [ ] Make use of newer LSP symbol kinds _struct, etc_
• [ ] Minor: Remove empty string name symbols & containerNames

Later enhancements:

• [ ] Outlines for common macro declarations _lazy_static!, thread_local!, etc_

Test code

#![allow(unused)]
#[macro_use]
extern crate lazy_static;

use std::collections::HashMap;
use std::sync::Arc;

mod stuff {
    mod speaks {
        pub trait Speaks {
            fn speak(&self);
        }
    }
    use self::speaks::Speaks;
    const NUM_A: usize = 234;

    struct Temp {
        field: i32,
    }
    struct Another;

    pub(crate) enum AnEnum {
        One(u32, u32),
        Two { name: &'static str },
        Three,
    }
    impl Temp {
        fn something() -> i32 {
            1
        }
    }
    impl Speaks for Temp {
        fn speak(&self) {
            eprintln!("hello");
        }
    }
    impl Speaks for AnEnum {
        fn speak(&self) {
            eprintln!("hmmmm");
        }
    }
    fn fun1() {
        fn fun2() {}
    }
}

lazy_static! {
    pub static ref MAP: HashMap<u32, &'static str> = {
        let mut m = HashMap::new();
        m.insert(345, "345");
        m
    };
}

fn main() {
    let a = Arc::new(123);
}

[Xanewok]

Closing this in favor of #909 since it's more recent and general in terms of approach.