Skip to content

Add Lints disallow_fixed_update and disallow_update #463

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 20 commits into from
Jul 8, 2025

Conversation

DaAlbrecht
Copy link
Collaborator

closes #99

@DaAlbrecht DaAlbrecht added A-Linter Related to the linter and custom lints D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 29, 2025
@DaAlbrecht DaAlbrecht force-pushed the 99-lint-fixed-or-update branch from 14797b1 to d998a34 Compare May 29, 2025 09:27
@DaAlbrecht DaAlbrecht marked this pull request as ready for review May 29, 2025 10:33
@DaAlbrecht DaAlbrecht added the S-Needs-Review The PR needs to be reviewed before it can be merged label May 29, 2025
@BD103 BD103 moved this to This Week in BD103 Work Planning Jun 26, 2025
@BD103 BD103 moved this from This Week to This Month in BD103 Work Planning Jun 26, 2025
@BD103 BD103 removed the status in BD103 Work Planning Jun 26, 2025
return;
};

let schedule_ty = cx.typeck_results().expr_ty(schedule_label);
Copy link
Member

Choose a reason for hiding this comment

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

Here's a "fun" thing I just realized, the following code isn't caught by the linter:

#![deny(bevy::disallow_update)]

fn update() -> impl ScheduleLabel {
    Update
}

fn main() {
    // Adding system to `Update` schedule, but no error is emitted.
    App::new().add_systems(update(), my_system);
}

impl ScheduleLabel is seen as an TyKind::Alias, which isn't caught in match_type(). We need to resolve the type again in order to get the final, concrete type:

if let TyKind::Alias(_, alias) = schedule_ty.kind() {
    schedule_ty = cx.tcx.type_of(alias.def_id);
}

This isn't something you need to handle now, but we may need to eventually check for it in every single lint (oof). I'd like to wait until after #456 before messing with type path matching more, though.

Copy link
Collaborator Author

@DaAlbrecht DaAlbrecht Jun 28, 2025

Choose a reason for hiding this comment

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

hahah nice catch! whoever writes update() is just evil ^^

@BD103 BD103 moved this to In Progress in BD103 Work Planning Jun 26, 2025
@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review The PR needs to be reviewed before it can be merged labels Jun 27, 2025
@DaAlbrecht DaAlbrecht force-pushed the 99-lint-fixed-or-update branch from be870e5 to c9e437a Compare June 28, 2025 14:30
@BD103 BD103 added this to the `bevy_lint` v0.4.0 milestone Jul 3, 2025
@BD103 BD103 added the C-Feature Make something new possible label Jul 5, 2025
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

It should be good to go now!

@BD103 BD103 enabled auto-merge (squash) July 7, 2025 23:10
@BD103 BD103 removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jul 7, 2025
@DaAlbrecht
Copy link
Collaborator Author

It should be good to go now!

😢 it was not. Missing ignore for the doctests (or importing bevy::prelude::*). Will fix when im home if you dont beat me to it

@BD103
Copy link
Member

BD103 commented Jul 8, 2025

It should be good to go now!

😢 it was not. Missing ignore for the doctests (or importing bevy::prelude::*). Will fix when im home if you dont beat me to it

I might have beaten you to it >:D

@BD103 BD103 merged commit 2eb204a into main Jul 8, 2025
10 checks passed
@BD103 BD103 deleted the 99-lint-fixed-or-update branch July 8, 2025 13:10
@github-project-automation github-project-automation bot moved this from In Progress to Done! in BD103 Work Planning Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints C-Feature Make something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint suggestion: deny Update or FixedUpdate
2 participants