Skip to content

Fix handling of item names for HIR #78345

New issue

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

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

Already on GitHub? # to your account

Merged
merged 2 commits into from
Nov 9, 2020
Merged

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 25, 2020

  • Handle variants, fields, macros in Node::ident()
  • Handle the crate root in opt_item_name
  • Rewrite item_name in terms of opt_item_name

I need this for both #77820 and #78082, so splitting it out into a separate PR so it can land early.

@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-HIR Area: The high-level intermediate representation (HIR) labels Oct 25, 2020
@rust-highfive
Copy link
Contributor

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2020
@jyn514
Copy link
Member Author

jyn514 commented Oct 25, 2020

I don't understand this failure:

---- [incremental] incremental/spans_in_type_debuginfo.rs stdout ----

error in revision `rpass2`: compilation failed!
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/spans_in_type_debuginfo.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "rpass2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/spans_in_type_debuginfo/spans_in_type_debuginfo.inc" "-Z" "incremental-verify-ich" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/spans_in_type_debuginfo/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-g" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/spans_in_type_debuginfo/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
warning: variant is never constructed: `B`
  --> /checkout/src/test/incremental/spans_in_type_debuginfo.rs:39:9
   |
LL |         B(u32),
   |         ^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

error: CGU-reuse for `spans_in_type_debuginfo-enums` is `No` but should be at least `PreLto`
  --> /checkout/src/test/incremental/spans_in_type_debuginfo.rs:9:1
   |
LL | #![rustc_partition_reused(module="spans_in_type_debuginfo-enums", cfg="rpass2")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: CGU-reuse for `spans_in_type_debuginfo-structs` is `No` but should be at least `PreLto`
  --> /checkout/src/test/incremental/spans_in_type_debuginfo.rs:8:1
   |
LL | #![rustc_partition_reused(module="spans_in_type_debuginfo-structs", cfg="rpass2")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 2 previous errors; 1 warning emitted

------------------------------------------

Is the test maybe wrong and depending that both the names are None?

@jyn514
Copy link
Member Author

jyn514 commented Oct 25, 2020

Oh - maybe the issue is that item_name is a query and can't depend on spans? But it removes the span from opt_item_name ... I'm not sure what's going wrong.

@jyn514
Copy link
Member Author

jyn514 commented Nov 1, 2020

ping @varkor - do you know what's going wrong here?

@jyn514

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Nov 1, 2020

Ok I think I see the issue - if item_name is called, we need to use the DefPath first, which doesn't have an associated span. But if opt_item_name is called we need to use hir().get_if_local() so the span will be right. I'll write that up.

@jyn514
Copy link
Member Author

jyn514 commented Nov 1, 2020

Ugh - this still doesn't do what I want because it still panics if you call opt_item_name on the crate root.

---- [rustdoc] rustdoc/without-redirect.rs stdout ----

error: rustdoc failed!
status: exit code: 101
------------------------------------------
stderr:
------------------------------------------
error: internal compiler error: compiler/rustc_middle/src/hir/map/mod.rs:477:41: couldn't find hir id HirId { owner: DefId(0:0 ~ foo[8787]), local_id: 0 } in the HIR map

@jyn514
Copy link
Member Author

jyn514 commented Nov 2, 2020

Ok, tested this by rebasing #77820 on top and it finally works 🎉 now I only have the problems from #77820 to solve 😆

@jyn514
Copy link
Member Author

jyn514 commented Nov 6, 2020

ping @varkor - is this waiting on anything from me?

@varkor
Copy link
Member

varkor commented Nov 7, 2020

I thought you were waiting for #77820 to land if this is rebased on top of it?

@jyn514
Copy link
Member Author

jyn514 commented Nov 7, 2020

@varkor it's the other way around - #77820 is rebased on top of this PR and needs it to land.

@jyn514
Copy link
Member Author

jyn514 commented Nov 7, 2020

The thing is there's no tests for this outside of #77820, which is why I've been constantly rebasing it.

@varkor
Copy link
Member

varkor commented Nov 7, 2020

Ah, my mistake. If the extra line can be removed, that'd be preferable. In either case r=me after checking that.

- Handle variants, fields, macros in `Node::ident()`
- Handle the crate root in `opt_item_name`
- Factor out `item_name_from_def_id` to reduce duplication
- Look at HIR before the DefId for `opt_item_name`

  This gives accurate spans, which are not available from serialized
  metadata.

- Don't panic on the crate root in `opt_item_name`
- Add comments
@jyn514
Copy link
Member Author

jyn514 commented Nov 7, 2020

If CI passes this is good to go, otherwise I need to add back the call to item_name_from_hir.

@jyn514
Copy link
Member Author

jyn514 commented Nov 7, 2020

@bors r=varkor

@bors
Copy link
Collaborator

bors commented Nov 7, 2020

📌 Commit f60fd49 has been approved by varkor

@bors 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 Nov 7, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
Fix handling of item names for HIR

- Handle variants, fields, macros in `Node::ident()`
- Handle the crate root in `opt_item_name`
- Rewrite `item_name` in terms of `opt_item_name`

I need this for both rust-lang#77820 and rust-lang#78082, so splitting it out into a separate PR so it can land early.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
Fix handling of item names for HIR

- Handle variants, fields, macros in `Node::ident()`
- Handle the crate root in `opt_item_name`
- Rewrite `item_name` in terms of `opt_item_name`

I need this for both rust-lang#77820 and rust-lang#78082, so splitting it out into a separate PR so it can land early.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#77640 (Refactor IntErrorKind to avoid "underflow" terminology)
 - rust-lang#78026 (Define `fs::hard_link` to not follow symlinks.)
 - rust-lang#78114 (Recognize `private_intra_doc_links` as a lint)
 - rust-lang#78228 (Promote aarch64-unknown-linux-gnu to Tier 1)
 - rust-lang#78345 (Fix handling of item names for HIR)
 - rust-lang#78437 (BTreeMap: stop mistaking node for an orderly place)
 - rust-lang#78476 (fix some incorrect aliasing in the BTree)
 - rust-lang#78674 (inliner: Use substs_for_mir_body)
 - rust-lang#78748 (Implement destructuring assignment for tuples)
 - rust-lang#78868 (Fix tab focus on restyled switches)
 - rust-lang#78878 (Avoid overlapping cfg attributes when both macOS and aarch64)
 - rust-lang#78882 (Nicer hunk headers for rust files)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 12c5f78 into rust-lang:master Nov 9, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 9, 2020
@jyn514 jyn514 deleted the proper-names branch November 9, 2020 04:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-HIR Area: The high-level intermediate representation (HIR) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants