Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

save-analysis doesn't dump ref entry for struct name in static method calls #57462

Closed
staktrace opened this issue Jan 9, 2019 · 7 comments · Fixed by #57474
Closed

save-analysis doesn't dump ref entry for struct name in static method calls #57462

staktrace opened this issue Jan 9, 2019 · 7 comments · Fixed by #57474
Labels
A-save-analysis Area: saving results of analyses such as inference and borrowck results to a file. C-bug Category: This is a bug.

Comments

@staktrace
Copy link
Contributor

Here's a small testcase:

struct Foo {
}

impl Foo {
  fn new() {
  }
}

fn main() {
  Foo::new();
}

If you run this through rustc -Z save-analysis and look at the save-analysis JSON file, you'll see that there is no entry for the Foo part of Foo::new(). This seems like a bug.
@staktrace staktrace changed the title save-analysis doesn't dump save-analysis doesn't dump ref entry for struct name in static method calls Jan 9, 2019
@staktrace
Copy link
Contributor Author

/cc @nrc

@staktrace
Copy link
Contributor Author

staktrace commented Jan 9, 2019 •
edited
Loading

According to rustc -Z ast-json test.rs the expression is a ExprKind::Call where the Foo::new part would be a Path with two PathSegments. This seems to be processed in process_path here. Not obvious to me why it would fail, since the write_sub_paths_truncated at the end looks like it should deal with the first path segment.

@Xanewok
Copy link
Member

Xanewok commented Jan 9, 2019

cc me

@emilio
Copy link
Contributor

emilio commented Jan 9, 2019

I'm taking a look at this, fwiw. process_method_call does nothing with the segment, looks like it should.

@emilio
Copy link
Contributor

emilio commented Jan 9, 2019

Oh, it's a bit more subtle, but I see what's going on now.

@emilio
Copy link
Contributor

emilio commented Jan 9, 2019

I got a fix, testing to see if I can remove hacks to existing code now.

@Centril Centril added A-save-analysis Area: saving results of analyses such as inference and borrowck results to a file. C-bug Category: This is a bug. labels Jan 9, 2019
@emilio emilio mentioned this issue Jan 9, 2019
Merged
@emilio
Copy link
Contributor

emilio commented Jan 9, 2019

Fix is at #57474, but it's unclear to me how to add a test for this. Any doc I could look at?

emilio added a commit to emilio/rls that referenced this issue Jan 9, 2019
@emilio
tests: Add a test attempt for rust-lang/rust#57462.
0b36c18 
This unfortunately passes regardless right now because of the racer fallback.
And without the racer fallback we're not able to go to the definition of either
Foo or Foo::new.
emilio added a commit to emilio/rls that referenced this issue Jan 9, 2019
@emilio
tests: Add a test attempt for rust-lang/rust#57462.
bd3ddba 
This unfortunately passes regardless right now because of the racer fallback.
And without the racer fallback we're not able to go to the definition of either
Foo or Foo::new.

The test is probably worth having regardless.
emilio added a commit to emilio/rust that referenced this issue Jan 9, 2019
@emilio
save-analysis: Get path def from parent in case there's no def for th…
c47ed14 
…e path itself.

This fixes rust-lang#57462.

The relevant part from the hir type collector is:

```
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(32) => Entry { parent: NodeId(33), dep_node: 4294967040, node: Expr(expr(32: <Foo>::new)) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(48) => Entry { parent: NodeId(32), dep_node: 4294967040, node: Ty(type(Foo)) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(30) => Entry { parent: NodeId(48), dep_node: 4294967040, node: PathSegment(PathSegment { ident: Foo#0, id: Some(NodeId(30)), def: Some(Err), args: None, infer_types: true }) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(31) => Entry { parent: NodeId(32), dep_node: 4294967040, node: PathSegment(PathSegment { ident: new#0, id: Some(NodeId(31)), def: Some(Err), args: None, infer_types: true }) }
```

We have the right ID when looking for NodeId(31) and try with NodeId(32) (which
is the right thing to look for) from get_path_data, but not for the segments
that we write from `write_sub_paths_truncated`.

Basically `process_path` takes an id which is always the parent, and that we
fall back to in `get_path_data()`, so we get the right result for the last path
segment, but not for the other segments that get written to from
`write_sub_paths_truncated`.

I think we can stop passing the explicit id around to `get_path_data` now, will
consider sending that as a followup.
emilio added a commit to emilio/rls that referenced this issue Jan 9, 2019
@emilio
tests: Add a test for rust-lang/rust#57462 and go-to-definition in ge…
5c0e545 
…neral.
emilio added a commit to emilio/rls that referenced this issue Jan 9, 2019
@emilio
tests: Add a test for rust-lang/rust#57462 and go-to-definition witho…
89eb079 
…ut racer in general.
emilio added a commit to emilio/rls that referenced this issue Jan 9, 2019
@emilio
tests: Add a test for rust-lang/rust#57462 and go-to-definition witho…
bd003e4 
…ut racer in general.
pietroalbini added a commit to pietroalbini/rust that referenced this issue Jan 12, 2019
@pietroalbini
Rollup merge of rust-lang#57474 - emilio:save-analysis-path, r=nrc
21ed69d 
save-analysis: Get path def from parent in case there's no def for the path itself.

This fixes rust-lang#57462.

The relevant part from the hir type collector is:

```
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(32) => Entry { parent: NodeId(33), dep_node: 4294967040, node: Expr(expr(32: <Foo>::new)) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(48) => Entry { parent: NodeId(32), dep_node: 4294967040, node: Ty(type(Foo)) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(30) => Entry { parent: NodeId(48), dep_node: 4294967040, node: PathSegment(PathSegment { ident: Foo#0, id: Some(NodeId(30)), def: Some(Err), args: None, infer_types: true }) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(31) => Entry { parent: NodeId(32), dep_node: 4294967040, node: PathSegment(PathSegment { ident: new#0, id: Some(NodeId(31)), def: Some(Err), args: None, infer_types: true }) }
```

We have the right ID when looking for NodeId(31) and try with NodeId(32) (which
is the right thing to look for) from get_path_data. But not when we look from `write_sub_paths_truncated`

Basically process_path takes an id which is always the parent, and that we
fall back to in get_path_data(), so we get the right result for the last path
segment, but not for the other segments that get written to from
write_sub_paths_truncated.

I think we can stop passing the explicit `id` around to get_path_data as a followup.
Centril added a commit to Centril/rust that referenced this issue Jan 13, 2019
@Centril
Rollup merge of rust-lang#57474 - emilio:save-analysis-path, r=nrc
edec838 
save-analysis: Get path def from parent in case there's no def for the path itself.

This fixes rust-lang#57462.

The relevant part from the hir type collector is:

```
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(32) => Entry { parent: NodeId(33), dep_node: 4294967040, node: Expr(expr(32: <Foo>::new)) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(48) => Entry { parent: NodeId(32), dep_node: 4294967040, node: Ty(type(Foo)) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(30) => Entry { parent: NodeId(48), dep_node: 4294967040, node: PathSegment(PathSegment { ident: Foo#0, id: Some(NodeId(30)), def: Some(Err), args: None, infer_types: true }) }
DEBUG 2019-01-09T15:42:58Z: rustc::hir::map::collector: hir_map: NodeId(31) => Entry { parent: NodeId(32), dep_node: 4294967040, node: PathSegment(PathSegment { ident: new#0, id: Some(NodeId(31)), def: Some(Err), args: None, infer_types: true }) }
```

We have the right ID when looking for NodeId(31) and try with NodeId(32) (which
is the right thing to look for) from get_path_data. But not when we look from `write_sub_paths_truncated`

Basically process_path takes an id which is always the parent, and that we
fall back to in get_path_data(), so we get the right result for the last path
segment, but not for the other segments that get written to from
write_sub_paths_truncated.

I think we can stop passing the explicit `id` around to get_path_data as a followup.
emilio added a commit to emilio/rls that referenced this issue Jan 14, 2019
@emilio
tests: Add a test for rust-lang/rust#57462 and go-to-definition witho…
afd1179 
…ut racer in general.
emilio added a commit to emilio/rls that referenced this issue Jan 15, 2019
@emilio
tests: Add a test for rust-lang/rust#57462 and go-to-definition witho…
e9e643b 
…ut racer in general.
bors added a commit to rust-lang/rls that referenced this issue Jan 22, 2019
@bors
Auto merge of #1230 - emilio:testing-testing, r=Xanewok
17dae09 
tests: Add a test for rust-lang/rust#57462 and go-to-definition without racer in general.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-save-analysis Area: saving results of analyses such as inference and borrowck results to a file. C-bug Category: This is a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

save-analysis: Get path def from parent in case there's no def for the path itself. emilio/rust
4 participants
@staktrace @Centril @emilio @Xanewok