Skip to content

Conversation

@ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Nov 23, 2025

Objective

Allow functions accepting StaticSystemInput as input to be used as systems accepting the inner type.

Solution

  • Add FromInput trait mirroring IntoResult

Testing

  • Compiles
  • Added test to input.rs

Showcase

Click to view showcase
    #[test]
    fn compatible_input() {
        fn takes_usize(In(a): In<usize>) -> usize {
            a
        }

        fn takes_static_usize(StaticSystemInput(In(b)): StaticSystemInput<In<usize>>) -> usize {
            b
        }

        assert_is_system::<In<usize>, usize, _>(takes_usize);
        // test if StaticSystemInput is compatible with its inner type
        assert_is_system::<In<usize>, usize, _>(takes_static_usize);
    }

Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

Note that #21916 and cBournhonesque#4 are alternative approaches to the same goal. I'm approving because this is a solid implementation of this approach, but I personally prefer #21916 because the change is smaller, and because associated types are less likely to cause type inference failures.

For example, world.run_system_cached_with(|_: StaticSystemInput<In<usize>>| {}, 0) would fail with "type annotations needed" under this PR, since I can be either In<usize> or StaticSystemInput<In<usize>>, while under #21916 it would unambiguously be In<usize>.

I don't know of any realistic use cases that would run into that problem, though. The only reason to use StaticSystemInput is if you're generic in the input type, like pipe, and in that case the types will all be written out anyway.

@ecoskey ecoskey added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 24, 2025
@ecoskey
Copy link
Contributor Author

ecoskey commented Nov 24, 2025

Note that #21916 and cBournhonesque#4 are alternative approaches to the same goal. I'm approving because this is a solid implementation of this approach, but I personally prefer #21916 because the change is smaller, and because associated types are less likely to cause type inference failures.

Valid! I still think I like this approach since it's a bit more symmetrical and doesn't add a new associated type, but I agree there could be problems with inference. I don't think there will be much more than present though, since IntoResult already leads to some ambiguities (like with run_system_cached) and in those cases the types are almost always specified, like you said.

Copy link
Contributor

@ItsDoot ItsDoot left a comment

Choose a reason for hiding this comment

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

As long as we can't prove further type inference issues in general usecases, I'd be fine with this actually. Looks good besides the one thing.

@ItsDoot ItsDoot 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 Nov 28, 2025
@ecoskey
Copy link
Contributor Author

ecoskey commented Nov 28, 2025

Note for final review: compare with #21916 as an alternative. They're pretty similar but have a few differences with type inference and such

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-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes 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