Attribute inheritance causes analysis failures for certain built-in attributes

[brandjon]

The new attribute inheritance system handles attributes with computed defaults by setting their value to None within a symbolic macro. That way, when they're passed through to the wrapped rule, the computed-default logic considers it to be missing and computes its value as intended.

But some attributes that are morally computed defaults are not in fact implemented that way. The timeout attribute of test rules is one example. restricted_to is another (that one only fails at analysis time).

A quick solution is to hardcode a list of these badly behaved builtin attributes and None-ify them all. Though this might require a backwards incompatible change to fix their behavior in the future.

Edit: Correction: restricted_to causes it to fail at loading time, not analysis. timeout works for starlark-defined rules but not native ones, because it's specified as a computed default only for starlark rules. Yet we also found a crash bug here, when inheriting attributes of an unexported rule. We think a good fix is to hardcode problematic attribute names in MacroClass#forceDefaultToNone, and only in cases where the Attribute does not have the property flag STARLARK_DEFINED (to avoid triggering on user-defined attrs of the same name). Alternatively, we could give some of these problematic attributes trivial computed defaults, but that seems like it could subtly break things (query output or native.existing_rules perhaps).

[brandjon]

@bazel-io fork 8.0.0

[brandjon]

Our discussion of this issue turned into a tangled web of confusion around the meaning of None and default values in macros, but we think we have a reasonably elegant solution now:

1. Within macros, but not rules, attribute descriptors (attr.string(), etc.) are now allowed to pass default=None. (Previously the only commonly used attr type that could do this was attr.label().) This lets a macro opt into declaring that it can see when one of its attributes was not explicitly set. (This is already possible for attr.label(), since my_label_attr = None counts as not passing anything in at all.) Having this feature is conceptually important if macros are ever going to help deprecate the concept of computed defaults.

2. For inherited attributes, the default of the descriptor is always overridden to be None. Doesn't matter if you're inheriting a computed default, or a simple default="abc" -- the macro will see None if the caller didn't pass anything in.

3. For configurable attributes, None gets promoted to a trivial select, select({"//conditions:default": None}). (We really need a more concise stringification syntax for that -- maybe select(None)?). When passing this to either a rule or a macro, this gets unwrapped as my_attr = select({"//conditions:default": None}) -> my_attr = None -> dropped. So you see a select value inside your macro, but the rule or macro you're instantiating doesn't get passed anything, and it's not considered explicitly set. Thus, computed defaults execute as intended, it's omitted from bazel query inspection, etc.

This would seem to solve the direct bugs raised in this issue about crashes when inheriting built-in attributes. It also maintains the nice property that macro authors don't need to worry about None being a distinct value, at least for non-inherited attributes, unless they specifically opt into that as their default or they are trying to manipulate an inherited attribute. And when they do see a None in a configurable attribute, it's still a select() just like anything else. Furthermore, when migrating legacy macros to symbolic, we avoid a regression where all the **kwargs attributes seem to be explicitly set on the rule.