Closed nacl closed 2 months ago
Summary (correct me if I'm wrong): let all attrs default to None
to make it easy to tell if an attribute was explicitly set.
I like the idea but we need more input from Starlark API experts.
Thanks for taking a look.
None
attr defaults is what I would like to see (it's very intuitive and pythonic), but I'd be happy to see other options too.
Another (perhaps not as pretty) idea would be implement another layer in the rule ctx
structure, like ctx.provided.$ATTR
that would contain boolean values specifying whether $ATTR
was present in the target's attribute list.
I'd lean toward the None
approach on account of it being a leaner API (smaller # of API functions).
Technically it also expands the API but I'd always prefer simple solutions leveraging what we already have vs. one more function call to maintain and remember.
Oh, @brandjon has floated the idea of ctx.attr
providing the target's complete attr
structure instead of just its value. That's another potential location for this.
I'm generally against any extension to the expressivity of attributes unless there's a pretty compelling use case that can't be met. But the intersection between "pretty compelling" and "but still not so ambitious as to be out-of-scope" is rather small if not zero. See a more detailed justification here.
As for the second idea, adding a way to directly probe whether an attribute value was set explicitly when the target was defined, or set by default because it was omitted: This is something of an antipattern. We have this feature for native rules, isAttributeValueExplicitlySpecified
, and it's caused some trouble.
For instance, I tried to use it once to help migrate the default value of python_version
from PY2
to PY3
. The idea was that if the value was set by the user, keep it, but if it was left at its default value, consult an incompatible change flag. This broke down due to fancy macros where users attempted to copy Python targets by programmatically iterating over their attributes via native.existing_rules
, since any default value would turn into a seemingly explicitly set default value. (I ended up instead creating a new sentinel value for the Python version and making it the default.)
@brandjon I think the original proposal is in essence requesting a way to have a sentinel value. Especially for booleans which are False by default--having a None option would allow for that.
+1 to allowing default=None for any attribute type. Although the original request doesn't say it, this would be very useful in any situation where we need a computed default.
Yes, the original proposal is proposing a sentinel, but the cost is that the consumer must be aware of and account for the possibility of that sentinel. This effectively turns booleans into tristates, for example. (Insert something about the "billion dollar mistake" around the concept of null references.)
That said, let's be a little more concrete about how the proposal might work. I assume we wouldn't change the behavior of existing attributes, since that'd be a major change to every Starlark rule in existence, giving a new failure mode that the rule author couldn't possibly have anticipated. So would it be that all attributes types now take a can_be_none=True
param?
One consistency problem then is that my_attr = None
means different things depending on whether my_attr
is an attr.label(...)
or an attr.label(can_be_none=True, ...)
. One converts to the empty list, and the other does not.
I'm somewhat ambivalent and tend to err on the side of caution, but I'm not convinced either way.
My initial thought was that one would write attr.bool(default = None)
.
I'd vote that default = None
is the most sensible choice here as it doesn't break the API and is explicit.
Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.
This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage
in a comment here and we'll take a look. Thanks!
Description of the feature request:
When creating bazel rules, I sometimes want to be able to identify when a rule attribute is provided. Usually, the
default
is good enough, but there are some cases where there isn't a consistent way to provide defaults that cannot otherwise be interpreted as something meaningful. In particular,attr.bool
,attr.int
, andattr.string
come to mind -- they only allow for their exact types (bool
,int
andstr
respectively). As an example, if I create a rule with anint
attribute thatdefault
s toNone
, I see something like:Defaulting to
None
is the obvious thing that comes to my mind. In its absence, rule authors need to implement custom behavior to identify integers or strings that can be translated as "not provided". In the case ofbool
s, this is not possible, and users would have to resort to usingint
s.The other
attr
s are not as strongly affected -- they AFAICT either already acceptNone
(attr.label
), or empty sequences or dictionaries (everything else). In each of these cases, there is no strong need to specify a new "not provided"-style default, sinceNone
already is this, or empty sequences can usually just be skipped.It would be nice if there was a consistent way to identify that an attribute is not provided.
None
is intuitively suitable for this purpose, but others options would be acceptable.An option for this could be to support
None
in rule attributes more broadly. For example, Bazel currently acceptsNone
as a valid input for at least some of the built-in attributes liketags
andfeatures
. Also, when provided to rules as arguments,None
is implicitly converted to the "null" value for the type in question (e.g.False
,0
,""
,[]
).Feature requests: what underlying problem are you trying to solve with this feature?
I'd like rules to be more consistently readable by end-users and reduce the overall use of magic values. The meaning of
None
is usually apparent from its presence, and would be a good choice to represent this.Having an obvious way to say "not provided" would have also been very useful in preventing issues like https://github.com/bazelbuild/rules_pkg/issues/486 from occurring -- if
None
was an option forattr.int
from the beginning, it likely would have been used.What operating system are you running Bazel on?
macOS Big Sur 11.16.1, x86_64.
What's the output of
bazel info release
?(Same behavior occurs with Bazel 4.2.2, also downloaded via bazelisk)
If
bazel info release
returns "development version" or "(@non-git)", tell us how you built Bazel.Downloaded with
bazelisk
using:(That hash was
last_green
at the time)What's the output of
git remote get-url origin ; git rev-parse master ; git rev-parse HEAD
?N/A
Have you found anything relevant by searching the web?
None
viaselect
s, but does not refer to the behavior ofdefault
sAny other information, logs, or outputs that you want to share?
Got an example below. To reproduce the failure mentioned earlier in this ticket, uncomment the
default
argument to theint
attribute:https://gist.github.com/nacl/1b469f75ffbf704ab1955955687d7e00