dict.update should return None, not self

[alandonovan]

In .bzl files, the expression dict(a=1).update(b=2) evaluates to {a: 1, b: 2} but according to
https://github.com/bazelbuild/starlark/blob/master/spec.md#dictupdate
it should return None.

Both Pythons and Starlark-in-Go return None:

$ python # 2 or 3

x = {}
x.update(k=1)
x
{'k': 1}

$ starlark # in Go

x = {}
x.update(k=1)
x
{"k": 1}

The Java standalone interpreter gives a strange error --- I have no idea what it means:
% blaze-bin/third_party/bazel/src/main/java/com/google/devtools/skylark/Skylark

x = {}
x.update(k=1)
parameter 'other' has no default value, in method call update(int k) of 'dict'

Googlers: see L23 of dart_dev_ctx.bzl in cr/223363742 for a real-world example.

[laurentlb]

Thanks!
That's a bug in Bazel.

[brandjon]

In .bzl files, the expression dict(a=1).update(b=2) evaluates to {a: 1,

b: 2} How does it do that? The bug is that dict.update doesn't take entries as **kargs, so shouldn't that fail as per the example you gave?

…

On Thu, Nov 29, 2018 at 6:44 PM Laurent Le Brun ***@***.***> wrote: Thanks! That's a bug in Bazel. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#18 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABP1phtFT0XxLPhigdLCTycR2bje5RHzks5u0HFlgaJpZM4Y6uHs> .

[laurentlb]

Error message is bad, but the .update() method doesn't accept keyword arguments: https://docs.bazel.build/versions/master/skylark/lib/dict.html#update

If you do: x.update({"k": 1}), you'll see that update() returns None as expected.

cr/223363742 boils down to:

SomeInfo = provider(
    fields = {
        "...": "...",
    }.update(other_fields),

This is confusing, because update returns None. In other words, the dictionary is created, modified and thrown away.

[alandonovan]

Error message is bad, but the .update() method doesn't accept keyword arguments:
https://docs.bazel.build/versions/master/skylark/lib/dict.html#update

So it seems like the resolution is to make the Java impl and its doc match what the spec doc says:
https://github.com/bazelbuild/starlark/blob/master/spec.md#dictupdate

This is confusing, because update returns None. In other words, the dictionary is created, modified and thrown away.

Yes, it's an easy trap to fall into. Do you have a lint check for it? The deprecated {} + {} wasn't so bad after all. :(

[laurentlb]

make the Java impl and its doc match what the spec doc says

Right. Good enough, it's just a new feature and not a breaking change.

In the future, we can indeed extend the linter to catch this kind of things. Right now, we focus on the deprecated features, and help users migrate their code.

[laurentlb]

Fixed in Bazel