-
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
Convert values to Starlark values when concating string_list_dict type #23424
Convert values to Starlark values when concating string_list_dict type #23424
Conversation
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 for your contribution! I had reviewed #15075 two years ago - and had failed to notice the problem.
My main concern with the cast()
call is that (1) it introduces a completely unnecessary temporary Dict
object per each concatenated map, and (2) the semantics of Type.concat
- as far as I can tell - don't require the returned value to be a subclass of StarlarkValue (compare Type.ListType.concat
, which returns an ImmutableList
, not a StarlarkList
).
So instead, can we use a plain ordinary ImmutableMap.Builder
?
ImmutableMap.Builder<KeyT, ValueT> builder = ImmutableMap.builder();
for (Map<KeyT, ValueT> map : iterable) {
builder.putAll(map);
}
return builder.buildKeepingLast();
src/test/java/com/google/devtools/build/lib/packages/TypeTest.java
Outdated
Show resolved
Hide resolved
0720678
to
9be44a8
Compare
Thanks for the review, this is really the better solution for the problem. |
@lior10r - that's an excellent idea. I would suggest adding such a test as a new method to src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java |
Dict type. This is because Dict checks that all the values put in it are Starlark valid values. But string_list_dict value is of type List<String>, when Java's List isn't a Starlark value. Also added some tests. Fixes bazelbuild#23065
9be44a8
to
dfcf5a0
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.
Thanks for the test, looks good! I apologize for missing notification about the last commit.
@bazel-io fork 7.4.0 |
Convert values to Starlark values when concating string_list_dict type. This is because Dict checks that all the values put in it are Starlark valid values. But string_list_dict value is of type List<String>, when Java's List isn't a Starlark value. Fixes bazelbuild#23065 My first PR to bazel 🤩 Would love you to review the PR. Checked that the test didn't pass before the change and did after it. Closes bazelbuild#23424. PiperOrigin-RevId: 673463108 Change-Id: Ib2cdce7d94243f4e3bda23203a196fee4e766189
…dict type (#23603) Convert values to Starlark values when concating string_list_dict type. This is because Dict checks that all the values put in it are Starlark valid values. But string_list_dict value is of type List<String>, when Java's List isn't a Starlark value. Fixes #23065 My first PR to bazel 🤩 Would love you to review the PR. Checked that the test didn't pass before the change and did after it. Closes #23424. PiperOrigin-RevId: 673463108 Change-Id: Ib2cdce7d94243f4e3bda23203a196fee4e766189 Commit f1f8d58 --------- Co-authored-by: Lior Gorelik <lior10rrr@gmail.com> Co-authored-by: Alexandre Rostovtsev <arostovtsev@google.com>
Convert values to Starlark values when concating string_list_dict type.
This is because Dict checks that all the values put in it are Starlark valid values. But string_list_dict value is of type List, when Java's List isn't a Starlark value.
Fixes #23065
My first PR to bazel 🤩 Would love you to review the PR.
Checked that the test didn't pass before the change and did after it.