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

Use type aliases for Query item types in Query and QueryState methods #4297

Closed

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Mar 22, 2022

Objective

  • The Query and QueryState methods have a large number of <<Q as WorldQuery>::ReadOnlyFetch as Fetch<'w, 's>>::Item return types
  • The unaliased types are mysterious and hard to read, especially for beginners

Solution

  • Use the QueryItem type alias and a parallel ReadOnlyQueryItem alias to make this code easier to read and write.

Future Work

I think we should consider refactoring QueryIter to have an Item associated type and use these aliases there too. I'd prefer to keep this minimal though, so we can merge it ASAP and make #3333 easier.

This is obviously not true given the type signature, contradicts the safety comment and it's not clear how it could be made true.
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 22, 2022
@@ -467,14 +467,12 @@ where
/// Runs `func` on each query result for the given [`World`]. This is faster than the equivalent
/// iter() method, but cannot be chained like a normal [`Iterator`].
///
/// This can only be called for read-only queries.
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment was blatantly false, and seems to just be a mistake. It contradicts the safety comment, and doesn't seem to make sense with the API.

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 22, 2022

Does rustdoc show the type aliases or do they get normalized?

@alice-i-cecile
Copy link
Member Author

This was a serious barrier to productivity and correctness in #3333.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 22, 2022
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Mar 22, 2022

Does rustdoc show the type aliases or do they get normalized?

Normalized, unfortunately: rust-lang/rust#15823

image

@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 22, 2022

This code exposes a large number of tricky lifetimes, which are prone to soundness problems

These lifetimes are still exposed and are still just as prone to soundness problems 🤷‍♀️ this seems like a nice win for readability but I don't think it really has anything to do with soundness or correctness of the ecs.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Apr 11, 2022
@alice-i-cecile
Copy link
Member Author

Closing this out; not very useful until the rust bug is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants