-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Support dict select union #15075
Support dict select union #15075
Conversation
3c29422
to
7ff757c
Compare
@tetromino Would you be able to review this? It's the reason why I worked on introducing the dict union operator in the first place. |
src/main/java/com/google/devtools/build/lib/packages/SelectorList.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/packages/SelectorList.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for type/operator mismatches: dict | list, list | dict, dict + list, list + dict.
aa0c989
to
13675fc
Compare
@comius AFAIK tetromino won't be available until July. Is there anybody else who could take over the review? |
@@ -559,6 +560,15 @@ public Map<KeyT, ValueT> convert(Object x, Object what, Object context) | |||
return ImmutableMap.copyOf(result); | |||
} | |||
|
|||
@Override | |||
public Map<KeyT, ValueT> concat(Iterable<Map<KeyT, ValueT>> iterable) { | |||
LinkedHashMap<KeyT, ValueT> output = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably LinkedHashMap is used instead of ImmutableMap.Builder (at the cost of a copy) to avoid an exception when the keys intersect. A comment to this effect would be good.
Better still: use Dict.Builder, which avoids the exception, and will take advantage of the exception-free builder semantics (buildKeepingLast) when they become available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildKeepingLast
is in fact already available: https://cs.opensource.google/bazel/bazel/+/4eb62cd07393e495ab9ead49b771871d1050ee51:src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java;l=111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use buildKeepingLast
, although from the docs:
In the current implementation, all values associated with a given key are stored in the Builder object, even though only one of them will be used in the built map. If there can be many repeated keys, it may be more space-efficient to use a LinkedHashMap and ImmutableMap.copyOf(Map) rather than ImmutableMap.Builder.
src/main/java/com/google/devtools/build/lib/packages/SelectorList.java
Outdated
Show resolved
Hide resolved
13675fc
to
462f796
Compare
@fmeum @tetromino @comius Any update on this? I'd be awesome to have it in the next release |
@AlessandroPatti Not a Googler, so I can't do more than stating that this LGTM. @brandjon Do you happen to have cycles to get this into 5.3.0? |
@comius @tetromino Friendly ping. This has been in review for almost half a year now, it would be great to finally get it merged. |
@comius @tetromino @gregestren Friendly ping, is there any blocker for merging this? |
* @throws EvalException if all values don't have the same underlying type | ||
* @throws IllegalStateException if all values don't have the same underlying type | ||
*/ | ||
static SelectorList of(Iterable<?> values) throws EvalException { | ||
static SelectorList of(Iterable<?> values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this change. Unchecked exceptions in the Starlark interpreter are dangerous: at worst, they could lead to a Bazel server process crash; at best, they might be caught somewhere in the depths of skyframe where we would be in no position to print a useful error message. (In particular, what worries me is the possibility of the unchecked exception in of
being thrown as a result of a native.existing_rule()
call - see StarlarkNativeModule.) Instead, we genuinely do want to throw an EvalException, because that's what gets caught at the boundary between Bazel and the Starlark interpreter and results in an error message with a beautiful Starlark stack trace using which a rule author can use to fix their rules.
(Also, for future reference: if we wanted to use an unchecked exception, we would signal an invalid argument to a function by throwing an IllegalArgumentException, not an IllegalStateException. See e.g. Guava's https://github.com/google/guava/wiki/PreconditionsExplained guide.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, the other two places where this function is called are not performing any evaluation, but rather exposing this data structure in a human-friendly way. This is why I assumed that this should never happen (assuming the validation during the creation of the SelectorList
is correct) and it is thus an "illegal state" if it happens. Regardless, reverted to throw an EvalException if this happens to be on the safe side.
src/main/java/com/google/devtools/build/lib/packages/SelectorList.java
Outdated
Show resolved
Hide resolved
if (!canConcatenate(getNativeType(this), getNativeType(that))) { | ||
throw Starlark.errorf( | ||
"'%s' operator applied to incompatible types (%s, %s)", | ||
op.toString(), getTypeName(thisLeft ? this : that), getTypeName(thisLeft ? that : this)); | ||
} | ||
if (getNativeType(that).equals(Dict.class)) { | ||
if (op == TokenKind.PIPE) { | ||
return thisLeft ? concat(this, that) : concat(that, this); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we are pointlessly checking canConcatenate twice: in binaryOp
and in of()
.
A solution, IMHO, would be to make concat
a private method which simply does return new SelectorList(getNativeType(x), ImmutableList.of(x, y))
without performing any canConcatenate
check. It should be easy to do since we're no longer calling concat
from SelectorValue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried making concat
private and return a new SelectorList directly but
- A few tests use
concat
, so the visibility cannot be private, it would require using@VisibileForTesting
of
flattens the SelectorLists elements, which we would not do here instantiating the SelectorList directly, causing errors likeexpected type <type> for attribute <attr>, but got select
Instead I removed the check entirely so we rely on of
to fail if the types cannot be combined.
My apologies for the very late review! |
462f796
to
f6ff54b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looks good now!
Starlark has recentely added support for union operations over dictionaries (bazelbuild/starlark#215). The syntax is already supported in bazel as of bazelbuild#14540, but the same operation with selects of dictionaries is still usupported. Related issue: bazelbuild#12457 Closes bazelbuild#15075. PiperOrigin-RevId: 469827107 Change-Id: If82cfaf577db41efc2e9b55af47b0b1710badc10
Starlark has recentely added support for union operations over dictionaries (bazelbuild/starlark#215). The syntax is already supported in bazel as of bazelbuild#14540, but the same operation with selects of dictionaries is still usupported. Related issue: bazelbuild#12457 Closes bazelbuild#15075. PiperOrigin-RevId: 469827107 Change-Id: If82cfaf577db41efc2e9b55af47b0b1710badc10
Starlark has recentely added support for union operations over dictionaries (bazelbuild/starlark#215). The syntax is already supported in bazel as of bazelbuild#14540, but the same operation with selects of dictionaries is still usupported. Related issue: bazelbuild#12457 Closes bazelbuild#15075. PiperOrigin-RevId: 469827107 Change-Id: If82cfaf577db41efc2e9b55af47b0b1710badc10
Starlark has recentely added support for union operations over dictionaries (bazelbuild/starlark#215). The syntax is already supported in bazel as of bazelbuild#14540, but the same operation with selects of dictionaries is still usupported. Related issue: bazelbuild#12457 Closes bazelbuild#15075. PiperOrigin-RevId: 469827107 Change-Id: If82cfaf577db41efc2e9b55af47b0b1710badc10
Starlark has recentely added support for union operations over dictionaries (bazelbuild/starlark#215). The syntax is already supported in bazel as of #14540, but the same operation with selects of dictionaries is still usupported.
Related issue: #12457