Skip to content
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

Diagnostics for label traits #17441

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SpecificProtagonist
Copy link
Contributor

Objective

Diagnostics for labels don't suggest how to best implement them.

error[E0277]: the trait bound `Label: ScheduleLabel` is not satisfied
   --> src/main.rs:15:35
    |
15  |     let mut sched = Schedule::new(Label);
    |                     ------------- ^^^^^ the trait `ScheduleLabel` is not implemented for `Label`
    |                     |
    |                     required by a bound introduced by this call
    |
    = help: the trait `ScheduleLabel` is implemented for `Interned<(dyn ScheduleLabel + 'static)>`
note: required by a bound in `bevy_ecs::schedule::Schedule::new`
   --> /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/schedule/schedule.rs:297:28
    |
297 |     pub fn new(label: impl ScheduleLabel) -> Self {
    |                            ^^^^^^^^^^^^^ required by this bound in `Schedule::new`

Solution

diagnostics::on_unimplemented and diagnostics::do_not_recommend

Showcase

New error message:

error[E0277]: the trait bound `Label: ScheduleLabel` is not satisfied
   --> src/main.rs:15:35
    |
15  |     let mut sched = Schedule::new(Label);
    |                     ------------- ^^^^^ the trait `ScheduleLabel` is not implemented for `Label`
    |                     |
    |                     required by a bound introduced by this call
    |
    = note: consider annotating `Label` with `#[derive(ScheduleLabel)]`
note: required by a bound in `bevy_ecs::schedule::Schedule::new`
   --> /home/vj/workspace/rust/bevy/crates/bevy_ecs/src/schedule/schedule.rs:297:28
    |
297 |     pub fn new(label: impl ScheduleLabel) -> Self {
    |                            ^^^^^^^^^^^^^ required by this bound in `Schedule::new`

@SpecificProtagonist SpecificProtagonist added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 19, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 19, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jan 19, 2025
@alice-i-cecile
Copy link
Member

Looks like CI is mad about the attribute. What's different vs the existing usages?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 19, 2025
@SpecificProtagonist
Copy link
Contributor Author

SpecificProtagonist commented Jan 19, 2025

@LikeLakers2 Would you happen to know what the correct formulation for the expect & allow attribute here is?

I though I found a workaround by putting the trait impl into a module inside a const _: () = {…, but that doesn't work for doctests because super accesses the parent module, not parent scope.

@LikeLakers2
Copy link
Contributor

LikeLakers2 commented Jan 20, 2025

@SpecificProtagonist I'm not actually sure what the solution for this would be.

See, those lint attributes should be applying, but rustc clearly isn't applying them as we expect.

Normally, my next step would be to move the lint attributes up a level. For example, if I have a module, and inside that module is a struct that I'm trying to place those lint attributes on... then I move those lint attributes to apply to the entire module.

Only problem is, there's not exactly an encapsulating item here... So you'd probably have to make an encapsulating item here.

But you seem to have tried that - and I don't know how to fix it otherwise without causing some churn. If you're okay with some churn, here's a couple ideas:

  • You could put the #[diagnostic::do_not_recommend] behind a cfg_attr that will only include it on nightly... But then you'd have to come back and change it once it's stable.

  • You could also modify the root Cargo.toml to include lints.rust.unknown_or_malformed_diagnostic_attributes = "allow", but I don't know how the maintainers would feel about that.

Otherwise, I might suggest giving up on this until #[diagnostic::do_not_recommend] is stable.

@SpecificProtagonist
Copy link
Contributor Author

Thanks! I'm going with the last option of removing the do_not_recommend for now and adding it back when stable.

@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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

3 participants