Skip to content

Commit

Permalink
Fix ICE when checking bevy_reflect (#149)
Browse files Browse the repository at this point in the history
Fixes #148.

The `missing_reflect` lint crashed when checking `bevy_reflect` due to
incorrect assumptions about the orphan rule.

# Explanation

When searching for types that implement certain traits, `Reflect` in
this case, `missing_reflect`:

1. Finds all local trait `impl` blocks within a specific crate.
2. Filters them for `impl Reflect for T` blocks.
4. Looks up the
[`Node`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/hir/enum.Node.html)
for `T` in the local crate, returning it in an iterator.

While that is a greatly simplified version of what
`TraitType::from_local_crate()` does, it conveys enough to understand
where things went wrong.

In order to find the final `Node` for `T`, the lint pass calls
[`TyCtxt::hir_node()`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.hir_node).
This method **requires** that `T` be defined within the local crate (see
[`LocalDefId`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def_id/struct.LocalDefId.html)).

I assumed incorrectly that this would always be the case. I assumed that
`Reflect` would _always_ be external, forcing `T` to be local to comply
with Rust's orphan rule. This is why I thought I could safely call
`DefId::expect_local()`:


https://github.com/TheBevyFlock/bevy_cli/blob/e520db3fa4ea64760cd458cdb7e8ab0fe0364cf0/bevy_lint/src/lints/missing_reflect.rs#L185-L189

> The only case this may not be upheld is within Bevy's own crates.

This comment was foreshadowing for this bug. When I ran `bevy_lint` on
the `bevy_reflect` crate, which defines `Reflect`, `T` was no longer
required to be a type declared in the local crate. (As #148 hints, where
it implements `Reflect` for `String`.)

This was the source of the bug. The lint cannot call `expect_local()`
when run on `bevy_reflect`, since it is not actually required to be
local.

# Solution

If the `DefId` cannot be converted into a `LocalDefId`, return early and
do not lint that `impl` block. It's as simple as that :)

This means that standard library types like `String` will not be linted,
but that's fine because they shouldn't implement `Component` or
`Resource` anyways.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
  • Loading branch information
BD103 and alice-i-cecile authored Oct 16, 2024
1 parent e520db3 commit 469a992
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions bevy_lint/src/lints/missing_reflect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,18 @@ impl TraitType {
_ => return None,
};

// Find the `HirId` from the `DefId`. This is like a `DefId`, but with further
// Tries to convert the `DefId` to a `LocalDefId`, exiting early if it cannot be done.
// This will only work if `T` in `impl T` is defined within the same crate.
//
// In most cases this will succeed due to Rust's orphan rule, but it notably fails
// within `bevy_reflect` itself, since that crate implements `Reflect` for `std` types
// such as `String`.
let local_def_id = def_id.as_local()?;

// Find the `HirId` from the `LocalDefId`. This is like a `DefId`, but with further
// constraints on what it can represent.
let hir_id = OwnerId {
// This is guaranteed to be a `LocalDefId` due to Rust's orphan rule. The traits
// (`Reflect`, `Component`, etc.) are from an external crate, so the type
// definition _must_ be local. The only case this may not be upheld is within
// Bevy's own crates.
def_id: def_id.expect_local(),
def_id: local_def_id,
}
.into();

Expand Down

0 comments on commit 469a992

Please sign in to comment.