Clarify and rename CrateItem

[celinval]

I was trying to write a test that would traverse the body of every function in a crate, but I couldn't do that by only using StableMIR APIs.

Not every CrateItem is a function, and there is no method to retrieve information about the type of CrateItem, so users can't tell whether it's valid to retrieve the item body. If a user tries to get the body of a non-function, StableMIR crashes. Note that just knowing that an item is a function will likely not be enough, since there are cases where the function body might not be available.

Besides providing more information about a CrateItem, we could also change the body of CrateItem::body to return Result<> instead.

[ouz-a]

@rustbot claim

[ouz-a]

We indeed definitely need to add more helper functions and better printings, for Debug. Similar to what I did here rust-lang/rust#115534

[oli-obk]

Hmm... what items are crate items but have no mir? I thought we explicitly listed items that have mir

[celinval]

If I try to get the body of all items, the following test crashes:

tests/ui/cfg/cfg_stmt_expr.rs FAILED:
command: "/tmp/smir/bin/test-drive" "--error-format=json" "--check-smir" "--out-dir" "/tmp/rust/build" "tests/ui/cfg/cfg_stmt_expr.rs" "--edition" "2021"

thread 'rustc' panicked at compiler/rustc_mir_transform/src/lib.rs:602:24:
do not use `optimized_mir` for constants: Const
stack backtrace:
  0: rust_begin_unwind
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/std/src/panicking.rs:619:5
  1: core::panicking::panic_fmt
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/core/src/panicking.rs:72:14
  2: rustc_mir_transform::optimized_mir
     [... omitted 2 frames ...]
  3: rustc_middle::query::plumbing::query_get_at::<rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, rustc_middle::query::erase::Erased<[u8; 8]>>>
  4: <rustc_smir::rustc_smir::Tables as rustc_smir::stable_mir::Context>::mir_body
  5: <scoped_tls::ScopedKey<core::cell::Cell<*mut ()>>>::with::<rustc_smir::stable_mir::with<rustc_smir::stable_mir::mir::body::Body, <rustc_smir::stable_mir::CrateItem>::body::{closure#0}>::{closure#0}, rustc_smir::stable_mir::mir::body::Body>
  6: <rustc_smir::stable_mir::CrateItem>::body
  7: test_drive::sanity_checks::test_all_fns
            at /smir/tools/test-drive/src/sanity_checks.rs:57:20
  8: test_drive::test_stable_mir::{{closure}}
            at /smir/tools/test-drive/src/main.rs:58:46
  9: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/core/src/panic/unwind_safe.rs:271:9
 10: std::panicking::try::do_call
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/std/src/panicking.rs:526:40
 11: __rust_try
 12: std::panicking::try
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/std/src/panicking.rs:490:19
 13: std::panic::catch_unwind
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/std/src/panic.rs:142:14
 14: test_drive::run_test
            at /smir/tools/test-drive/src/main.rs:93:24
 15: test_drive::test_stable_mir
            at /smir/tools/test-drive/src/main.rs:72:19
 16: <rustc_smir::rustc_internal::StableMir<B,C> as rustc_driver_impl::Callbacks>::after_analysis::{{closure}}::{{closure}}
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_smir/src/rustc_internal/mod.rs:242:36
 17: rustc_smir::stable_mir::run::g::{{closure}}
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_smir/src/stable_mir/mod.rs:179:13
 18: scoped_tls::ScopedKey<T>::set
            at /cargo/registry/src/index.crates.io-6f17d22bba15001f/scoped-tls-1.0.1/src/lib.rs:137:9
 19: rustc_smir::stable_mir::run::g
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_smir/src/stable_mir/mod.rs:178:9
 20: rustc_smir::stable_mir::run
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_smir/src/stable_mir/mod.rs:182:5
 21: rustc_smir::rustc_internal::run
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_smir/src/rustc_internal/mod.rs:170:5
 22: <rustc_smir::rustc_internal::StableMir<B,C> as rustc_driver_impl::Callbacks>::after_analysis::{{closure}}
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_smir/src/rustc_internal/mod.rs:241:13
 23: rustc_middle::ty::context::GlobalCtxt::enter::{{closure}}
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_middle/src/ty/context.rs:595:37
 24: rustc_middle::ty::context::tls::enter_context::{{closure}}
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_middle/src/ty/context/tls.rs:82:9
 25: std::thread::local::LocalKey<T>::try_with
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/std/src/thread/local.rs:270:16
 26: std::thread::local::LocalKey<T>::with
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/library/std/src/thread/local.rs:246:9
 27: rustc_middle::ty::context::tls::enter_context
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_middle/src/ty/context/tls.rs:79:9
 28: rustc_middle::ty::context::GlobalCtxt::enter
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_middle/src/ty/context.rs:595:9
 29: rustc_interface::queries::QueryResult<&rustc_middle::ty::context::GlobalCtxt>::enter
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_interface/src/queries.rs:71:9
 30: <rustc_smir::rustc_internal::StableMir<B,C> as rustc_driver_impl::Callbacks>::after_analysis
            at /rustc/e3abbd4994f72888f9e5e44dc89a4102e48c2a54/compiler/rustc_smir/src/rustc_internal/mod.rs:240:9
 31: <rustc_interface::interface::Compiler>::enter::<rustc_driver_impl::run_compiler::{closure#1}::{closure#2}, core::result::Result<core::option::Option<rustc_interface::queries::Linker>, rustc_span::ErrorGuaranteed>>
+note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[oli-obk]

Oh 🤦 i'll look into it

[celinval]

That said, I would expect CrateItems to return things that don't have body. Such as an external C function that was declared in the crate.

[oli-obk]

Maybe we should rename it then. I think we're iterating over things with MIR

[celinval]

We don't want to have both though, right? An option to get all items and an option to get all things with MIR.

[oli-obk]

Depends on how useful a general "iterate over everything" system would be. We do have some ways to iterate over specific groups of items (e.g. over all impls, all traits, ...).

If we iterate over all DefIds and then expose ways to check what kind of item it is, that may not be the ideal API for many use cases. It may also be, idk 😆 Basically if every user of the "iterate over all def ids" API would just filter for one or two elements, I believe it would be the wrong API. But if generally many items get processed, then it would be a good API.

[celinval]

I wonder if we should rename CrateItems then to be something more narrow. I do like the idea of having narrower APIs.

[oli-obk]

ItemWithBody, BodyItem, MirItem, ...?

Agreed that CrateItem is a bad name and should be replaced

[ouz-a]

BodyItem sounds descriptive enough imo

[celinval]

I kinda prefer MirItem.

That said, I am planning on creating a trait for things that have a DefId, so it's easy to retrieve information such as name, and crate. Any objection?

[celinval]

BTW, I think it would be nice to clarify what CrateItem even means. Do we want it to be any item that has a body or is it meant to represent only local items that have a body?

No matter what we decide, I suggest that we add a constructor to be used in other places in our crate that ensures this item invariants hold. Something like:

pub(crate) try_new(def_id: DefId) -> Result<Self> { /* todo */}