Skip to content

Fully qualify crate paths in BevyManifest #18938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

jnhyatt
Copy link
Contributor

@jnhyatt jnhyatt commented Apr 25, 2025

Objective

Subtle, very minor issue. The following code fails to compile on main:

struct bevy;
#[derive(::bevy::ecs::component::Component)]
struct MyComponent;

The derive proc macro is pasting in essentially:

impl bevy::ecs::component::Component for MyComponent

...which normally works, but I've added struct bevy, which makes the proc macro spit out incorrect code.

Very cursed, but to my knowledge has never been encountered in practice. All the same, it's technically incorrect and should be fixed.

Solution

The solution is simply to prepend :: to crate names. Specifically, all (all?) Bevy's derive macros determine the root crate name using BevyManifest, which does some toml-parsing witchcraft to figure out whether to qualify names using the umbrella bevy crate or individual bevy_xxx crates. I just added a :: to the spot where we parse the syn::Path. The above example compiles properly after that.

Testing

  • CI should catch any errors since this change should cause compile errors if for some reason they're being used in a cursed way somewhere that would make this break something.

Note

If this does break something for someone, this really needs a comment in BevyManifest::maybe_get_path explaining why we can't make this change.

Copy link
Contributor

@SpecificProtagonist SpecificProtagonist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found a way to break this (apart from renaming crates of course, but that's broken already anyways).

@SpecificProtagonist SpecificProtagonist added C-Bug An unexpected or incorrect behavior A-Utils Utility functions and types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 26, 2025
Copy link

@dj-blume9 dj-blume9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have anything breaking for me with this change. Looks good.

@SpecificProtagonist SpecificProtagonist added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 28, 2025
@mockersf mockersf added this pull request to the merge queue Apr 28, 2025
Merged via the queue into bevyengine:main with commit 7f9eae2 Apr 28, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants