Not all attributes are present in module tags with sensible default values

[shs96c]

Description of the problem / feature request:

When using bazel 5.0.0rc3, declare a module tag similar to:

mytag = tag_class(
    attrs = {
        "target": attr.label(),
    },
)

If the tag is used in MODULE.bazel file without a value (eg. mytag()), then in the module's implementation function calls to mytag.target will fail, whereas it's expected they return None, as happens with empty attributes on repository and regular rules.

[oquenchil]

@meteorcloudy

[meteorcloudy]

/cc @Wyverald Is this work as intended? I guess you can also specify a default value?

[shs96c]

This behaves differently from repository or regular rules. I sincerely hope that this isn't the intended behaviour as it's inconsistent. My expectation would be for these attributes to be set to None if they are not specified (as happens with other rule types)

[Wyverald]

This certainly isn't intended -- in fact, if you dir(tag), you'll see that the field target is supposed to be there. It's just that the code in getValue returning the default value of "null" causes the field to be considered absent. I'm trying to dig into why this returns None for normal rules.

[Wyverald]

I've located the root cause -- the fix is in flight.

[Wyverald]

Note: We're probably not cherrypicking this fix into 5.0 so please use a rolling release to test bzlmod-related stuff.

[fmeum]

Note: We're probably not cherrypicking this fix into 5.0 so please use a rolling release to test bzlmod-related stuff.

@Wyverald While I agree with the decision not to cherrypick #14447 into 5.0, I would like to speak out in favor of cherrypicking this fix. bzlmod is picking up traction in the community right now and the release of 5.0 will be when most actively maintained rulesets will start to add support for it. That means that bugs like this, where established patterns unexpectedly don't work in the new setting and have to be worked around in non-optimal ways, will lead to the creation of "legacy code" on day 1 of the availability of this new feature. That can hamper adoption as well as create anti-patterns that will stay with bzlmod for a long time due to backwards compatibility concerns.

[mattyclarkson]

I hit an IllegalArgumentException when using an attr.string_dict() with a tag_class. I created a reproducer project.

def _rimpl(rctx):
    rctx.file("hey.sh", '#!/bin/sh\nset -eu\necho "${@}"')
    rctx.file("BUILD.bazel", 'exports_files(["hey.sh"], visibility=["//visibility:public"])')

_repo = repository_rule(implementation = _rimpl)

tag = tag_class(
  attrs = {
    "version": attr.string(),
    "test": attr.string_dict(),  # this cannot be read
  },
)

def _impl(mctx):
    for module in mctx.modules:
        for tag in module.tags.test:
            print(tag.test) # 💥 this creates the error
            _repo(name="test-{}".format(tag.version))

test = module_extension(
    implementation = _impl,
    tag_classes = {
        "test": tag,
    },
)

End up hitting:

Caused by: net.starlark.java.eval.Starlark$UncheckedEvalException: IllegalArgumentException thrown during Starlark evaluation

I've been told this issue is the root cause. Hence, could we get this cherry-picked so that we can use dict/list attributes on tag_classes in the 5.0 series?

If we don't cherry-pick the patch: could we enable a fail-fast where at tag_class parsing time, any dict/list attributes bork. Hitting it at runtime (with a hard to grok message) isn't ideal.

[Wyverald]

You can use dict/list attributes on tag_classes, but you can't not specify those attributes (without the fix, that is).

[Wyverald]

re: cherrypicking, after much deliberation with Yun, we've decided to cherrypick the bzlmod-related fixes (~3 commits) into 5.0, given that these are for an explicitly experimental feature and won't affect any other component of Bazel, and are hence very low-risk, despite them not strictly being regressions.

This does mean that any breakages resulting from those fixes won't be fixed before 5.0 actually ships. If this does happen (and unrelated bugs are likely to be discovered anyhow), we'll try to fix them in a 5.1 point release.