Adding all the DefKinds #121 #177

projektir · 2017-09-27T01:03:05Z

Addresses #121.

This excludes Local, which means it needs to be deliberately skipped. Maybe it should be more specific like DefKind::Local => continue in case something else is added in the future?

I'm not sure if it makes sense for Static or Const and such to have children?

I believe the idea is to set this boilerplate up and then we can move on to actually implementing things in the frontend?

steveklabnik · 2017-09-27T13:27:05Z

Maybe it should be more specific like DefKind::Local => continue in case something else is added in the future?

Yeah, seems good.

I'm not sure if it makes sense for Static or Const and such to have children?

Yeah, this is true. They shouldn't have children, they probably will be children, though.

I believe the idea is to set this boilerplate up and then we can move on to actually implementing things in the frontend?

💯

Could you add tests please?

mgattozzi · 2017-09-27T14:00:12Z

For the tests you would want to put something under this directory here. I think something like def_kind.rs would be a good thing and then just create every type of DefKind in the source code there and set up some assertions. Or seperately (and probably a better idea) is to set up one type of file for each DefKind we don't have yet and make the assertions in there. You can look at the other files for examples of how to assert something is of the output JSON that rustdoc emits.

@projektir does that make sense? Anything I can clear up with my explanation?

projektir · 2017-09-27T21:10:36Z

@mgattozzi
OK, I've tried some of this but I'm hitting a lot of issues (see comments)... this commit will break since otherwise I have to delete the whole file for macros.

projektir · 2017-09-27T21:11:30Z

tests/source/macros.rs

@@ -0,0 +1,9 @@
+#![crate_type = "lib"]
+
+// @has /data/relationships/macros/data/0 'macro'


I can't get this to pick up at all. It doesn't seem to see macros. I have the same issue with unions and I imagine would have the same issue with statics and consts.

@steveklabnik what about this problem?

I'm seeing in #7 a comment"After a quick look at generated JSON, it looks like the exported macros (and their doc comments) are not included.". So that kind of seems like a problem. And I've also seen the same issue with unions.

I'm going to tweak the lib.rs file with all these things and see what it actually generates because it doesn't seem like a lot right now.

sounds like maybe the same thing as impls, I'm not sure.

projektir · 2017-09-27T21:12:26Z

tests/source/methods.rs

+pub struct UnitStruct;
+
+impl UnitStruct {
+    pub fn method_function() {}


Since to create a method it needs to be attached to some object, I need to identify it somehow, but nothing seems to work. Am I misunderstanding that this is not actually a child of a struct or such? Is there some intermediate type?

@nrc, I don't know anything about this, maybe you do?

Yeah, basically nothing with impls (and thus methods) is going to work very well yet. We need some work in rustc to add support and that is a bit non-trivial in the general case. We could add support for inherent impls pretty easily, if someone wanted to hack on that I could give instructions.

And actually, I thought inherent impls worked, but maybe I was just wrong about that.

In which case it might just be best for now if we put tests for what works, mark what doesn't (but still keep the code there), and have the tests written for whatever works. Thoughts @steveklabnik?

I'd remove the code for the stuff that doesn't work, and go with a _ =>. But yeah, otherwise, let's do that, @projektir . It might also be worth noting this over at #7

Thanks @nrc

projektir · 2017-09-27T23:24:51Z

Looking at it a bit more, it seems the data.json (which I am assuming is backend relevant) is not getting generated right. It only has modules, structs, and functions, and doesn't get methods nor other things if I modify the lib.rs. So it looks like my boilerplate code is just not getting the job done in the first place.

steveklabnik

Let's see what nick says about this

steveklabnik · 2017-09-28T13:09:08Z

tests/source/methods.rs

+pub struct UnitStruct;
+
+impl UnitStruct {
+    pub fn method_function() {}


@nrc, I don't know anything about this, maybe you do?

mgattozzi · 2017-09-28T13:31:48Z

Yeah it might just something with rls-analysis or something in your code (which I doubt).

projektir · 2017-09-30T00:20:50Z

All right, so I added everything that I could figure out. I'd like to say that the following DefKinds can be marked as done for #121: Trait, Function, Mod, Type, Static, Const.
Tuple and Local should just be removed maybe since they won't be implemented? For the rest there seem to be some bugs.

I've also added various examples to the example lib.rs.

The new lib.rs contains the following: https://gist.github.com/projektir/5f51fc77fe0608e06f3d374d1b1bce03

Here's the resulting data.json: https://gist.github.com/projektir/f4070a06cb88b8e8093cdfd2f54dd24b

Seeing a few weird things:

Absence of Macros and Unions as we already know.
Can't see the impl and therefore Methods as we already know.
Information seems to get duplicated for Structs, struct Fields, and Enums. If you look you'll see EnumVariant is listed twice, etc. This doesn't happen for modules...
When child_structs and such is used, it seems that it'd be more accurate to say struct_children... or just plain children... but this is odd in general because while the ContainerStruct has as a field that's not a struct, its type is still marked as struct (see comment in test case). That doesn't seem right. Similarly, enum variants have type enum, I think this is a related thing.
There's no way to see docs for EnumVariants. It seems to be because there's no docs attribute for children, only for top level includes, so I am not sure if EnumVariants need to be listed as includes or what.
Docs representation. Multiple lines of docs seem to go poof, only the first line is in the .json. Doc lines sometimes include spaces or \n, but this may be a problem with my IDE or something.

projektir · 2017-09-30T00:22:26Z

tests/source/fields.rs

+// @has /included/0/attributes/docs ''
+
+// This will fail, the type of 'fields::StructWithFields::integer' is 'struct'
+// has /included/0/relationships/child_structs/data/0/type 'field'


I expect this to be a field, but it's a struct. It seems "children" of something all get set to have the type as the parent, which of course is not true for complex types like Structs or Enums.

Hm, that seems... less than ideal.

projektir · 2017-09-30T00:23:03Z

tests/source/structs.rs

+// @has /included/1/type 'struct'
+// @has /included/1/id 'structs::ContainerStruct'
+// @has /included/1/attributes/name 'ContainerStruct'
+// @has /included/1/attributes/docs 'A struct that contains another struct.'


As you can see there's no "Docs for the ContainerStruct." here. That part of the documentation just seems to disappear.

projektir · 2017-09-30T00:24:15Z

tests/source/enums.rs

+
+/// An enum.
+pub enum SampleEnum {
+    /// An enum variant.


No way to see these docs that I can find.

steveklabnik · 2017-09-30T11:43:04Z

When child_structs and such is used, it seems that it'd be more accurate to say struct_children... or just plain children... but this is odd in general because while the ContainerStruct has as a field that's not a struct, its type is still marked as struct (see comment in test case). That doesn't seem right. Similarly, enum variants have type enum, I think this is a related thing.

Yeah, so I was doing the child thing because modules have children, but other types don't, so I'm wondering if this isn't, in the end, an anti-pattern. Hrm.

nrc · 2017-10-05T01:05:54Z

I think that I would avoid surfacing 'children' to the user at all - it's a bit vague and doesn't reflect any term in the code. I would use different terms for different structures - variants of an enum, fields of a struct, etc. Internally, it makes sense to use children, at least for save-analysis where we try to abstract things as much as possible.

It seems like there are a few rough edges with the save-analysis data. I think they should mostly be easy to fix (unions, docs for enum variants, etc.). If you could file issues on the rls-analysis repo it is easier for me than the Rust repo (just too many notifications). I'll try and fix them or organise an impl period/mentoring effort around them when I get back from parental leave.

projektir · 2017-10-07T23:14:05Z

@nrc
I've made some issues for the stuff that I found. Note that my understanding of rls-analysis is limited, so I may be looking at the wrong thing somewhere.

I'd be happy to try to work on those issues, as well.

nrc · 2017-10-09T12:10:11Z

@projektir thanks for filing the issues! It would be great to have you work on some of them. I'm currently on parental leave, but I'll write some instructions when I get back in a couple of weeks.

mgattozzi · 2017-10-09T16:54:46Z

@nrc if there's instructions left I can take a crack at them as well so we can get this PR moving.

steveklabnik · 2017-10-16T18:12:10Z

Question: since this has been a while, do we want to merge, with the understanding that some stuff doesn't work yet, rather than block this on upstream work?

steveklabnik · 2017-10-17T20:02:51Z

src/json/mod.rs

@@ -88,6 +101,19 @@ pub fn create_json(host: &AnalysisHost, crate_name: &str) -> Result<String> {
        let (ty, child_ty) = match def.kind {
            DefKind::Mod => (String::from("module"), String::from("child_modules")),
            DefKind::Struct => (String::from("struct"), String::from("child_structs")),
+            DefKind::Enum => (String::from("enum"), String::from("child_enums")),


yeah, there's no real reason that we need the child stuff here; i was cargo-culting from modules

this also means that this block and the one above are identical except for modules, which is annoying

steveklabnik · 2017-10-17T20:04:15Z

Okay, so more thorough review today; we should cut the stuff that doesn't work yet, and leave in the stuff that does. wrt the child stuff, let's make it so that only modules does the child thing. And then we can ship this. Sound good?

steveklabnik · 2017-10-18T19:45:39Z

Appveyor is failing on traits, but not for me or @projektir locally :/

steveklabnik · 2017-10-18T20:54:44Z

Update: needed to rebase the branch; appveyor basically does this. Fixed that up. Now to figure out enums...

This adds as much stuff as we can so far; some issues like rust-dev-tools/rls-analysis#98 mean that we can't do everything. Fixes #121

steveklabnik · 2017-10-19T13:16:51Z

🎊

nrc · 2017-11-02T01:48:17Z

@projektir Hey, are you still interested in working on some rls-analysis issues? I'm back from leave now, so you can ping me anytime in #rust-dev-tools or @Rustc.

Some basic instructions for how to tackle these issues:

I track the issues on the rls-analysis repo (you've already filed some issues there)
the first job is figuring out where in the toolchain the problem is. There are only really three options:
- rls-analysis - we get correct data from the compiler but then screw things up when processing the data. This is actually fairly rare. When it does happen, 9 times out of 10 you need to look in lowering.rs. If you log the info! statements, you should get a good idea of what is happening.
- librustc_save_analysis - conceptually this is the last step of the compiler and it processes the compiler's data into the save-analysis JSON. Most bugs are here.
- spans - sometimes save-analysis is working properly but the input is wrong. In this case the bug is usually in the parser and it is causing save-analysis to skip an expression or get the analysis wrong when it shouldn't.
Write a small test case, then look at the JSON emitted by the compiler when run with -Zsave-analysis. You should be able to interpret the defs and refs to work out if the correct info is there. If it is, then the bug is in rls-analysis, if it isn't then the bug is in the compiler. Note that a ref without a def is ignored, so even if there is a ref, if it doesn't point at any def, it may as well be missing.
If the bug is in the compiler, then look in dump_visitor. It is a visitor which walks the AST, so for your minimal test case you should be able to see where we ought to be dumping the node. Then use a debugger or write printlns to work out what is going on.
Bugs with macros (or other generated code) tend to be 10x harder to solve, I'd ignore them to start with.

nrc · 2017-11-02T02:01:23Z

previous comment also for @mgattozzi

mgattozzi · 2017-11-02T02:58:09Z

@nrc I need to find some time hopefully soon I've been busier than expected but if I can I'd like to take a crack at it. I'll ping you whenever I have the time.

projektir commented Sep 27, 2017

View reviewed changes

steveklabnik reviewed Sep 28, 2017

View reviewed changes

mgattozzi mentioned this pull request Sep 29, 2017

Does rls-analysis support everything we need? #7

Open

projektir commented Sep 30, 2017

View reviewed changes

mgattozzi mentioned this pull request Oct 1, 2017

Maintaining Up To Date Documentation #181

Open

steveklabnik reviewed Oct 17, 2017

View reviewed changes

projektir added 2 commits October 18, 2017 20:49

Adding more DefKinds to the backend

5b92bba

This adds as much stuff as we can so far; some issues like rust-dev-tools/rls-analysis#98 mean that we can't do everything. Fixes #121

Ignoring .vscode/launch.json

e24ae87

steveklabnik merged commit d3584d5 into steveklabnik:master Oct 19, 2017

projektir deleted the backend-defkinds branch October 19, 2017 18:19

steveklabnik mentioned this pull request Oct 30, 2017

Tools news rust-dev-tools/dev-tools-team#28

Closed

Xanewok mentioned this pull request Feb 25, 2019

Missing impls in analysis rust-lang/rls#1364

Open

		@@ -0,0 +1,9 @@
		#![crate_type = "lib"]

		// @has /data/relationships/macros/data/0 'macro'

Adding all the DefKinds #121 #177

Adding all the DefKinds #121 #177

Conversation

projektir commented Sep 27, 2017 • edited Loading

steveklabnik commented Sep 27, 2017 • edited Loading

mgattozzi commented Sep 27, 2017

projektir commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

projektir commented Sep 27, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgattozzi commented Sep 28, 2017

projektir commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Sep 30, 2017

nrc commented Oct 5, 2017

projektir commented Oct 7, 2017

nrc commented Oct 9, 2017

mgattozzi commented Oct 9, 2017

steveklabnik commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Oct 17, 2017

steveklabnik commented Oct 18, 2017

steveklabnik commented Oct 18, 2017

steveklabnik commented Oct 19, 2017

nrc commented Nov 2, 2017

nrc commented Nov 2, 2017

mgattozzi commented Nov 2, 2017

projektir commented Sep 27, 2017 •

edited

Loading

steveklabnik commented Sep 27, 2017 •

edited

Loading