Design: Symbolic Macros

[brandjon]

Symbolic Macros are a proposed new feature to make macros easier to write and less brittle. I wrote a 2-page* doc summarizing their main properties. A formal design review will come in the future, after we are further along in the prototype.

* (If you don't count the appendix)

[alexeagle]

Feature request: I often use a macro to permit users to supply an "attribute" that accepts multiple types. The polymorphism makes a nicer developer experience: for example you can provide a label of a file, or give a list of strings or a dictionary that the macro automatically serializes with a write_file so the underlying rule only accepts a label of a file.
Would be nice if macro#attrs could express more than one attr.(type)

[brandjon]

It sounds like a nice convenience but I worry that it's scope creep, and that the feature can be justified for both macros and rules and therefore considered independently of this proposal.

Absent this feature, you should be able to achieve the same thing less conveniently by using optional attributes (passing default=...) and having the macro logic decide which attr to use based on sentinel values.

[comius]

May I propose that we consider such a feature in the design, but not in the initial implementation. That is, a possible future extension, which is not prevented by the first design/implementation we roll out. If this is possible. This way it wouldn't be a such a big scope creep.

[katre]

Side comment: can we get selectable = True/False in attr for regular rules? I've had several requests for this from rule authors recently (although I keep asking them to file feature requests and none do, so maybe it's not that necessary).

[brandjon]

I'd actually like that for consistency between attr descriptors used in rules and those used in symbolic macros. It's a little tricky because I can't imagine we'd make it a mandatory param for rule attrs (think of all the rule definitions that would need updating), but it'd be nice to make it mandatory for macros.

For rules, I can also imagine it affecting whether the attr is visible to certain types of Starlark-defined transitions that care about whether an attr is selectable.

[comius]

One thing that Jon told me once is that we should point the users in the right direction through APIs. Ideally in my opinion the right direction is that all label/dependency attributes are selectable. That would mean the API should be attr.label(legacy_unselectable=True).

I feel a bit different about for example bool types. It's more extraordinary they are selectable, so thus attr.bool(selectable = True) would signify the direction

[dgp1130]

This might be a scope increase / different problem, but one challenge I've seen is that macros as they exist today always "leak" their internal state. For example:

def my_binary(name):
    native.genrule(
        name = "%s_genrule" % name,
        outs = ["%s.sh" % name],
        # ...
    )

    native.sh_binary(
        name = name,
        srcs = ["%s.sh" % name],
        deps = ["//some:deps"],
    )

my_binary(name = "bin")

sh_binary(
    name = "other_bin",
    srcs = ["bin.sh"],
    deps = ["//other:deps"],
)

In this example, a user is calling my_binary, but then generating a completely different sh_binary using the generated .sh file, something which was likely intended to be a private implementation detail in the macro.

Personally I have a habit of accepting visibility into all my macros and only setting it on the "public" output targets. However that doesn't stop targets from within the same package depending on internals of a macro. I wonder if there's appetite for making symbolic macros actually restrict visibility for internal targets? This would require some mechanism with which to declare exactly which targets are the public outputs and I'm not 100% on what that looks like. Maybe you can specify visibility manually, but when not given any target defaults to "macro private" visibility, which is even more restrictive than the typical //visibility:private.

Related concern, what about passing through testonly, compatible_with, and restricted_to? Macros often fail to pass through these particular attributes to submacros and targets, which frequently lead to bugs. I wonder if we can do something similar there? In this case it might be an even simpler approach of just setting the default value of these attributes in targets to the given value of the symbolic macro invocation. Based on your example in the doc:

# //pkg/BUILD
load(“//lib:my_macros.bzl”, “my_validated_binary”)

my_validated_binary(
    name = "foo",
    testonly = True, # Can this implicitly mark `:main` and `:check` targets `testonly`?
)

If a submacro target is explicitly given a testonly, compatible_with, or restricted_to value, then that should override this default. For example:

def _my_validated_binary_impl(name, srcs, deps, create_test, test_runner):
    my_binary(
        name = "main",
        # No `testonly` set, defaults to `my_validated_binary`'s `testonly` value.
    )
    my_validation_test(
        name = "check",
        testonly = True, # Always `testonly = True`.
    )

Feel free to call this out of scope or not the problem at hand, but just a few thoughts of how we can solve some related macro problems with this approach.

[dgp1130]

This might be too specific at this stage of feedback but I'll point out that symbolic macro implementation function in the example _impl puts arguments in a specific order as defined in the macro() dictionary which seems very brittle.

my_validated_binary = macro(
    # Syntax aligns with `rule`, makes sense.
    attrs = {
        "srcs": attr.label_list(selectable=True),
        "deps": attr.label_list(selectable=True),
        "create_test": attr.bool(selectable=False),
        "test_runner": attr.label(
            selectable=False, default="//lib:default_runner"),
    },
    # ...
)

# How is this order defined? Is it the source order of the `attrs` dictionary? What about when using tricks like
# `dict.update` or `**` expansion?
def _impl(name, srcs, deps, create_test, test_runner):
    # ...

Similar, what happens when I define a keyword value with a default?

def _impl(name, srcs, deps, create_test, test_runner = Label("//lib:default_runner")):
    # ...

That could be an error, but I think the fact that there's two reasonable-looking approaches to setting a default is inherently a design smell.

Since we're trying to follow existing rule conventions, I feel like we'll want some kind of loading_ctx value to represent all the inputs, rather than a list of arguments in the implementation function.

def _impl(loading_ctx):
    my_binary(name = “main”, srcs = loading_ctx.attr.srcs, deps = loading_ctx.attr.deps)
    if loading_ctx.attr.create_test:
        if loading_ctx.attr.test_runner in BANNED_RUNNERS:
            fail(“Please avoid banned test runners”)
        my_validation_test(name = “check”, srcs = loading_ctx.attr.srcs, deps = loading_ctx.attr.deps,
                           test_runner = loading_ctx.attr.test_runner)

It doesn't look great, and I do wish we could use arguments in _impl functions, but given the existing state of the world and its relationship to macro() attributes, I feel like this is closer to what the end result will actually look like?

[comius]

Macros and queries (and maybe visibility):
For the first implementation/design it's probably best to just display all the targets in queries like before.

If the visibility separates public from private targets within a macro, possible future extension is a query operator, that would only display public targets of a macro - to reduce the complexity of the results. (proposal by @murali42)

[murali42]

@dgp1130 FWIW I completely agree with macros and leaking their internal rules. There is an internal proposal around that (e.g. supporting things like //visibility:macro_internal). There are several nuances though

• Should it be visible to a specific macro instance?
• Should it be visible to all instances of that macro?
• What about friend macros (e.g. my_binary should be able to get access to all my_library generated rules)?
• If macro foo calls macro bar, should foo get access to internal rules of bar? Ideally no, but implementation could get tricky (especially if we want to keep memory usage down).

All that said... coming up with a solution for visibility could be done later?