change maybe_body_owned_by to take local def id #99311

kckeiks · 2022-07-16T03:18:30Z

Issue #96341
r? @cjgillot

rustbot · 2022-07-16T03:18:33Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

kckeiks · 2022-07-16T03:22:44Z

@cjgillot

Could you please clarify how one should approach converting a HirId to LocalDefId? Should we check that a conversion can be made before calling tcx.hir().local_def_id?
Can every HirId be converted to LocalDefId? Why or why not?

cjgillot · 2022-07-17T20:51:57Z

compiler/rustc_driver/src/pretty.rs

@@ -328,7 +328,7 @@ impl<'tcx> pprust_hir::PpAnn for TypedAnnotation<'tcx> {
            let typeck_results = self.maybe_typeck_results.get().or_else(|| {
                self.tcx
                    .hir()
-                    .maybe_body_owned_by(self.tcx.hir().local_def_id_to_hir_id(expr.hir_id.owner))
+                    .maybe_body_owned_by(self.tcx.hir().local_def_id(expr.hir_id))


local_def_id(local_def_id_to_hir_id(def_id)) is def_id by definition.

Suggested change

.maybe_body_owned_by(self.tcx.hir().local_def_id(expr.hir_id))

.maybe_body_owned_by(expr.hir_id.owner)

Sorry, I was using expr.hir_id.owner initially but I was unsure because of the different ways there are to convert HirId to LocalDefId like .owner, local_def_id or {expect | as }_owner.

cjgillot · 2022-07-17T20:52:46Z

compiler/rustc_metadata/src/rmeta/encoder.rs

-        let def_id = self.tcx.hir().local_def_id(id);
-        debug!("EncodeContext::encode_info_for_anon_const({:?})", def_id);
-        let body_id = self.tcx.hir().body_owned_by(id);
+        let local_did = self.tcx.hir().local_def_id(id);


Why the rename?

I keep seeing local_did, did and def_id used for LocalDefId so I changed it to keep it consistent with local_did.

cjgillot · 2022-07-17T20:54:43Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -398,7 +398,7 @@ impl<'hir> Map<'hir> {

    pub fn enclosing_body_owner(self, hir_id: HirId) -> HirId {
        for (parent, _) in self.parent_iter(hir_id) {
-            if let Some(body) = self.maybe_body_owned_by(parent) {
+            if let Some(local_did) = parent.as_owner() && let Some(body) = self.maybe_body_owned_by(local_did) {


Inlining the former implementation of maybe_body_owned_by here is probably a better solution.

This change fixed all broken tests. What is logically wrong with this condition let Some(local_did) = parent.as_owner() && let Some(body) = self.maybe_body_owned_by(local_did)?

cjgillot · 2022-07-17T20:56:12Z

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

@@ -26,7 +26,7 @@ use rustc_span::{BytePos, Span};
 pub(crate) fn check_match(tcx: TyCtxt<'_>, def_id: DefId) {
    let body_id = match def_id.as_local() {
        None => return,
-        Some(id) => tcx.hir().body_owned_by(tcx.hir().local_def_id_to_hir_id(id)),
+        Some(did) => tcx.hir().body_owned_by(did),


Suggested change

Some(did) => tcx.hir().body_owned_by(did),

Some(def_id) => tcx.hir().body_owned_by(def_id),

cjgillot · 2022-07-17T20:57:21Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -1761,8 +1761,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {

        let generator_body = generator_did
            .as_local()
-            .map(|def_id| hir.local_def_id_to_hir_id(def_id))
-            .and_then(|hir_id| hir.maybe_body_owned_by(hir_id))
+            .and_then(|local_did| hir.maybe_body_owned_by(local_did))


Suggested change

.and_then(|local_did| hir.maybe_body_owned_by(local_did))

.and_then(|def_id| hir.maybe_body_owned_by(def_id))

cjgillot · 2022-07-17T21:00:04Z

compiler/rustc_ty_utils/src/ty.rs

+    let body_id = local_did
+        .and_then(|id| tcx.hir().maybe_body_owned_by(id).map(|body| body.hir_id))
+        .or(hir_id)
+        .map_or(hir::CRATE_HIR_ID, |did| did);


Could you simplify the combinators here?

compiler/rustc_typeck/src/check/expr.rs

cjgillot · 2022-07-17T21:03:00Z

src/tools/clippy/clippy_lints/src/utils/author.rs

@@ -138,7 +138,7 @@ impl<'tcx> LateLintPass<'tcx> for Author {

 fn check_item(cx: &LateContext<'_>, hir_id: HirId) {
    let hir = cx.tcx.hir();
-    if let Some(body_id) = hir.maybe_body_owned_by(hir_id) {
+    if let Some(body_id) = hir.maybe_body_owned_by(hir.local_def_id(hir_id)) {


This function is only called for HIR owners.

Suggested change

if let Some(body_id) = hir.maybe_body_owned_by(hir.local_def_id(hir_id)) {

if let Some(body_id) = hir.maybe_body_owned_by(hir_id.expect_owner()) {

kckeiks · 2022-07-20T03:23:49Z

src/tools/clippy/clippy_lints/src/methods/suspicious_map.rs

@@ -12,7 +13,7 @@ pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, count_recv: &hi
    if_chain! {
        if is_trait_method(cx, count_recv, sym::Iterator);
        let closure = expr_or_init(cx, map_arg);
-        if let Some(body_id) = cx.tcx.hir().maybe_body_owned_by(closure.hir_id);
+        if let Some(body_id) = cx.tcx.hir().find(closure.hir_id).map(map::associated_body).flatten();


@cjgillot I initially used closure.hir_id.owner but it broke the test for this function so on a whim I used .find(closure.hir_id).map(map::associated_body).flatten() and it worked. Why didn't maybe_body_owned_by(LocalDefId) worked but using the Node works? It looks like maybe_body_owned_by also uses find to get the Node.

For closures, hir_id.owner and local_def_id(hir_id) point to different things. The former points to the enclosing function, the latter to the closure.
Using local_def_id(closure.hir_id) should work.
Is it enough to make associated_body private?

Using local_def_id(closure.hir_id) does not work. Here is the error:

+error: internal compiler error: /home/fausto/Repo/rust/compiler/rustc_middle/src/hir/map/mod.rs:195:13: local_def_id: no entry for `HirId { owner: DefId(0:6 ~ suspicious_map[53a1]::negative), local_id: 79 }`, which has a map of `Some(Expr(Expr { hir_id: HirId { owner: DefId(0:6 ~ suspicious_map[53a1]::negative), local_id: 79 }, kind: Path(Resolved(None, Path { span: $DIR/suspicious_map.rs:27:24: 27:36 (#0), res: Def(Fn, DefId(0:10 ~ suspicious_map[53a1]::do_something)), segments: [PathSegment { ident: do_something#0, hir_id: Some(HirId { owner: DefId(0:6 ~ suspicious_map[53a1]::negative), local_id: 78 }), res: Some(Def(Fn, DefId(0:10 ~ suspicious_map[53a1]::do_something))), args: None, infer_args: true }] })), span: $DIR/suspicious_map.rs:27:24: 27:36 (#0) }))` + +thread 'rustc' panicked at 'Box<dyn Any>', /home/fausto/Repo/rust/compiler/rustc_errors/src/lib.rs:1392:9 +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace + +note: the compiler unexpectedly panicked. this is a bug. + +note: we would appreciate a bug report: https://github.com/rust-lang/rust-clippy/issues/new + +note: Clippy version: clippy 0.1.64 (9b485f92000 2022-07-19) + +query stack during panic: +#0 [analysis] running analysis passes on this crate +end of query stack +error: aborting due to 3 previous errors

Aside from this, I think all tasks are done. Let me know if I missed something.

As the code does not check whether this is a closure beforehand. You can use tcx.hir().opt_local_def_id which won't ICE on failure, and just return None.

bors · 2022-07-26T02:22:59Z

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

rustbot · 2022-07-29T20:42:38Z

Some changes occurred in src/tools/cargo

cc @ehuss

The Miri submodule was changed

cc @rust-lang/miri

kckeiks · 2022-07-29T20:44:58Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

cjgillot · 2022-07-30T13:05:08Z

Thanks @kckeiks!
@bors r+

bors · 2022-07-30T13:05:09Z

📌 Commit 94611b8 has been approved by cjgillot

It is now in the queue for this repository.

Rollup of 5 pull requests Successful merges: - rust-lang#99311 (change maybe_body_owned_by to take local def id) - rust-lang#99862 (Improve type mismatch w/ function signatures) - rust-lang#99895 (don't call type ascription "cast") - rust-lang#99900 (remove some manual hash stable impls) - rust-lang#99903 (Add diagnostic when using public instead of pub) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 16, 2022

rust-highfive assigned cjgillot Jul 16, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2022