Skip to content

[mypyc] Optimize str.startswith and str.endswith with tuple argument #18678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 18, 2025

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Feb 14, 2025

No description provided.

@cdce8p cdce8p added the topic-mypyc mypyc bugs label Feb 14, 2025
@cdce8p cdce8p force-pushed the mypyc-str-startswith branch from d25cb2b to 1ecfacc Compare February 14, 2025 12:34
@Skylion007
Copy link
Contributor

Skylion007 commented Feb 14, 2025

We can also force these semantics in the mypy code with PIE810, this is a desirable behavior since it only has to traverse the string once to check multiple args simultaneously in a 'trie' like fashion. Ah nvm, I see it's already selected as a ruff rule.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good, just some ideas about additional test coverage.

assert match_tuple('abc', ('d', 'e')) == (False, False)
assert match_tuple('abc', ('a', 'c')) == (True, True)
assert match_tuple('abc', ('a',)) == (True, False)
assert match_tuple('abc', ('c',)) == (False, True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a test case where startswith matches a non-first tuple item. Also add test for error case (tuple contains a non-string).

It would be good to add an irbuild test for a tuple literal argument, since it's easy to imagine how a fixed-length tuple literal wouldn't match against a variable-length tuple in the primitive arg type.

@cdce8p cdce8p force-pushed the mypyc-str-startswith branch from bb479ed to 5ab9840 Compare February 18, 2025 01:39
from typing import Tuple

def do_startswith(s1: str, s2: Tuple[str, ...]) -> bool:
return s1.startswith(s2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm more interested in what happens when startswith is given a tuple literal argument (e.g. return s1.startswith(('x', 'y'))), since this is probably the most common use case. Can you test also this? It would be good to have also a run test for this.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks! We can improve this later by avoiding tuple boxing, but this is already a nice improvement.

@JukkaL JukkaL merged commit 09ac3ba into python:master Feb 18, 2025
13 checks passed
@cdce8p cdce8p deleted the mypyc-str-startswith branch February 18, 2025 13:47
JukkaL pushed a commit that referenced this pull request Feb 18, 2025
…18703)

Followup to #18678

Missed that we can also use `bool_rprimitive` as return type with a
value of `2` for errors.
x612skm pushed a commit to x612skm/mypy-dev that referenced this pull request Feb 24, 2025
…ython#18703)

Followup to python#18678

Missed that we can also use `bool_rprimitive` as return type with a
value of `2` for errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-mypyc mypyc bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants