Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbolic macro attribute inheritance #24066

Closed
tetromino opened this issue Oct 23, 2024 · 8 comments
Closed

Symbolic macro attribute inheritance #24066

tetromino opened this issue Oct 23, 2024 · 8 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: feature request

Comments

@tetromino
Copy link
Contributor

tetromino commented Oct 23, 2024

We want to allow symbolic macros to inherit attributes from a given rule, a given symbolic macro, or the set of common attributes for Starlark rules, Starlark *_test rules, or Starlark *_binary rules. And we'd want the ability to remove or override inherited attributes.

Motivation:

  • make it easier to wrap a rule (or macro) in another macro without needing to copy-paste the list of attributes
  • if Bazel adds a new common Starlark rule attribute, many rule-wrapping macros would want to be able to use it immediately, without needing to be explicitly updated
  • replicate the common **kwargs pattern seen in legacy macros, where the attributes that the macro doesn't explicitly care about are passed unchanged down to the macro's main target.

The last point, in particular, would make it vastly easier to migrate legacy macros to symbolic.


I'm thinking of syntax along the following lines:

my_cc_library = macro(
    # inherit native.cc_library's attributes
    # alternatively: inherit_attrs = "common", "test", or "binary" to inherit the common attributes of
    # all Starlark rules, Starlark test rules, or Starlark binary rules respectively.
    inherit_attrs = native.cc_library,
    # remove cxxopts from inherited attrs list (so callers of my_cc_library cannot set it)
    no_inherit_attrs = ["cxxopts"],
    attrs = {
        # override/redefine the inherited copts attribute
        "copts": attr.string_list(...),
    },
    ...
)

inherit_attrs can take either a rule or macro symbol, or the special string constants "common", "test", or "binary" to indicate the set of attributes common to all Starlark rules, Starlark test rules, or Starlark binary rules respectively (see https://bazel.build/reference/be/common-definitions).

Specifically, the algorithm is as follows:

  • remove the special name and visibility attributes from the inherited list, since symbolic macros auto-inject them
  • remove private (prefixed with "_") attributes, since the macro cannot pass them to the wrapped rule
  • remove computed defaults and late-bound attributes, since macros cannot use them
  • remove attributes whose names are listed in no_inherit_attrs
  • combine with attrs dict, with attrs taking precedence

@brandjon @susinmotion FYI

@tetromino tetromino added type: feature request P1 I'll work on this now. (Assignee required) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob labels Oct 23, 2024
@tetromino tetromino self-assigned this Oct 23, 2024
@brandjon
Copy link
Member

Instead of no_inherit_attrs = ["cxxopts"], how about attrs = {"cxxopts": None}? This would generalize the "shadow this attr with my own" use case to also support the "don't allow this attr at all" use case.

Alternatively, don't support that at all, and require the rule author to fail if "cxxopts" is a key of **kwargs. Honestly, that seems more minimalist and safer than adding a new None value type to attrs. Do we have evidence that this use case is common? Can we afford to punt on it?

@brandjon
Copy link
Member

Note that inherit_attrs should also work for inheriting from other symbolic macros, not just rules. And with @comius's designs about parent rules, the name may also need to be applicable for rules inheriting from other rules for API consistency.

@brandjon
Copy link
Member

What would the syntax be for inheriting common attrs (of all rules, test rules, or binary rules) without inheriting the specific attrs added by any particular rule?

I'm also worried that the idea of common attrs as documented in the Build Encyclopedia might be a fiction -- it's assembled from some seemingly manually curated template content, rather than extracted from a source of truth in code. So there may in fact be many more than three particular sets of predeclared rule attrs. Also the attrs for e.g. native binary rules and starlark binary rules may be subtly different. All of this is to say that perhaps we shouldn't yet support this use case, and instead require the user to name a particular rule.

@tetromino
Copy link
Contributor Author

@brandjon -

Instead of no_inherit_attrs = ["cxxopts"], how about attrs = {"cxxopts": None}? This would generalize the "shadow this attr with my own" use case to also support the "don't allow this attr at all" use case.

For consistency, I'd prefer to keep the shape of the attrs dict compatible between macro() and rule().

Alternatively, don't support that at all, and require the rule author to fail if "cxxopts" is a key of **kwargs. Honestly, that seems more minimalist and safer than adding a new None value type to attrs. Do we have evidence that this use case is common? Can we afford to punt on it?

An internal user explicitly requested a way to hide attributes.

Marking attributes as illegal via implementation fail is bad for readability (we want a macro to be described by the macro() declararation, not by the implementation function), IDE code completion, and Stardoc-generated docs introspection.

What would the syntax be for inheriting common attrs (of all rules, test rules, or binary rules) without inheriting the specific attrs added by any particular rule?

Good question! My plan was to use special string constants - e.g. "common", "test", and "binary". Will update the design above.

I'm also worried that the idea of common attrs as documented in the Build Encyclopedia might be a fiction -- it's assembled from some seemingly manually curated template content, rather than extracted from a source of truth in code. So there may in fact be many more than three particular sets of predeclared rule attrs. Also the attrs for e.g. native binary rules and starlark binary rules may be subtly different. All of this is to say that perhaps we shouldn't yet support this use case, and instead require the user to name a particular rule.

Good question! And the answer, I think, is to explicitly state that "common", "test", and "binary" refer to common attributes of Starlark rules. Starlark rule common attributes do have a common source of truth, while native rule common attributes more-or-less coincide with them by coincidence :)

@tetromino
Copy link
Contributor Author

Instead of no_inherit_attrs = ["cxxopts"], how about attrs = {"cxxopts": None}? This would generalize the "shadow this attr with my own" use case to also support the "don't allow this attr at all" use case.

@brandjon - good idea, I agree (as discussed offline).

@tetromino
Copy link
Contributor Author

@bazel-io fork 8.0.0

bazel-io pushed a commit to bazel-io/bazel that referenced this issue Nov 11, 2024
Allows the following syntax:

```
my_cc_library = macro(
    # inherit all attributes from a given rule or macro symbol (or "common" for
    # the set of common Starlark rule attributes)
    inherit_attrs = native.cc_library,
    attrs = {
        # remove cxxopts from inherited attrs list
        "cxxopts": None,
        # override the copts attribute inherited from native.cc_library
        "copts": attr.string_list(default = ["-D_FOO"]),
    },
    ...
)
```

Fixes bazelbuild#24066

Tricky parts:
* Some common rule attributes ("testonly", "size", etc.) have computed defaults;
  native rule "license" and "distribs" attrs have defaults which fail type check if
  lifted. The only sane way of handling such attrs, if the attribute is unset in
  the macro function call, to pass the value as None to the implementation function
  so that the implementation function will pass None to the rule function, which
  will thus set the default appropriately.
* We need RuleFunctionApi and MacroFunctionApi interfaces (yet more interfaces,
  alas, or we get a circular dependency) to express the type of inherit_attrs param
* Iterating over all native rules (for testing inheritance from them) is tricky: we
  need to look at builtins (since the ConfiguredRuleClassProvider may have stubs for
  java rules overridden by builtins). This is already correctly handled by
  `bazel info build-language`, so let's move the logic into a utility class.

RELNOTES: Add inherit_attrs param to macro() to allow symbolic macros to
inherit attributes from rules or other symbolic macros.
PiperOrigin-RevId: 694154352
Change-Id: I849a7f16b4da8eb2829cdbc6a131d85a28bc4740
github-merge-queue bot pushed a commit that referenced this issue Nov 11, 2024
Allows the following syntax:

```
my_cc_library = macro(
    # inherit all attributes from a given rule or macro symbol (or "common" for
    # the set of common Starlark rule attributes)
    inherit_attrs = native.cc_library,
    attrs = {
        # remove cxxopts from inherited attrs list
        "cxxopts": None,
        # override the copts attribute inherited from native.cc_library
        "copts": attr.string_list(default = ["-D_FOO"]),
    },
    ...
)
```

Fixes #24066

Tricky parts:
* Some common rule attributes ("testonly", "size", etc.) have computed
defaults;
native rule "license" and "distribs" attrs have defaults which fail type
check if
lifted. The only sane way of handling such attrs, if the attribute is
unset in
the macro function call, to pass the value as None to the implementation
function
so that the implementation function will pass None to the rule function,
which
  will thus set the default appropriately.
* We need RuleFunctionApi and MacroFunctionApi interfaces (yet more
interfaces,
alas, or we get a circular dependency) to express the type of
inherit_attrs param
* Iterating over all native rules (for testing inheritance from them) is
tricky: we
need to look at builtins (since the ConfiguredRuleClassProvider may have
stubs for
java rules overridden by builtins). This is already correctly handled by
`bazel info build-language`, so let's move the logic into a utility
class.

RELNOTES: Add inherit_attrs param to macro() to allow symbolic macros to
inherit attributes from rules or other symbolic macros.
PiperOrigin-RevId: 694154352
Change-Id: I849a7f16b4da8eb2829cdbc6a131d85a28bc4740

Commit
eec5305

Co-authored-by: Googler <[email protected]>
copybara-service bot pushed a commit that referenced this issue Nov 14, 2024
…al_enable_macro_inherit_attrs flag

We still have open questions about how macro attribute inheritance ought to
interact with the tracking of whether rule attributes were or were not
explicitly provided.

In effect, this re-opens #24066
Part of addressing #24319

RELNOTES: symbolic macro attribute inheritance is now marked experimental; set --experimental_enable_macro_inherit_attrs flag to enable it.
PiperOrigin-RevId: 696582223
Change-Id: I3d7cb434bf8fe2da9cd10019e6075990205e7153
@tetromino tetromino reopened this Nov 14, 2024
@tetromino
Copy link
Contributor Author

@bazel-io fork 8.0.0

github-merge-queue bot pushed a commit that referenced this issue Nov 15, 2024
…perimental_enable_macro_inherit_attrs flag (#24336)

We still have open questions about how macro attribute inheritance ought
to interact with the tracking of whether rule attributes were or were
not explicitly provided.

In effect, this re-opens
#24066 Part of addressing
#24319

RELNOTES: symbolic macro attribute inheritance is now marked
experimental; set --experimental_enable_macro_inherit_attrs flag to
enable it.

Commit

08beb21

PiperOrigin-RevId: 696582223
Change-Id: I3d7cb434bf8fe2da9cd10019e6075990205e7153

Working towards #24335
copybara-service bot pushed a commit that referenced this issue Nov 15, 2024
It did not and cannot work; I was misled by an incorrect test. (So take the
opportunity to fix the tests.)

Working towards #24066

PiperOrigin-RevId: 696954211
Change-Id: Ia205fe4e6686d51248c1389e1cf7b826650908e8
copybara-service bot pushed a commit that referenced this issue Nov 17, 2024
…utes to None

This fixes two problems:

* Various attributes (e.g. compatible_with, restricted_to, shard_count,
  genrule's cmd and cmd_bat, etc.) have hard-coded analysis-time behavior in
  Bazel which differs depending on whether the attribute was unset (or set to
  None) or set to any other value (including the attribute's non-None
  default!). Using the original default value for such an inherited attribute
  and passing it to the wrapped rule would in many cases break the build.
* The fact that an attribute was set explicitly is reflected in query proto
  and xml output. In a legacy macro that wraps a rule and passes the bulk of
  them via **kwargs, it is expected that most of the **kwargs will be empty
  and most of the wrapped rule's attributes will thus be not explicitly set.
  We want to preserve the same behavior in symbolic macros.

Working towards #24066

Fixes #24319

PiperOrigin-RevId: 697301269
Change-Id: I47563898c511a1f065d117f51a7a3a227e23260e
github-merge-queue bot pushed a commit that referenced this issue Nov 18, 2024
Merge fix for #24066 into 8.0.0.

Closes tracking issue #24320.

I'm not sure why the automatic merge failed but it works locally, so
here goes.
tetromino added a commit to tetromino/bazel that referenced this issue Nov 19, 2024
…cros

It did not and cannot work; I was misled by an incorrect test. (So take the
opportunity to fix the tests.)

Working towards bazelbuild#24066

Cherry-pick of
bazelbuild@372980c

PiperOrigin-RevId: 696954211
Change-Id: Ia205fe4e6686d51248c1389e1cf7b826650908e8
tetromino added a commit to tetromino/bazel that referenced this issue Nov 19, 2024
…d attributes to None

This fixes two problems:

* Various attributes (e.g. compatible_with, restricted_to, shard_count,
  genrule's cmd and cmd_bat, etc.) have hard-coded analysis-time behavior in
  Bazel which differs depending on whether the attribute was unset (or set to
  None) or set to any other value (including the attribute's non-None
  default!). Using the original default value for such an inherited attribute
  and passing it to the wrapped rule would in many cases break the build.
* The fact that an attribute was set explicitly is reflected in query proto
  and xml output. In a legacy macro that wraps a rule and passes the bulk of
  them via **kwargs, it is expected that most of the **kwargs will be empty
  and most of the wrapped rule's attributes will thus be not explicitly set.
  We want to preserve the same behavior in symbolic macros.

Working towards bazelbuild#24066

Fixes bazelbuild#24319

Cherry-pick of
bazelbuild@a2f1f58

PiperOrigin-RevId: 697301269
Change-Id: I47563898c511a1f065d117f51a7a3a227e23260e
keertk pushed a commit that referenced this issue Nov 19, 2024
…al_enable_macro_inherit_attrs flag

We still have open questions about how macro attribute inheritance ought to
interact with the tracking of whether rule attributes were or were not
explicitly provided.

In effect, this re-opens #24066
Part of addressing #24319

RELNOTES: symbolic macro attribute inheritance is now marked experimental; set --experimental_enable_macro_inherit_attrs flag to enable it.
PiperOrigin-RevId: 696582223
Change-Id: I3d7cb434bf8fe2da9cd10019e6075990205e7153
@brandjon
Copy link
Member

brandjon commented Dec 4, 2024

@tetromino Any reason not to close this issue?

ramil-bitrise pushed a commit to bitrise-io/bazel that referenced this issue Dec 18, 2024
Allows the following syntax:

```
my_cc_library = macro(
    # inherit all attributes from a given rule or macro symbol (or "common" for
    # the set of common Starlark rule attributes)
    inherit_attrs = native.cc_library,
    attrs = {
        # remove cxxopts from inherited attrs list
        "cxxopts": None,
        # override the copts attribute inherited from native.cc_library
        "copts": attr.string_list(default = ["-D_FOO"]),
    },
    ...
)
```

Fixes bazelbuild#24066

Tricky parts:
* Some common rule attributes ("testonly", "size", etc.) have computed defaults;
  native rule "license" and "distribs" attrs have defaults which fail type check if
  lifted. The only sane way of handling such attrs, if the attribute is unset in
  the macro function call, to pass the value as None to the implementation function
  so that the implementation function will pass None to the rule function, which
  will thus set the default appropriately.
* We need RuleFunctionApi and MacroFunctionApi interfaces (yet more interfaces,
  alas, or we get a circular dependency) to express the type of inherit_attrs param
* Iterating over all native rules (for testing inheritance from them) is tricky: we
  need to look at builtins (since the ConfiguredRuleClassProvider may have stubs for
  java rules overridden by builtins). This is already correctly handled by
  `bazel info build-language`, so let's move the logic into a utility class.

RELNOTES: Add inherit_attrs param to macro() to allow symbolic macros to
inherit attributes from rules or other symbolic macros.
PiperOrigin-RevId: 694154352
Change-Id: I849a7f16b4da8eb2829cdbc6a131d85a28bc4740
ramil-bitrise pushed a commit to bitrise-io/bazel that referenced this issue Dec 18, 2024
…al_enable_macro_inherit_attrs flag

We still have open questions about how macro attribute inheritance ought to
interact with the tracking of whether rule attributes were or were not
explicitly provided.

In effect, this re-opens bazelbuild#24066
Part of addressing bazelbuild#24319

RELNOTES: symbolic macro attribute inheritance is now marked experimental; set --experimental_enable_macro_inherit_attrs flag to enable it.
PiperOrigin-RevId: 696582223
Change-Id: I3d7cb434bf8fe2da9cd10019e6075990205e7153
ramil-bitrise pushed a commit to bitrise-io/bazel that referenced this issue Dec 18, 2024
It did not and cannot work; I was misled by an incorrect test. (So take the
opportunity to fix the tests.)

Working towards bazelbuild#24066

PiperOrigin-RevId: 696954211
Change-Id: Ia205fe4e6686d51248c1389e1cf7b826650908e8
ramil-bitrise pushed a commit to bitrise-io/bazel that referenced this issue Dec 18, 2024
…utes to None

This fixes two problems:

* Various attributes (e.g. compatible_with, restricted_to, shard_count,
  genrule's cmd and cmd_bat, etc.) have hard-coded analysis-time behavior in
  Bazel which differs depending on whether the attribute was unset (or set to
  None) or set to any other value (including the attribute's non-None
  default!). Using the original default value for such an inherited attribute
  and passing it to the wrapped rule would in many cases break the build.
* The fact that an attribute was set explicitly is reflected in query proto
  and xml output. In a legacy macro that wraps a rule and passes the bulk of
  them via **kwargs, it is expected that most of the **kwargs will be empty
  and most of the wrapped rule's attributes will thus be not explicitly set.
  We want to preserve the same behavior in symbolic macros.

Working towards bazelbuild#24066

Fixes bazelbuild#24319

PiperOrigin-RevId: 697301269
Change-Id: I47563898c511a1f065d117f51a7a3a227e23260e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob type: feature request
Projects
None yet
Development

No branches or pull requests

2 participants