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

bevy_lint: Add zst_query lint #168

Merged
merged 6 commits into from
Nov 10, 2024
Merged

Conversation

MrGVSV
Copy link
Contributor

@MrGVSV MrGVSV commented Nov 1, 2024

Resolves #130

@MrGVSV MrGVSV force-pushed the mrgvsv/lint/zst_query branch from 4bdc77e to 61f0d59 Compare November 1, 2024 23:01
@MrGVSV MrGVSV force-pushed the mrgvsv/lint/zst_query branch from 61f0d59 to c1e886b Compare November 2, 2024 05:36
@BD103 BD103 self-requested a review November 2, 2024 11:38
@janhohenheim
Copy link
Member

Just making sure, this still allows Query<(), With<Foo>>, right? :)

@MrGVSV
Copy link
Contributor Author

MrGVSV commented Nov 2, 2024

Just making sure, this still allows Query<(), With<Foo>>, right? :)

Good question! I just tested it and it seems like it does! I added a test for this case specifically.

bevy_lint/src/utils/hir_parse.rs Outdated Show resolved Hide resolved
bevy_lint/src/utils/hir_parse.rs Show resolved Hide resolved
return None;
};

path.segments.last()?.args().args.get(index)
Copy link
Member

Choose a reason for hiding this comment

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

Is there ever a case where there may be generics before the final path segment? If so we could do this:

Suggested change
path.segments.last()?.args().args.get(index)
path.segments.iter().flat_map(|segment| segment.args().args).nth(index)

This implementation joins all of the generics in a path together, then finds the GenericArg at the index. This may have undesired effects, though I haven't tested it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah there could be like Foo::<i32>::baz::<usize> for a function signature type or something. So it could be useful. I don't think it would break things for our purposes here since Query doesn't look like that haha.

I'll try it out!

Copy link
Contributor Author

@MrGVSV MrGVSV Nov 10, 2024

Choose a reason for hiding this comment

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

On second thought, this might break in cases like:

trait Queryable {
    type Query<'w, 's, D: QueryData + 'static, F: QueryFilter + 'static>;
}

struct MyQueryable<T>(T);

impl<T> Queryable for MyQueryable<T> {
    type Query<'w, 's, D: QueryData + 'static, F: QueryFilter + 'static> = Query<'w, 's, D, F>;
}

fn system<'w, 's>(query: <MyQueryable<f32> as Queryable>::Query<'w, 's, &'static Foo, ()>) {}

Although, to be fair, I just tested this and we don't currently catch associated types like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we save adding this functionality for the future when it's needed? It might at that point even make sense to be a separate function depending on the edge cases.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we save adding this functionality for the future when it's needed?

I'm fine with that, but would you mind adding a comment to the function docs do users know that it only indexes the generics in the final path segment? (Non-blocking, though)

bevy_lint/src/utils/mod.rs Outdated Show resolved Hide resolved
bevy_lint/src/utils/hir_parse.rs Show resolved Hide resolved
bevy_lint/src/lints/zst_query.rs Show resolved Hide resolved
bevy_lint/src/lints/zst_query.rs Show resolved Hide resolved
bevy_lint/src/lints/zst_query.rs Show resolved Hide resolved
bevy_lint/tests/ui/zst_query/query.rs Show resolved Hide resolved
bevy_lint/tests/ui/zst_query/query.rs Outdated Show resolved Hide resolved
@MrGVSV MrGVSV requested a review from BD103 November 10, 2024 00:39
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.

Thank you, let's get this merged!

@BD103 BD103 merged commit 605d7a7 into TheBevyFlock:main Nov 10, 2024
8 checks passed
@BD103 BD103 added A-Linter Related to the linter and custom lints C-Feature Make something new possible labels Nov 10, 2024
This was referenced Nov 10, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Lint for when querying for zero size structs
4 participants