save-analysis: Get path def from parent in case there's no def for the path itself. #57474

emilio · 2019-01-09T17:48:03Z

This fixes #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.

rust-highfive · 2019-01-09T17:48:18Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

emilio · 2019-01-09T17:52:37Z

r? @Xanewok or @nrc or @nikomatsakis

Diff for the reduced example:

diff --git a/old.fmt.json b/new.fmt.json
index 2415f6cc49..cbfd1e8b86 100644
--- a/old.fmt.json
+++ b/new.fmt.json
@@ -35,7 +35,7 @@
         "output": [
             116
         ],
-        "program": "/home/emilio/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc"
+        "program": "/home/emilio/.rustup/toolchains/s1/bin/rustc"
     },
     "config": {
         "borrow_data": false,
@@ -236,8 +236,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        4434232721938776227,
-                        17281811030953228010
+                        9127723463649385592,
+                        16992998011563025011
                     ],
                     "name": "std"
                 },
@@ -247,8 +247,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        6745124436050292838,
-                        9023317464072694199
+                        9540902161314303485,
+                        5892744450285911576
                     ],
                     "name": "core"
                 },
@@ -258,8 +258,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        4937334589465572539,
-                        11025774180943919234
+                        3567694970301908068,
+                        12381570538758350463
                     ],
                     "name": "compiler_builtins"
                 },
@@ -269,8 +269,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        14608825675300652641,
-                        5624983184858756260
+                        1268094027406646749,
+                        16671612044650121087
                     ],
                     "name": "rustc_std_workspace_core"
                 },
@@ -280,8 +280,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        4816735864666008363,
-                        16327538614053773527
+                        4081450667420026748,
+                        8748692286566855985
                     ],
                     "name": "alloc"
                 },
@@ -291,8 +291,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        16552867398073770369,
-                        5049264177097161806
+                        2148842693393025388,
+                        2096135594621366634
                     ],
                     "name": "libc"
                 },
@@ -302,8 +302,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        13550048322004745885,
-                        10255425186366194469
+                        637807282419211407,
+                        1094869744078742723
                     ],
                     "name": "rustc_demangle"
                 },
@@ -313,8 +313,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        7472751085928104055,
-                        6713240442092040939
+                        11361088791814475075,
+                        17054808281364937733
                     ],
                     "name": "unwind"
                 },
@@ -324,8 +324,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        10393131234752194917,
-                        798619682818297028
+                        3089822922231137440,
+                        3540503990866678019
                     ],
                     "name": "backtrace_sys"
                 },
@@ -335,8 +335,8 @@
                 "file_name": "/home/emilio/src/moz/rust/t.rs",
                 "id": {
                     "disambiguator": [
-                        7232545494250943698,
-                        1752408679562706164
+                        559735056422858498,
+                        15882156247405266861
                     ],
                     "name": "panic_unwind"
                 },
@@ -400,6 +400,27 @@
                 "line_end": 10,
                 "line_start": 10
             }
+        },
+        {
+            "kind": "Type",
+            "ref_id": {
+                "index": 6,
+                "krate": 0
+            },
+            "span": {
+                "byte_end": 64,
+                "byte_start": 61,
+                "column_end": 6,
+                "column_start": 3,
+                "file_name": [
+                    116,
+                    46,
+                    114,
+                    115
+                ],
+                "line_end": 10,
+                "line_start": 10
+            }
         }
     ],
     "relations": [

I tried to see if existing tests are affected but apparently save-analysis tests aren't here? Or at least ./x.py test -vv src/test/run-make-fulldeps/save-analysis is not changing anything, even though there's a line that tests this, and that I verified changes output with my patch in a similar fashion:

    // static method on struct
    let r = some_fields::stat(y);

Xanewok · 2019-01-09T18:15:14Z

src/librustc_save_analysis/lib.rs

-                Some(def) => def,
-                None => HirDef::Err,
+            Node::PathSegment(seg) => {
+                let def = match seg.def {


Is this equivalent to:

match seg.def { Some(def) if def != HirDef::Err => def, _ => self.get_path_def(self.tcx.hir().get_parent_node(id)), }

And if so, could we keep the concise form? I believe it reads better.

Sure, I can write it that way.

Xanewok · 2019-01-09T18:24:32Z

Just to follow up here, as I mentioned in IRC I think we're better off testing this directly in the RLS, since save-analysis is internal and the RLS is the main consumer/motivation behind that.

Looks like a right thing to do, although I don't have r+ magical powers so I'll have to delegate 😅

…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 · 2019-01-09T19:00:42Z

I tried to add a test in rust-lang/rls#1230, but see the caveat there.

Fixed the nit too.

r? @nrc

nrc · 2019-01-09T20:48:07Z

@bors: r+

bors · 2019-01-09T20:48:09Z

📌 Commit c47ed14 has been approved by nrc

bors · 2019-01-10T06:08:55Z

⌛ Testing commit c47ed14 with merge db7a5bdd855d9a4c4c7323f3059fd473f52adb5c...

bors · 2019-01-10T06:09:05Z

💔 Test failed - status-appveyor

pietroalbini · 2019-01-10T08:20:11Z

@bors retry
AppVeyor... what's wrong with you today?

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.

@ghost

Rollup of 16 pull requests Successful merges: - #57351 (Don't actually create a full MIR stack frame when not needed) - #57353 (Optimise floating point `is_finite` (2x) and `is_infinite` (1.6x).) - #57412 (Improve the wording) - #57436 (save-analysis: use a fallback when access levels couldn't be computed) - #57453 (lldb_batchmode.py: try `import _thread` for Python 3) - #57454 (Some cleanups for core::fmt) - #57461 (Change `String` to `&'static str` in `ParseResult::Failure`.) - #57473 (std: Render large exit codes as hex on Windows) - #57474 (save-analysis: Get path def from parent in case there's no def for the path itself.) - #57494 (Speed up item_bodies for large match statements involving regions) - #57496 (re-do docs for core::cmp) - #57508 (rustdoc: Allow inlining of reexported crates and crate items) - #57547 (Use `ptr::eq` where applicable) - #57557 (resolve: Mark extern crate items as used in more cases) - #57560 (hygiene: Do not treat `Self` ctor as a local variable) - #57564 (Update the const fn tracking issue to the new metabug) Failed merges: r? @ghost

rust-highfive assigned nikomatsakis Jan 9, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2019

emilio mentioned this pull request Jan 9, 2019

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

Closed

Xanewok reviewed Jan 9, 2019

View reviewed changes

emilio force-pushed the save-analysis-path branch from 80d53f0 to c47ed14 Compare January 9, 2019 19:00

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2019

Xanewok mentioned this pull request Jan 9, 2019

tests: Add a test for rust-lang/rust#57462 and go-to-definition without racer in general. rust-lang/rls#1230

Merged

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2019

pietroalbini mentioned this pull request Jan 12, 2019

Rollup of 15 pull requests #57554

Closed

Centril mentioned this pull request Jan 13, 2019

Rollup of 15 pull requests #57561

Closed

Centril mentioned this pull request Jan 13, 2019

Rollup of 16 pull requests #57568

Merged

bors merged commit c47ed14 into rust-lang:master Jan 13, 2019

pnkfelix mentioned this pull request Jan 28, 2019

Project fails (or takes long time) to compile on latest stable and nightly (hang/stackoverflow) #57735

Closed

emilio deleted the save-analysis-path branch February 10, 2019 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

save-analysis: Get path def from parent in case there's no def for the path itself. #57474

save-analysis: Get path def from parent in case there's no def for the path itself. #57474

emilio commented Jan 9, 2019

rust-highfive commented Jan 9, 2019

emilio commented Jan 9, 2019 •

edited

Loading

Xanewok Jan 9, 2019

emilio Jan 9, 2019

Xanewok commented Jan 9, 2019

emilio commented Jan 9, 2019

nrc commented Jan 9, 2019

bors commented Jan 9, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

pietroalbini commented Jan 10, 2019

save-analysis: Get path def from parent in case there's no def for the path itself. #57474

save-analysis: Get path def from parent in case there's no def for the path itself. #57474

Conversation

emilio commented Jan 9, 2019

rust-highfive commented Jan 9, 2019

emilio commented Jan 9, 2019 • edited Loading

Xanewok Jan 9, 2019

Choose a reason for hiding this comment

emilio Jan 9, 2019

Choose a reason for hiding this comment

Xanewok commented Jan 9, 2019

emilio commented Jan 9, 2019

nrc commented Jan 9, 2019

bors commented Jan 9, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

pietroalbini commented Jan 10, 2019

emilio commented Jan 9, 2019 •

edited

Loading