Move methods from Map to TyCtxt, part 4.

[nnethercote]

A follow-up to #137350.

r? @Zalathar

[rustbot]

Some changes occurred in match checking

cc @Nadrieril

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred in need_type_info.rs

cc @lcnr

HIR ty lowering was modified

cc @fmease

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

[bors]

☔ The latest upstream changes (presumably #135726) made this pull request unmergeable. Please resolve the merge conflicts.

[Zalathar]

Feedback: From the perspective of reviewing bulk changes, I would prefer not to have things like the inlining/renaming of node_to_string in the same PR, even in a separate patch, unless absolutely necessary.

(Partly because it expands the scope of the review to include non-trivial changes, and partly because many-file PRs make GitHub very slow in ways that make me want to avoid any other kind of review friction.)

[nnethercote]

Huh.

So this is part 4 of my work to get rid of Map. Even though getting rid of Map is conceptually a single change, I've split it across multiple PRs because I didn't want to move every method at once, because that would create enormous diffs that are hard to review.

And in this PR I could have just renamed node_to_string as hir_node_to_string and left it as a trivial wrapper for hir_id_to_string. But in #136466 when I left hir_krate as a trivial wrapper around hir_crate cjgillot suggested that I remove it. Which I did in a separate commit. So I figured I would do the same here, and as part of the change I moved node_to_string into a method because it was odd having it as a free function when everything else is methods. And I put it in its own commit because it's slightly different to the mass renamings, to make it easier to review. Much like I did with the query renaming.

IIUC, you would have preferred me to leave the trivial wrapper in place and remove it in a separate PR, because that would make reviewing easier. Is that right?

[Zalathar]

IIUC, you would have preferred me to leave the trivial wrapper in place and remove it in a separate PR, because that would make reviewing easier. Is that right?

That's right.

For context, my workflow for reviewing these renamings has been to scroll through the list of changes, look at each change and say “that's trivial”, and then mark the file as viewed in the GitHub review UI. That UI for marking files doesn't work properly when viewing individual commits, which is why splitting off a separate commit didn't help in this case, even though it's normally a good idea.

So for a bulk-renaming PR like this, my approach as an author would have been to avoid any complication that isn't absolutely necessary (with hir_attr_map being a good example of a necessary change), especially since the overall migration is being split across several PRs anyway.

[nnethercote]

Huh, I've never noticed that the per-commit view lacks the "viewed" checkbox. Probably because I just click on the chevron in the top left if I've finished reviewing a file. That collapses it, just like clicking on the "viewed" checkbox does.

[rustbot]

Some changes occurred to constck

cc @fee1-dead

[nnethercote]

I rebased and fixed the broken link in the guide.

[nnethercote]

Updated again to fix the compile errors.

[nnethercote]

@Zalathar: any other changes needed here?

[Zalathar]

Ah sorry, I lost track of this. I'll have another look at it today.

[Zalathar]

@bors r+

[bors]

📌 Commit 1047768 has been approved by Zalathar

It is now in the queue for this repository.

[matthiaskrgr]

@bors r-
need rebase I think

[matthiaskrgr]

@bors try

[bors]

⌛ Trying commit 1047768 with merge 8becda10ff5df6b1ec2906c25213890fcd1921dd...

[bors]

☀️ Try build successful - checks-actions
Build commit: 8becda1 (8becda10ff5df6b1ec2906c25213890fcd1921dd)

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[nnethercote]

I rebased.

@bors r=Zalathar

[bors]

📌 Commit 256c27e has been approved by Zalathar

It is now in the queue for this repository.