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.

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

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

Merged
merged 1 commit into from Jan 13, 2019

Conversation

Projects
None yet
7 participants
@emilio
Copy link
Contributor

emilio commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 9, 2019

r? @nikomatsakis

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

@emilio

This comment has been minimized.

Copy link
Contributor

emilio commented Jan 9, 2019

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);
Some(def) => def,
None => HirDef::Err,
Node::PathSegment(seg) => {
let def = match seg.def {

This comment has been minimized.

@Xanewok

Xanewok Jan 9, 2019

Member

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.

This comment has been minimized.

@emilio

emilio Jan 9, 2019

Contributor

Sure, I can write it that way.

@Xanewok

This comment has been minimized.

Copy link
Member

Xanewok commented Jan 9, 2019

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 😅

save-analysis: Get path def from parent in case there's no def for th…
…e path itself.

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 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 emilio force-pushed the emilio:save-analysis-path branch from 80d53f0 to c47ed14 Jan 9, 2019

@emilio

This comment has been minimized.

Copy link
Contributor

emilio commented Jan 9, 2019

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

Fixed the nit too.

r? @nrc

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 9, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 9, 2019

📌 Commit c47ed14 has been approved by nrc

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

⌛️ Testing commit c47ed14 with merge db7a5bd...

bors added a commit that referenced this pull request Jan 10, 2019

Auto merge of #57474 - emilio:save-analysis-path, r=nrc
save-analysis: Get path def from parent in case there's no def for the path itself.

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.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 10, 2019

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

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Jan 12, 2019

Rollup merge of rust-lang#57474 - emilio:save-analysis-path, r=nrc
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.

bors added a commit that referenced this pull request Jan 12, 2019

Auto merge of #57554 - pietroalbini:rollup, r=pietroalbini
Rollup of 15 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).)
 - #57392 (Always calculate glob map but only for glob uses)
 - #57412 (Improve the wording)
 - #57436 (save-analysis: use a fallback when access levels couldn't be computed)
 - #57442 (Simplify `ConstValue::ScalarPair`)
 - #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)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57474 - emilio:save-analysis-path, r=nrc
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.

bors added a commit that referenced this pull request Jan 13, 2019

Auto merge of #57561 - Centril:rollup, r=Centril
Rollup of 15 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).)
 - #57392 (Always calculate glob map but only for glob uses)
 - #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)
 - #57560 (hygiene: Do not treat `Self` ctor as a local variable)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57474 - emilio:save-analysis-path, r=nrc
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.

bors added a commit that referenced this pull request Jan 13, 2019

Auto merge of #57568 - Centril:rollup, r=Centril
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

bors added a commit that referenced this pull request Jan 13, 2019

Auto merge of #57568 - Centril:rollup, r=Centril
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

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

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment