tag_classes don't support optional attrs

illicitonion commented 9 months ago

Description of the feature request:

In migrating rules_rust's third-party resolver support to bzlmod we've run into an issue due to tag_class arguments being more typed than WORKSPACE functions were.

We want to support a structure along the lines of (WORKSPACE):

crates_repostory(
    name = "crates",
    cargo_lockfile = "//third-party:Cargo.lock",
    manifests = ["//third-party:Cargo.toml"],
    default_generate_build_scripts = True,
    annotations = {
        "some-crate": [
            crate.annotation(generate_build_scripts = False),
        ],
    }
)

where crate.annotation is just a function with optional arguments which serializes its args as JSON (https://github.com/bazelbuild/rules_rust/blob/40232a07f11376c737b35bbfac89773880d09048/crate_universe/private/crate.bzl#L84-L212)

This allowed setting a default boolean to be used for all crates, and overriding it for specific crates.

In bzlmod, we're using a tag_class for these annotations, along the lines of:

crate.from_cargo(
    name = "crates",
    cargo_lockfile = "//third-party:Cargo.lock",
    manifests = ["//third-party:Cargo.toml"],
    default_generate_build_scripts = True,
)

crate.annotation(
    crate = "some-crate",
    generate_build_scripts = False,
)

However, tag_classes take fully typed arguments (which is generally good) and don't allow for None values, so there doesn't appear to be a way for us to say "generate_build_scripts is an optional argument which may be None in which case we should fall back to the repo's default value". Because we need to set a non-None default for crate.annotation's generate_build_scripts argument which means it will always have a value and never fall back to the repo's default.

As a workaround, we can use a string argument here (and perhaps copy rules_go's convention of using strings which are "on"/"off"/"auto" and default to "auto", rather than using a None-able boolean), but this feels kind of messy and hacky.

Which category does this issue belong to?

External Dependency, Rules API

What underlying problem are you trying to solve with this feature?

Allowing a bzlmod repo to set a default value for all of the third-party libraries which it generates rules, which may optionally be overridden in for some subset of those libraries.

Which operating system are you running Bazel on?

No response

What is the output of `bazel info release`?

No response

If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.

No response

What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

illicitonion commented 9 months ago

cc @Wyverald @meteorcloudy

matts1 commented 9 months ago

I think a nice solution here would be to allow adding default = None for any attr.* type. This is what I originally tried to do in the PR that @illicitonion mentioned, but it wouldn't allow it because none didn't match the type.

This should allow us to achieve this without breaking existing usages, or adding a new parameter, and would work not just for booleans, but for all types.

Wyverald commented 9 months ago

This seems to be an oversight -- attributes should be "optional" by default, and mandatory only when mandatory=True is set. But we're really swamped, so if someone wants to try for a PR, we'd appreciate the help :)

Silic0nS0ldier commented 1 month ago

This seems to be an oversight -- attributes should be "optional" by default, and mandatory only when mandatory=True is set.

From my own testing, optional attributes work. The OP is asking for None to be an allowed output for attributes (anything defined with the attr module).

The title should probably be updated to something along the lines of "Allow None default value in rule, repository_rule and tag_class.

Right now if None is given to a rule, the default value (if any) will be applied. A property which is useful when wrapping rules with a macro. None results in an error with tag_classes, however since functions can't be used in a MODULE.bazel file this isn't an right now issue.

I like @matts1's idea of using default = None. The utility goes beyond the scope of this feature request, rules and repository rules are sometimes wrapped with a function to work around the inability to natively preserve None;

attr.bool defaults to False
attr.int defaults to 0
attr.int_list defaults to []
attr.label_keyed_string_dict defaults to {}
attr.label_list defaults to []
attr.output defaults to... No clue. Docs do not say.
attr.output_list, same as attr.output.
attr.string defaults to ""
attr.string_dict defaults to {}
attr.string_list defaults to {}
attr.string_list_dict defaults to {}

None of the existing defaults can communicate an "unset" state without a workaround of treating a particular value as being special.

bazelbuild / bazel