Rename stripPrefix= to strip_prefix=

[jmillikin]

Fixes #16496

[tetromino]

If you really wish to change the builtin method annotation mechanism, you'd want to (1) add error handling for both old and new parameter names being used in the same call; (2) add error handling for an old parameter name matching either a current or old name of another parameter; (3) add tests to verify all above; (4) run a microbenchmark to check if performance was affected (since getParameterIndex is very frequently used iirc). This would be a lot of work - and it's not clear to me that the work would be worth it, considering that only 2 functions would benefit!

Instead, I'd suggest undoing the annotation changes, and simply adding an extra parameter to the 2 affected functions. Make the new param documented (but make sure to mention the old name in the doc string!), make the old param undocumented, and add a check to make sure that at most 1 of the two parameters is set.

[jmillikin]

There's been multiple PRs over the years to try fixing that issue by adding a new parameter:

• fix(rctx.download_and_extract): fix naming convention inconsistencies in rctx.download_and_extract #16757
• Naming inconsistency fixed : stripPrefix -> strip_prefix #17086
• stripPrefix to strip_prefix #17107

Doing it with an alias might be slightly more lines of code, but the chances of success seem higher.

---

If you really wish to change the builtin method annotation mechanism, you'd want to (1) add error handling for both old and new parameter names being used in the same call;

Done. The error checking was actually already there, but I added a test and ensured the error message would consistently refer to the current parameter name.

(2) add error handling for an old parameter name matching either a current or old name of another parameter;

Done. There were no pre-existing checks for duplicate parameter names, so I figured the best spot to implement that would be in the annotation processor.

(3) add tests to verify all above;

Done.

(4) run a microbenchmark to check if performance was affected (since getParameterIndex is very frequently used iirc).

I adjusted the code so it'll only look for an old parameter name if one was detected at the time MethodDescriptor was created.

If more performance is needed in getParameterIndex(), then it might be worth changing that code to use a HashMap instead of a linear search.

[fmeum]

A linear search could very well be faster than a HashMap for the case of few parameters (the common case). The additional branch may still have an impact on performance, especially since it also increases the byte code size of the method.

While this can all be ruled out by benchmarks, I also don't think that this migration is worth adding features to the Starlark interpreter. At some point, we may even want to get rid of the old parameter entirely, potentially backed by a bazel_feature.

The old PRs failed because they either weren't backwards compatible or failed to attract attention by reviewers. I can promise that won't happen this time. :⁠-⁠)

[jmillikin]

Today I learned that GitHub will automatically close pull requests if the author accidentally pushes without committing.

Branch updated so it effects the rename using a new strip_prefix parameter, renaming and hiding stripPrefix.

[fmeum]

@meteorcloudy

[tetromino]

Looks good, thanks!

I took the liberty of rewording the doc string addition a little bit, to mention that the old name may still be used but is now deprecated.

[tetromino]

@meteorcloudy - I've imported the patch internally and sent it to you for review

[fmeum]

@bazel-io fork 8.0.0