-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Starlark rules should be able to identify when attributes are not provided #14434
Comments
Summary (correct me if I'm wrong): let all attrs default to I like the idea but we need more input from Starlark API experts. |
Thanks for taking a look.
Another (perhaps not as pretty) idea would be implement another layer in the rule |
I'd lean toward the 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. |
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, For instance, I tried to use it once to help migrate the default value of |
@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 One consistency problem then is that 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 |
This uses the same technique as `cc_binary` with/without aspects. Essentially we are declaring two rules named cc_test with different private attribute defaults. This is primarily done this way to hide the implementation details of computed defaults until something like [bazel/issues/14434](#14434) is implemented. NOTE: When `cc_test` written in Starlark is made the default, this _will_ introduce a behavior change -- notably that we will default linkstatic based on whether we are _targeting_ Windows, not whether the host Platform is Windows (as is done today). The latter heuristic may have been the best available option before Platforms, but it does not match the actual intent. PiperOrigin-RevId: 441472088
I'd vote that |
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. |
Unfortunately bazel rules doesn't know if attr was set by user or not bazelbuild/bazel#14434
Unfortunately bazel rules doesn't know if attr was set by user or not bazelbuild/bazel#14434
This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post |
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 bazelbuild/rules_pkg#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
The text was updated successfully, but these errors were encountered: