Allow adding arguments to Rules

[illicitonion]

What type of PR is this?

Feature

What package or component does this PR mostly affect?

rule

What does this PR do? Why is it needed?

Allow adding arguments to Rules

This allows supporting patterns like maybe, which take a
rule-to-instantiate as a positional argument.

[achew22]

I think the code looks good, but I'm curious about where this would be used? Is it exclusively being used for the maybe macro? It is the only rule or macro in the entire ecosystem that I can think of that has an argument you don't explicitly invoke with kwargs.

One of the goals in my mind (I'm just some guy, so huge grain of salt) is that I want gazelle to generate "what I would have written." When combined with the fact that BUILD files are read a lot more than they are written, unnamed arguments in rules feels like it's going to impede understanding and clarity. Is this a situation where we should add special logic to make it only work for the maybe macro instead of everywhere? I could easily imagine a world where someone writes rules_foo and you just have to remember that the parameter ordering is name, srcs, deps, tags, testonly, visibility and that would be hard to interpret without looking up every time.

[illicitonion]

I'm curious about where this would be used

Internally, we have a number of maybe-style macros which are used effectively as decorators; things like:

requires_network(
    java_test,
    name = "FooTest",
    ...
)

These end up following the maybe-like pattern of calling the first argument with some modification of the kwargs passed. In the above example, we set up some exec_properties and tags to control where the test may run when executing remotely, and whether it may be cached.

I agree that in general positional args aren't very clear, but for this particular pattern it seems pretty reasonable (and our users are already familiar with maybe :)).

[achew22]

Out of curiosity, why is this superior to making a java_test_requires_networking macro, or making a java_test macro that has a requires_networking = True attribute? That's an extremely common pattern that I've seen through my codebases.

[illicitonion]

Mostly orthogonality - the same reason http_archive doesn't have a maybe_http_archive or an unless_already_exists attribute. Aside from the dozens of off the shelf per-language test rules we'd need to create wrappers for (and possibly force to be used across many repositories owned by different teams), we have several individual teams who write their own test rules. Having to create so many macros, and duplicate the logic across all of them (or have to remember to call some library in each of them), is a lot more work, and makes it more likely that it gets missed for some test rule.

This is particularly bad where different teams write different rules; for instance, consider a team which maintains an execution environment - if they need to change how they tell the execution environment details (e.g. a different platform property), knowing that there's one place this is done, rather than needing to audit whether a bunch of rules are doing the right thing, simplifies things a lot.