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:
Finds all local trait impl blocks within a specific crate.
Filters them for impl Reflect for T blocks.
Looks up the Node 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(). This method requires that T be defined within the local crate (see LocalDefId).
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():
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.
Fixes #148.
The
missing_reflect
lint crashed when checkingbevy_reflect
due to incorrect assumptions about the orphan rule.Explanation
When searching for types that implement certain traits,
Reflect
in this case,missing_reflect
:impl
blocks within a specific crate.impl Reflect for T
blocks.Node
forT
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
forT
, the lint pass callsTyCtxt::hir_node()
. This method requires thatT
be defined within the local crate (seeLocalDefId
).I assumed incorrectly that this would always be the case. I assumed that
Reflect
would always be external, forcingT
to be local to comply with Rust's orphan rule. This is why I thought I could safely callDefId::expect_local()
:https://github.com/TheBevyFlock/bevy_cli/blob/e520db3fa4ea64760cd458cdb7e8ab0fe0364cf0/bevy_lint/src/lints/missing_reflect.rs#L185-L189
This comment was foreshadowing for this bug. When I ran
bevy_lint
on thebevy_reflect
crate, which definesReflect
,T
was no longer required to be a type declared in the local crate. (As #148 hints, where it implementsReflect
forString
.)This was the source of the bug. The lint cannot call
expect_local()
when run onbevy_reflect
, since it is not actually required to be local.Solution
If the
DefId
cannot be converted into aLocalDefId
, return early and do not lint thatimpl
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 implementComponent
orResource
anyways.