Skip to content

Add start/end params to str.startswith/endswith, allow named default in dict.pop#172

Open
twitchax wants to merge 1 commit intofacebook:mainfrom
twitchax:bazel-compat-startswith-endswith-dict-pop
Open

Add start/end params to str.startswith/endswith, allow named default in dict.pop#172
twitchax wants to merge 1 commit intofacebook:mainfrom
twitchax:bazel-compat-startswith-endswith-dict-pop

Conversation

@twitchax
Copy link
Copy Markdown

Summary

This PR makes three Bazel-compatible improvements to Starlark built-in methods:

  1. str.startswith(prefix[, start[, end]]) — Add optional start and end parameters, matching the Starlark spec and Python semantics. The substring s[start:end] is tested instead of the full string.

  2. str.endswith(suffix[, start[, end]]) — Same as above for endswith().

  3. dict.pop(key[, default]) — Allow default to be passed as a named keyword argument (e.g., d.pop("key", default=None)), not just positional. This matches Bazel's Java Starlark implementation and Python's dict.pop().

Motivation

These changes are needed for Bazel compatibility. Real-world Bazel BUILD files and .bzl macros use these features extensively:

# startswith with start/end (common in monorepo BUILD files)
[b for b in GLOBAL_BINARIES if b.startswith("//", 0, 2)]

# dict.pop with named default (common in rule macros)  
workdir = kwargs.pop("workdir", default = None)

Related Issues

  • bazelbuild/starlark#56 — The Starlark spec was updated to include start/end params for startswith/endswith, but starlark-rust never implemented them.
  • Conformance test suite inconsistency — The bazelbuild/starlark conformance tests document this with: # _inconsistency_: rust fails when default=None

Implementation Details

startswith / endswith

Reuses the existing convert_str_indices infrastructure (already used by find(), count(), index(), rfind(), rindex()) to slice the string before testing. Supports both single string and tuple-of-strings prefix/suffix arguments.

dict.pop

Removes #[starlark(require = pos)] from the default parameter, allowing it to be passed as either positional or named. The key parameter remains positional-only.

Testing

  • All 981 existing tests pass (828 lib + 153 doc)
  • New tests added for each change:
    • test_startswith_with_start_end — 7 assertions covering basic, negative indices, and tuple prefix
    • test_endswith_with_start_end — 4 assertions covering basic, negative indices, and tuple suffix
    • test_dict_pop_default_named — 3 assertions covering missing key, existing key, and fallback value

…in dict.pop

This commit makes three Bazel-compatible improvements to Starlark built-in methods:

1. **str.startswith(prefix[, start[, end]])** — Add optional `start` and `end`
   parameters to `startswith()`, matching the Starlark spec and Python semantics.
   The substring `s[start:end]` is tested instead of the full string.

2. **str.endswith(suffix[, start[, end]])** — Same as above for `endswith()`.

3. **dict.pop(key[, default])** — Allow `default` to be passed as a named keyword
   argument (e.g., `d.pop("key", default=None)`), not just positional. This
   matches Bazel's Java Starlark implementation and Python's dict.pop().

These changes address:
- bazelbuild/starlark#56 — the Starlark spec was updated to include start/end
  params for startswith/endswith, but starlark-rust never implemented them.
- The known inconsistency documented in the conformance test suite at
  bazelbuild/starlark test_suite/testdata/go/dict.star:
  `# _inconsistency_: rust fails when default=None`

All existing tests pass. New tests added for each change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@meta-cla
Copy link
Copy Markdown

meta-cla Bot commented Feb 27, 2026

Hi @twitchax!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@twitchax twitchax marked this pull request as ready for review February 27, 2026 18:20
Copilot AI review requested due to automatic review settings February 27, 2026 18:20
@twitchax
Copy link
Copy Markdown
Author

Full disclosure: identified and implemented mostly with LLM. Hope that's OK. Happy to make artisanal edits.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds Bazel-compatible enhancements to Starlark string and dict methods by implementing optional start/end parameters for str.startswith() and str.endswith(), and allowing dict.pop() to accept default as a named keyword argument. These changes align starlark-rust with the official Starlark specification and Bazel's Java implementation.

Changes:

  • Add optional start and end parameters to str.startswith() and str.endswith() methods, enabling substring range checking
  • Allow dict.pop() to accept default as a named keyword argument (e.g., default=None) in addition to positional argument
  • Add comprehensive test coverage for all three enhancements

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
starlark/src/values/types/string/methods.rs Adds start and end parameters to startswith() and endswith() methods using convert_str_indices() infrastructure; updates documentation with new examples; adds test cases for new functionality
starlark/src/values/types/dict/methods.rs Removes require = pos constraint from dict.pop() default parameter to allow named keyword usage; adds test cases verifying named parameter functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@meta-cla
Copy link
Copy Markdown

meta-cla Bot commented Feb 27, 2026

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 27, 2026
@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented Apr 24, 2026

@cjhopman has imported this pull request. If you are a Meta employee, you can view this in D102404513. (Because this pull request was imported automatically, there will not be any future comments.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants