Keyword arguments are being accepted where they should be disallowed

[brandjon]

Some builtin functions accept keyword arguments where they should be positional-only. This creates unnecessary differences from Python and perhaps promotes bad style.

Example:

    # The fact that it's called "elements" is an implementation and documentation detail.
    # You shouldn't actually be able to call all() this way.
    all(elements=[True, False])

This comes from a bug in how Param#named is ignored for @SkylarkSignature. As we migrate @SkylarkSignature over to @SkylarkCallable, we've been inadvertently fixing this bug, thereby breaking backward compatibility without an incompatible change flag.

We've added Param#legacyNamed to prevent further breakages until we can control them with a flag (4baafac). For builtins that have already been migrated over, you may see errors like "unexpected keyword" or "parameter ... has no default value". If this impacts a significant number of users we could do a patch release to mark them legacyNamed. Otherwise it's better if users just update their code now since we want to be consistent with Python eventually anyway.

[brandjon]

To be clear, only some builtin functions are affected. It looks like in the vast majority of cases, the functions that have been migrated are functions that take almost all their parameters by keyword anyway, so there shouldn't be much difference noticed.

[brandjon]

Reassigning to Chris since he's managing Build Api related issues.

[c-parsons]

This is on my radar for this quarter.

[laurentlb]

cc @vladmos
Buildifier should help do the cleanup.

[vladmos]

That looks like an easy cleanup for buildifier. Is there a complete list of functions with keywords that shouldn't be accepted?

[c-parsons]

Any @Param annotation for a @SkylarkCallable which has legacyNamed = true falls into the category of "parameter we wish were not keyword".

Example:

bazel/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java

Line 174 in df5fe4a

legacyNamed = true,

[vladmos]

The rule function has 11 arguments marked with legacyNamed = true, do we want all of them to become positional only?

bazel/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java

Line 97 in c33ae8a

name = "rule",

[brandjon]

It looks like legacyNamed was used when what's needed is the opposite -- "legacyPositional". rule() should ideally have no positional parameters.

[c-parsons]

Summarizing our offline discussion:
legacyNamed is used in cases where, historically, we did not include named = True yet there was a bug present where it was as if named = True. Sometimes the exclusion was intentional, sometimes it wasn't. We have to make a call in many of these cases.

In cases where we make the call to make the param unspecifiable by name, this is technically a breaking change, so we need to attach it to an incompatible flag. We don't have the infrastructure to do this yet, so I'll need to implement this infrastructure.

[vladmos]

There are the following usage of legacyNamed = true in Bazel:

• provider: doc
• rule: implementation, 10 more params
• aspect: implementation, 8 more params
• Label: label_string
• FileType: types (deprecated anyway)
• glob: include, exclude, exclude_directories
• existing_rule: name
• exports_files: srcs, visibility, licenses

String methods:

• join: elements
• lstrip, rstrip, strip: chars
• replace: old, new, maxsplit
• split, rsplit: sep, maxsplit
• partition, rpartition: sep
• rfind, find, rindex, index, count, endswith, startswith: sub, start, end
• splitlines: keepends

Some builtins with keyword parameters that we'd probably like to make positional-only are @SkylarkSignatures and therefore don't have parameter maked with legacyNamed = true. Here are some examples:

• all, any: elements
• tuple, list, len, str, repr, bool, int, dir, type: x
• dict: kwargs
• enumerate: list
• hash: value
• range: start_or_stop, stop_or_none, step (even if we keep these we'll need to rename the first two)
• hasattr: x, name
• getattr: x, name, default (we may want to keep default, maybe also name?)
• fail: msg, attr
• select: x, no_match_error

I can make a buildifier fix for some of the parameters above and update it later as we agree on making more parameters positional. I guess all parameters called x should be positional, also elements for all and any, and list for enumerate. May I implement a fix for them? Should I include anything else?

[brandjon]

We should (if it hasn't already been done) manually vet changes we make to arguments' keywordy-ness against the behavior of Python 2 and 3 to confirm we're making things more consistent. But even if a change is inconsistent, we can still go ahead with it if it helps readability.

[vladmos]

Created a prototype: bazelbuild/buildtools#559

Once we agree on the list of functions and their arguments (it doesn't have to be the final list, can be extended any time), I'll merge it to buildifier.

[alandonovan]

The important distinction is between core Starlark (e.g. getattr, splitlines) and the build language (e.g. fail, aspect). In the core, following Python, very few functions accept named arguments; int(base=...) is one of the rare exceptions. In the build language, and for user-defined macros, it's much more common for functions to accept or even require named arguments.

There is something ironic about the fact that the core is moving closer to Python (removing names) while the build language is moving towards Python3 by allowing users to require names in their functions. But I think it is the right thing.

[c-parsons]

Marking fixed, as #8147 is fixed.