Stream: t-compiler/wg-rls-2.0

Topic: hover


std::Veetaha (Mar 15 2020 at 22:56, on Zulip):

Isn't this exact flag no longer useful in HoverResult? There is even no way to set it to false by using the public API and it is never set to false withing the module it is defined in.

matklad (Mar 16 2020 at 09:12, on Zulip):

yeah, we should remove it

std::Veetaha (Mar 16 2020 at 09:18, on Zulip):

There is a PR

std::Veetaha (Mar 21 2020 at 23:42, on Zulip):

@matklad , @Florian Diebold am I right to suppose that the Ty struct declared in ra_hir_ty should not be visible in ra_ide and instead only interacted through ra_hir::Type?

matklad (Mar 22 2020 at 08:18, on Zulip):

In theory, yes.

matklad (Mar 22 2020 at 08:18, on Zulip):

It's not completely clear if this will work 100%

std::Veetaha (Mar 22 2020 at 08:34, on Zulip):

I asked because iirc the are some methods on Type that return structs from ra_hir_ty, but ra_ide doesn't list it as it's dependency, so we can't e.g. match the returned Ty enum

matklad (Mar 22 2020 at 08:38, on Zulip):

they should be changed to use Type

std::Veetaha (Mar 22 2020 at 13:16, on Zulip):

@matklad , how would you change this method? https://github.com/rust-analyzer/rust-analyzer/blob/6fe956420fc63609a84dd005b8190b1f19fff280/crates/ra_hir/src/code_model.rs#L1093-L1096

std::Veetaha (Mar 22 2020 at 13:17, on Zulip):

I mean, what should it return instead of ra_hir_ty::CallableDef?

matklad (Mar 22 2020 at 13:18, on Zulip):

I would remove it completely, add fn fn_params() -> Vec<(Option<Name>, Type)>; fn fn_ret_type() -> Type accessors to Type and would fix callers to use those instead of CallableDef

matklad (Mar 22 2020 at 13:18, on Zulip):

There isn't a reasonable thing we can return there, because there's no defs for things like closures

std::Veetaha (Mar 22 2020 at 13:20, on Zulip):

And shouln't we reexport ra_hir_ty stuff from ra_hir here?

matklad (Mar 22 2020 at 13:34, on Zulip):

HirDisplay might actually be ok

std::Veetaha (Mar 22 2020 at 17:00, on Zulip):

I wish we had some docs on ast node types, at least an example source code that shows to which kind of symbols the node referes to. E.g. the difference between Name and NameRef is not very evident, does ParenType pertain to unnamed tuples or tuple structs too and all these kinds of questions arise... I suppose the ast api is quite stable already so we might want to better document it, @matklad ?

matklad (Mar 22 2020 at 17:08, on Zulip):

SGTM! This probably should be added to ast_src

Laurențiu Nicola (Mar 22 2020 at 17:15, on Zulip):

Maybe we could have some kind of doc tests which show example Rust code and what it parses to?

Laurențiu Nicola (Mar 22 2020 at 17:15, on Zulip):

We might already have something like that

std::Veetaha (Mar 22 2020 at 17:20, on Zulip):

Yeah, that would be wonderful

std::Veetaha (Mar 22 2020 at 20:35, on Zulip):

@matklad , would you mind elaborating on the relationship between ra_hir and ra_hir_def? I see the data structures in ra_hir::code_model are almost literally copy-pasted for ra_hir_def:
image.png

matklad (Mar 23 2020 at 09:09, on Zulip):

hir is the public API of the all of the compiler logic above syntax trees. It is written in "OO" style. Each type is self contained (as in, it knows it's parents and full context). It should be "clean code".

ra_hir_xxx crates are the implementation of the compiler logic. They are written in "ECS" style, with relatively little abstractions. Many types are not self-contained, and explicitelly use local indexes, arenas, etc.

ra_hir is what insulates the "we don't know how to actually write an incremental compiler" from the ide with completions, hovers, etc. It is a (soft, internal) boundary: https://www.tedinski.com/2018/02/06/system-boundaries.html.

std::Veetaha (Mar 24 2020 at 00:05, on Zulip):

@matklad , could you elaborate on why do we store the signature of the closure as the first parameter type. It appears that we store there the function where this closure was created instead (even nested closure refers to the outer function that declares the closure tree)

std::Veetaha (Mar 24 2020 at 00:13, on Zulip):

And am I right, that we don't have hir representation of closures? E.g. the existing ra_hir::Function appears to be a wrapper around ra_hir_def::FunctionId, but closures don't have own ra_hir_def::FunctionId (as they contain only the fn id of the plain function they are declared in as I mentioned above)...

std::Veetaha (Mar 24 2020 at 01:13, on Zulip):

My gut feeling is that closure should desugar to a callable struct. Maybe we should introcude:

struct ra_hir::Closure {
    impl_id: ra_hir_def::ImplId
   // closed state id maybe  ???
}
Florian Diebold (Mar 24 2020 at 08:26, on Zulip):

what are you actually trying to do?

std::Veetaha (Mar 24 2020 at 08:34, on Zulip):

I was trying to replace ra_hir::Type::as_callable() -> Option<ra_hir_ty::CallableDef> with ra_hir-only types. There is a comment on top of that method that it doesn't work for closures, so I wondered how we should support closures otherwise (e.g. currently we don't show neither call hierarchy, nor call info, nor inlay Param hints for closures)

Florian Diebold (Mar 24 2020 at 11:42, on Zulip):

you could certainly create a Closure type in code_model that just points to the containing def and ExprId of the closure

std::Veetaha (Mar 24 2020 at 23:35, on Zulip):

Hmm, I am thinking about how this will scale since there should be callable structs (i.e. functors), which are the ADTs that implement Fn* traits. But I have a feeling we should bother and take closures and callable adts apart

std::Veetaha (Mar 25 2020 at 11:03, on Zulip):

@matklad I have a strong feeling that the ra_hir::Type should be an enum, but higher level than ra_hir_ty::Ty and hide it in its impl. We already have ra_hir::Function/Struct/Enum... etc. that should be the members of Type enum. What do you think?

std::Veetaha (Mar 25 2020 at 11:07, on Zulip):

E.g. you proposed to add fn_params and fn_ret_type, but both of them should return an option, because now the Type is a general thing and we will and up with a bunch of unwrap() if we want to look at it as a function, by the way these method names are a bit misleading, because closures are not strictly-speaking functions

Florian Diebold (Mar 25 2020 at 11:09, on Zulip):

types are not just defs. you'd have to mostly replicate the structure of hir_ty::Ty, and I don't think that would be very useful in the end

matklad (Mar 25 2020 at 11:09, on Zulip):

I am not really sure.... There are a lot of intricate details about types. Like, hir::Struct can be either the ctor function, or a struct. Threading substitutions is tricky.

My current hypothesis is that the IDE would work best with an unitype representation of type, with a methods for specific operations.

Ie, for completion, I think it would be better of completion asks "what fields are available on this type", rather than "match this type with a record, and iterate its fields".

matklad (Mar 25 2020 at 11:10, on Zulip):

I can also imagine a type which you both can call, and which has fields, if we allow for custom Fn impls

std::Veetaha (Mar 25 2020 at 11:11, on Zulip):

So this means that we should move more logic from IDE into the Type itself

matklad (Mar 25 2020 at 11:11, on Zulip):

The general design guideline is probably to try to put high-level methods directly onto the Type, like Type::signature

matklad (Mar 25 2020 at 11:11, on Zulip):

So this means that we should move more logic from IDE into the Type itself

Yup

Florian Diebold (Mar 25 2020 at 11:12, on Zulip):

For signature help, you probably want a function that returns the signature for the type. For things like closures, that will even involve trait solving, unless we save the signature during inference

Florian Diebold (Mar 25 2020 at 11:13, on Zulip):

part of the problem there is that currently I think signature help displays the function definition as written in the AST, and that's not going to work for things like closures

std::Veetaha (Mar 25 2020 at 11:13, on Zulip):

Right, this is what bothers me in the first place. The FunctionSignature in ra_ide works with Function, Struct and EnumVariant, but not Type

std::Veetaha (Mar 25 2020 at 11:13, on Zulip):

Thus I tried to introduce Closure, but failed...

matklad (Mar 25 2020 at 11:16, on Zulip):

Yeah, that's why Type::as_callable is broken . Rather than trying to downcast the type to a specific def, we should have a function whicih returns a type's signature as a funciton

matklad (Mar 25 2020 at 11:18, on Zulip):
struct Signature {
    params: Vec<(Option<Name>, Type)>
    ret_ty: Type,
    kind: FnOnce | FnMut | Fn

    // We need a way to extract doc comment, if there is one, but I don't know what's the right API

    def: Option<Def>
}

impl Type {
  fn callable_signautre(&self) -> Option<Signature>
}
std::Veetaha (Mar 25 2020 at 11:20, on Zulip):

Also, I wondered, why don't we use Vec<(Option<Pat>, Type)> for params?

matklad (Mar 25 2020 at 11:21, on Zulip):

While I am looking at Type, seems like it might make sense to try to kill Type::autoderef in favor of parameters on methods like fields and iterate methods

matklad (Mar 25 2020 at 11:21, on Zulip):

Hm, riiiight, I think that should even be a Local and not Pat

matklad (Mar 25 2020 at 11:24, on Zulip):

The more general philosophical guideline here is that, for impl, you usually want a fully-transparent pure-data implementation.

When using an API, it is however more convenient to work with opaque types without any visible internal structure.

Florian Diebold (Mar 25 2020 at 11:28, on Zulip):

matklad said:

Hm, riiiight, I think that should even be a Local and not Pat

hmm but what do we do if there's a parameter like (foo, bar): (u32, i32)?

Florian Diebold (Mar 25 2020 at 11:28, on Zulip):

like, we don't actually care about the locals of the function, we care about the parameters, and it's not really a 1:1 mapping

matklad (Mar 25 2020 at 11:30, on Zulip):

Right.... Yeah, I guess that needs to be a PatId in the end, plus some public accessor which return whatever an IDE needs

matklad (Mar 25 2020 at 11:32, on Zulip):

Hm, I guess that means that, in the limit, we should treat patterns as defs in the code-model API.....

Like, there's little difference between struct Foo { foo: u32, bar: u32} and (foo, bar): (u32, u32)

Florian Diebold (Mar 25 2020 at 11:43, on Zulip):

for something like signature help, we'd need a) either a string representation of the whole thing or an AST pointer, and b) the type, right?

matklad (Mar 25 2020 at 11:47, on Zulip):

Not sure about strings and ast --- there are macros in patterns: pari!(a, b): (u32, u32). We might want to render that as (a, b) and not as pair!(a, b)

Florian Diebold (Mar 25 2020 at 11:48, on Zulip):

hmm but will the macro-expanded pattern always be more clear about what the parameter means? in the end, this is just a hint about the parameter, right, and I'm not convinced that the macro-expanded form is a better hint :thinking:

Florian Diebold (Mar 25 2020 at 11:49, on Zulip):

but in the end, we won't always have a non-expanded form..

std::Veetaha (Mar 25 2020 at 11:53, on Zulip):

This is actually heuristic. For some macros, some users would like to see their unexpanded form in the signature help...

Florian Diebold (Mar 25 2020 at 11:57, on Zulip):

I think I'd even argue that the patterns in the function signature are mostly for documentation purposes, otherwise you can always move them into the body. That doesn't really hold for closures though

Florian Diebold (Mar 25 2020 at 11:57, on Zulip):

OTOH, maybe we should just do whatever rustdoc does there

std::Veetaha (Mar 25 2020 at 22:47, on Zulip):

FYI: rustdoc expands macros and shows the patterns in desugared form
e.g. this

pub struct A { a: u32, b: bool, }

macro_rules! pair {
    ($a: tt, $b: tt) => { A { $a, $b } };
}

/// My docs!
pub fn function(pair!(b, a): A) {

}

results in

pub fn function(A { b: b, a: a }: A)
std::Veetaha (Mar 25 2020 at 23:18, on Zulip):

Hmm, what does that ?0 mean, @Florian Diebold ?
image.png This comes from here

Florian Diebold (Mar 26 2020 at 08:09, on Zulip):

hm, it's a variable, that shouldn't be there

std::Veetaha (Mar 27 2020 at 01:00, on Zulip):

I struggle to get parameters Vec<PatId> from FunctionId. It seems like this should be stored in FunctionData or how else can I get it? Besides that FunctionData is quite sparse, there is no info about function's abi, unsafety, asynchrony, and maybe more things which I haven't tried to collect yet (maybe there are some other queries for them)

matklad (Mar 27 2020 at 11:26, on Zulip):

Patterns matter only for function internals (unlike parameter types), so they belong to the body

std::Veetaha (Mar 28 2020 at 02:00, on Zulip):

Okay, I managed to get the ra_hir_def::Pats, for function parameters, but since they contain Idx<Pat> types they make no sense without the Arena<Pat>. So is it ok exposing them to ra_ide, or otherwise how better to encapsulate them in ra_hir? .
I would be thinking about smth like:

struct ra_hir::Pat {
    inner: ra_hir_def::expr::Pat;
    body: Arc<ra_hir_def::Body>; // this is kinda bad, because we only need body.pats from it, but otherwise we would need to copy the arena
}

Besides that, the primary goal of getting them was for creating a signature as referenced here by @matklad and to use it in signature help afterwards. But using it this way means displaying the ra_hir_def::Pat and this requires implementing std::fmt::Display for a number of types in ra_hir_def. Whilst for now the signature help uses ast api to display the function parameters patterns and types.

So I want to clarify whether it is okay to move from ast impl to using the ra_hir api (and somehow solve how to expose Pats) in this case even speaking about the long term design? (My guess is that is what we want, since using the ast api is akin manipulating the raw text but slightly more high-level).

Last update: Oct 28 2020 at 18:00UTC