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.

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

Improve pretty-printing of HirIdValidator errors #107515

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

Swatinem
Copy link
Contributor

This now uses node_to_string for both missing and seen Ids, which includes the snippet of code for which the Id was allocated. Also removes the duplicated printing of HirId, as node_to_string also includes that.

@rustbot
Copy link
Collaborator

rustbot commented Jan 31, 2023

r? @compiler-errors

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 31, 2023
@Swatinem
Copy link
Contributor Author

Before:

ItemLocalIds not assigned densely in ::example. Max ItemLocalId = 22, missing IDs = [
    "[local_id: 3, owner: ::example]",
]; seens IDs = [
    "(HirId(DefId(0:6 ~ rust_out[ddcc]::example).0) fn example (hir_id=HirId(DefId(0:6 ~ rust_out[ddcc]::example).0)))",
    "(HirId(DefId(0:6 ~ rust_out[ddcc]::example).1) param x: &'a str (hir_id=HirId(DefId(0:6 ~ rust_out[ddcc]::example).1)))",
    "(HirId(DefId(0:6 ~ rust_out[ddcc]::example).2) pat x (hir_id=HirId(DefId(0:6 ~ rust_out[ddcc]::example).2)))",
    "(HirId(DefId(0:6 ~ rust_out[ddcc]::example).4) type async move { x.len() } (hir_id=HirId(DefId(0:6 ~ rust_out[ddcc]::example).4)))",

After:

ItemLocalIds not assigned densely in ::example. Max ItemLocalId = 22, missing IDs = [
    "unknown node (hir_id=HirId(DefId(0:3 ~ foo[b698]::example).3))",
]; seen IDs = [
    "fn example (hir_id=HirId(DefId(0:3 ~ foo[b698]::example).0))",
    "param x: &'a str (hir_id=HirId(DefId(0:3 ~ foo[b698]::example).1))",
    "pat x (hir_id=HirId(DefId(0:3 ~ foo[b698]::example).2))",
    "type async move { x.len() } (hir_id=HirId(DefId(0:3 ~ foo[b698]::example).4))",

@eggyal
Copy link
Contributor

eggyal commented Jan 31, 2023

Is the "before" example in #107515 (comment) correct? It too shows the code snippet—indeed it looks like it contains the same information as in the "after" example (albeit with the duplication you mention), except in the latter the layout/order has become a little more difficult for my brain to parse?

@Swatinem
Copy link
Contributor Author

The code snippet was there before, though the format was rather ($hirid $codesnippet ($hirid)) so the id was duplicated. The code snippet was not present in the "missing IDs" before.

@Noratrieb
Copy link
Member

Printing the HirIds first as before looks way more readable to me since they are aligned. I think we should delete the trailing HirId instead.

@Swatinem
Copy link
Contributor Author

Swatinem commented Feb 1, 2023

I completely changed the node_to_string output now. It will always print the HirId first, and then info about the item. I also cleanup up other users which similarly were printing the ID multiple times.

This means that all the output (I only found bug! and debug! output) that was using this will change as well!

Let me try to capture another example later, as I already fixed the case where I was running into this.

@Swatinem
Copy link
Contributor Author

Swatinem commented Feb 1, 2023

Now looks like this:

ItemLocalIds not assigned densely in ::example. Max ItemLocalId = 24, missing IDs = [
    "HirId(DefId(0:4 ~ foo[b698]::example).4) (unknown node)",
]; seen IDs = [
    "HirId(DefId(0:4 ~ foo[b698]::example).0) (fn example)",
    "HirId(DefId(0:4 ~ foo[b698]::example).1) (param `x: &'a str`)",
    "HirId(DefId(0:4 ~ foo[b698]::example).2) (pat `x`)",
    "HirId(DefId(0:4 ~ foo[b698]::example).3) (expr `async move { x.len() }`)",
    "HirId(DefId(0:4 ~ foo[b698]::example).5) (type `async move { x.len() }`)",

This now uses `node_to_string` for both missing and seen Ids, which includes
the snippet of code for which the Id was allocated.
Also removes the duplicated printing of `HirId`, as `node_to_string` includes that already.
Similarly, changes all other users of `node_to_string` that do so, and changes the output of `node_to_string`, which is now "$hirid ($what `$span` in $path)".
@compiler-errors
Copy link
Member

Cool, given that this is internal-only, I'm happy with merging this now. I'll approve once CI is green.

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 2, 2023

📌 Commit 3a75f10 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Feb 2, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 2, 2023
…errors

Improve pretty-printing of `HirIdValidator` errors

This now uses `node_to_string` for both missing and seen Ids, which includes the snippet of code for which the Id was allocated. Also removes the duplicated printing of `HirId`, as `node_to_string` also includes that.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 2, 2023
…errors

Improve pretty-printing of `HirIdValidator` errors

This now uses `node_to_string` for both missing and seen Ids, which includes the snippet of code for which the Id was allocated. Also removes the duplicated printing of `HirId`, as `node_to_string` also includes that.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 2, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#106919 (Recover `_` as `..` in field pattern)
 - rust-lang#107493 (Improve diagnostic for missing space in range pattern)
 - rust-lang#107515 (Improve pretty-printing of `HirIdValidator` errors)
 - rust-lang#107524 (Remove both StorageLive and StorageDead in CopyProp.)
 - rust-lang#107532 (Erase regions before doing uninhabited check in borrowck)
 - rust-lang#107559 (Rename `rust_2015` → `is_rust_2015`)
 - rust-lang#107577 (Reinstate the `hir-stats.rs` tests on stage 1.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 230c9e9 into rust-lang:master Feb 2, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 2, 2023
@Swatinem Swatinem deleted the hirvalidator branch February 15, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants