amethyst
Let's talk about the _system suffix in #[system]
This issue is to discuss the problematic nature of relying on compile-time generated code that's user-facing insofar as they are required to invoke that code to use Legion for the most basic of purposes for which it was designed - i.e. something as natural as creating a system.
Please see the problem on the image below.
The solution to this problem is:
The #[system] macro will create a system and append _system to the function. So instead of referencing your fn, you want to reference the actual system.
I'm looking for clarification, motivation and hopefully justifiable reasoning behind this decision to require users to know about a magic function name, that is, the _system suffix onto their declared function name.
This magic symbol makes it impossible for users to surmise how to use Legion without resorting to online documentation or code examples - this is a big red flag that marks a poor design decision. If a core part of the API is based on underhanded knowledge (however thoroughly documented that knowledge may be externally to the code), you're creating a knowledge barrier that new users can't get past on their own. This creates a demotivating atmosphere and limits API discoverability and ease-of-use, as the user will hit an unsolvable barrier literally within minutes of starting to use Legion for which they will have to resort to online help.
If users cannot possibly use an API without requiring online documentation or code examples, that's a poorly designed API. It may be difficult to progress without these aids but it certainly should not be impossible by intentional design. I'm raising my concerns here because this, to me, is a very large red flag that Legion will - however slowly and gradually - move in that direction. It only takes so many instances of "not a big deal" for things to add up to something larger, and this should be nipped in the bud before it takes root.
If you require two named user-facing symbols for every system, make the user declare both names. It will be immediately obvious to the user that the second name exists and serves a purpose, and they will learn to use it or at the very least look it up with intention. This can easily be done in the #[system] macro, for example. If the counter-argument to this, as one person suggested, is to avoid boilerplate code then I dare say compile a list of pros/cons and see which comes out worse (and do add a splash of foresight to that list).
Brief list of problem factors with generated _system suffix off the top of my head to summarize:
- User is unaware of new (or mutated) symbol name (poor API design)
- User already has a symbol of that name in their code base (confused compiler errors, rename system to less appropriate name vs. refactor existing code)
- User declares their systems with
_systemsuffix and now gets generatedvisual_system_system - Developer (same or new) down the line is required (for whatever imaginable reason) to change suffix, breaks all backwards compatibility
None of these problems would exist if...
- Two symbol names are not required for each system, or
- The user is required to specify both symbol names and thus maintains control over them
Both of which are possible solutions without negative side-effects to replace a design decision that comes with an otherwise growing grocery list of potential problems.
A user who does not look at the documentation or examples wouldn't even know about #[system] in the first place. If all that they knew was that it exists, but not how to use it (which is the level of knowledge you are assuming here), then they would still not know how to specify the system constructor identifier without reading the docs.
A good API design is not one where the user never needs to ever read any documentation or look at any examples. No API meets such a standard. Even hello_world.rs fails to pass such a goal. What is far more important is that the API behaves in a consistent manor such that once the user has learned how to do something, their solution can also be applied to other similar situations. A user not knowing what to do in a novel situation without referring to docs or examples is not nearly as important as avoiding frustration in cases where the user thinks that they know what to do but actually do not...
If a user writes a function and then calls their function, they expect the function to have the signature as they had declared it. That is how functions have always behaved in every programming language most people have ever used, Rust included. If #[system] re-wrote their function under the hood, it leaves the user in a situation where they can see their code appearing to do one thing, while the compiler insists it is actually doing something else entirely. That is most certainly not going to meet expectations.
Someone might not be able to guess that #[system] generates new code, but that is far less surprising than finding that it rewrites their code while leaving the source code they are reading unmodified. Generating additional code is what most macros in Rust do; it is the less surprising option compared to modifying the user's code.
Additionally, you actually can manually specify the identifier for the system generator. However, due to how the attribute parsing code in syn works, this option does not function correctly as it is mutually exclusive with the system type flag. Fixing this will require custom parsing code. Rather than delay release, I just left this undocumented until it is fixed, as fixing this would be a non-breaking change.
I would have preferred to use the fn identifier itself (i.e. .add_system(print_test)), or print_test.system() (as Bevy does), but this is not possible. We need to generate unique code for each system, but it is not possible to write an impl block for a function (only all functions with a specific signature).
We could probably do it on nightly with #![feature(fn_traits)], by generating a zero-sized struct with the same name as the original function, which impls the wrapped function, a .system() generator function, and any appropriate fn traits to delegate to the wrapped function. This still might be a bit too much magic, and I'm not sure the result is sufficiently transparent. I am also not sure how this might play out with generics. I want legion to be entirely usable on stable, however, so this is not an option right now.
Oh! this is my post.
I agree with @TomGillen for the most part about this.
I think the best resolve for now would stating such things more clearly / having a migration guide.
After rereading the docs I saw this line:
By default, the system macro will create a new function named <attributed_fn_name>_system
And to be honest, I think this is sufficient for the most part, I just have a bad skim through practice.
An additional comment in the hello world example, to point this out again may be helpful enough.
#[system]
fn hello_world() {
println!("hello world");
}
Schedule::builder()
.add_system(hello_world_system()) // <- notice the '_system()'?
.build();
Edit: But I see your points too, the ecosystem wants to open up for more people and this means having a better documentation besides the docs is a must. Something like a 'Legion Book' would be nice to have, maybe we as a community can work something like that out?
I want to clarify that I didn't say that users shouldn't be expected to ever look at examples or documentation. I said insofar as it's possible, the purpose of the API design should be to prevent it. The _system suffix goes against that notion.
I certainly agree with keeping Legion on stable rust and I appreciate the in-depth reply. It's convincing that this decision wasn't made on a whim, which is encouraging.
I would be satisfied with a direction where Legion 0.3 launches with its current #[system] macro and in the near future after that release introduces either a workable solution (in line with what you've suggested) or a macro "overload" that allows the user to be explicit about the name, so as to allow the freedom to opt out of the automated function suffix. A follow-up suggestion to this would then be that all official Legion examples use that explicitly named approach and that any documentation encourages it but offers #[system] as a quality-of-life productivity hack for those users who are more inclined towards that (and perhaps less pedantic).
I've voiced my concerns about this on other forums as well and can confidently claim that this is already a "significant" problem with Legion for new users. I don't think new users eventually realising and learning that this is a thing is justification enough to not prevent the problem happening in the first place.
You can already set the name explicitly with #[system(ctor = "constructor_name")]. But this doesn't seem to be possible with for_each and par_for_each systems.
In my opinion, the way Bevy does this is a lot cleaner. There is no macro magic, and to turn a function into a system you simply call .system() on it. But if this is impossible for Legion, maybe specifying the name should be mandatory?
