Handle negative count arg in string.replace by zegl · Pull Request #9228 · bazelbuild/bazel

zegl · 2019-08-22T20:49:19Z

Also update the name of the argument to be "count" instead of "maxsplit",
to match the Starlark spec.

This fixes #9181

iirina · 2019-08-29T13:45:06Z

@laurentlb @c-parsons can you take a look? Thanks!

zegl · 2019-09-07T07:02:32Z

I've rebased and force-pushed to resolve conflicts in EvaluationTest.java.

ulfjack · 2019-10-02T13:31:57Z

            doc = "The string to replace with."),
        @Param(
-            name = "maxsplit",
+            name = "count",


I think renaming this would be a breaking change. May need to keep the old name for now.

Yes, it's clearly breaking! The question is if it's a change that's allowed to be made according to the compatability policy, the Backwards Compatability document does not mention what's allowed and not in these edge cases.

To draw some inspiration from other projects: the Go1 compatability document states:

If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.

I think that this is an example of that, as the Starlark implementation in Bazel does not satisfy the spec.

I'll leave it up to you and the team to figure this out, but I do think that it would be nice to allow to make some breaking changes without increasing the major-version number in the future.

This is definitely a breaking change which is not allowed by our policy -- apologies this is not clear by the policy doc, but this is definitely going to break some users and cause some pain if we change it.

We should avoid renaming this for now.
The parameter is tagged with legacyNamed = true, so, subject to the rollout of #8147, this parameter will be positional-only in the future. We can then rename the parameter safely for documentation purposes.

To clarify, there's two breaking changes: the rename and the behavior for negative values.

The rename is subsumed by #8147, so we should definitely not do any extra work here. You can add a comment reminding us to get back to it:

// TODO(#8147): rename param to "count" once it's positional-only.

The behavioral change for negatives is also breaking. I feel like it's not very likely people will be broken by it, but it seems like the kind of thing we should guard with a flag. Look for other examples of using incompatible flags in Starlark builtins -- for instance, incompatibleDisableDepsetItems in the depset constructor later in this file, and its corresponding definitions in StarlarkSemantics and StarlarkSemanticsOptions.

Ok, let's do this the proper way, by marking the negative value behaviour change as breaking.

Other than adding the flag and the handling of it, is there anything else that needs to be done? What's the process for creating issues with the incompatible-change and breaking-change-* labels?

When you file the GitHub issue, you can cc me so that I add the labels.
If your change is ready this week, we can include it in Bazel 3.2.

Thanks!

@alandonovan points out to me that we actually already flipped and deleted --incompatible_restrict_named_params, so I'll do the rename as part of the follow-up.

aiuto · 2020-02-12T19:45:41Z

Ping

aiuto · 2020-04-23T02:31:20Z

Another ping

zegl · 2020-04-23T09:01:04Z

@aiuto Are you pinging me?

What would be the process of getting this change in now? I'm guessing that it's still not allowed breakage, or is it?

zegl · 2020-04-23T09:07:27Z

I resolved the conflicts (Runtime.NONE has been renamed to Starlark.NONE), and fixed the issues found by c-parsons.

Also update the name of the argument to be "count" instead of "maxsplit", to match the Starlark spec.

laurentlb · 2020-04-23T17:13:23Z

Can you take a look at https://www.bazel.build/breaking-changes-guide.html?
If you put the change behind a flag, we'll be able to import it and ensure a smooth rollout.

Thanks!

aiuto · 2020-04-25T02:08:31Z

I was pinging all the stale PRs so we can finish them or close them.

brandjon · 2020-04-27T21:33:08Z

            doc = "The string to replace with."),
        @Param(
-            name = "maxsplit",
+            name = "count",


To clarify, there's two breaking changes: the rename and the behavior for negative values.

The rename is subsumed by #8147, so we should definitely not do any extra work here. You can add a comment reminding us to get back to it:

// TODO(#8147): rename param to "count" once it's positional-only.

The behavioral change for negatives is also breaking. I feel like it's not very likely people will be broken by it, but it seems like the kind of thing we should guard with a flag. Look for other examples of using incompatible flags in Starlark builtins -- for instance, incompatibleDisableDepsetItems in the depset constructor later in this file, and its corresponding definitions in StarlarkSemantics and StarlarkSemanticsOptions.

…behaviour if true The previous (pre PR) change has been re-added, and is used by default.

This allows to test with different values of the incompatible flag

alandonovan · 2020-05-01T14:01:22Z

+
+    StarlarkSemantics semantics = thread.getSemantics();
+    if (semantics.incompatibleStringReplaceCount()) {
+      if (count != Starlark.NONE && (Integer) count >= 0) {


Also: 'None' is not a legal argument for count, according to the Starlark spec (which matches Pythons 2 and 3 and go.starlark.net implementation).

Hm, I'll write a quick followup to change the default value to the internal UNBOUND sentinel and have the incompatible flag also reject the value None.

Thanks. I suggest to:

Change the default value to unbound

Raise an error if the argument is None and the incompatible flag is enabled

After the flag flip, remove noneable=True

brandjon · 2020-05-01T14:06:33Z

FYI in the internal merge we retained some of the test cases in string_misc.sky and added a TODO to port StringModuleTest back over to those once the incompatible flag is no longer a blocker. We do prefer that tests of pure Starlark features be written in Starlark where possible (we've got enough verbose Java tests :) ).

zegl requested review from brandjon, c-parsons, dslomov, laurentlb and vladmos as code owners August 22, 2019 20:49

googlebot added the cla: yes label Aug 22, 2019

zegl force-pushed the string-replace-fixes branch from 4cac4a1 to 9545d79 Compare August 22, 2019 20:52

aiuto added team-Starlark untriaged labels Aug 23, 2019

aiuto removed request for brandjon, dslomov and vladmos August 23, 2019 18:56

iirina removed the untriaged label Aug 29, 2019

zegl force-pushed the string-replace-fixes branch from eded3fd to e0bbf92 Compare September 7, 2019 07:02

jin assigned laurentlb Oct 2, 2019

ulfjack reviewed Oct 2, 2019

View reviewed changes

c-parsons reviewed Oct 2, 2019

View reviewed changes

aiuto self-assigned this Apr 23, 2020

zegl force-pushed the string-replace-fixes branch from e0bbf92 to 6a25d31 Compare April 23, 2020 09:06

zegl added 2 commits April 23, 2020 11:08

Handle negative count arg in string.replace

097bdc8

Also update the name of the argument to be "count" instead of "maxsplit", to match the Starlark spec.

Update string.replace tests in string_misc.sky

c8f0e94

zegl force-pushed the string-replace-fixes branch from 6a25d31 to c8f0e94 Compare April 23, 2020 09:08

brandjon requested changes Apr 27, 2020

View reviewed changes

Docs for string.replace, use existing param name, and simplify condition

abe0a98

zegl force-pushed the string-replace-fixes branch from 9730ce6 to 73ff880 Compare April 28, 2020 21:51

zegl added 2 commits April 29, 2020 00:06

Add --incompatible_string_replace_count flag and only enable the new …

88e912f

…behaviour if true The previous (pre PR) change has been re-added, and is used by default.

Move most string.replace() tests from string_misc to StringModuleTest

fafa256

This allows to test with different values of the incompatible flag

zegl force-pushed the string-replace-fixes branch from 73ff880 to fafa256 Compare April 28, 2020 22:07

zegl mentioned this pull request Apr 29, 2020

incompatible_string_replace_count #11244

Closed

bazel-io closed this in 00ea8fa May 1, 2020

alandonovan reviewed May 1, 2020

View reviewed changes

Conversation

zegl commented Aug 22, 2019

Uh oh!

iirina commented Aug 29, 2019

Uh oh!

zegl commented Sep 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zegl Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aiuto commented Feb 12, 2020

Uh oh!

aiuto commented Apr 23, 2020

Uh oh!

zegl commented Apr 23, 2020

Uh oh!

zegl commented Apr 23, 2020

Uh oh!

laurentlb commented Apr 23, 2020

Uh oh!

aiuto commented Apr 25, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandjon commented May 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

zegl Apr 28, 2020 •

edited

Loading