Skip to content

Rework Match.group handling - #5557

Merged
Akuli merged 4 commits into
python:masterfrom
srittau:re-match
Jun 8, 2021
Merged

Akuli merged 4 commits into
python:masterfrom
srittau:re-match

Conversation

@srittau

@srittau srittau commented May 30, 2021 •

Copy link
Copy Markdown
Collaborator

Standardize group(), groups(), groupdict(), and __getattr__() to return AnyStr | Any instead of AnyStr. Also special case group(0) and __getattr__(0) to always return AnyStr. Correctly handle defaults. Use PEP 604 for unions in changed lines.

Edited to include changes after review below.

Fixes #3902

srittau added 2 commits May 30, 2021 14:53
Standardize group(), groups(), groupdict(), and __getattr__() to return
Any instead of AnyStr. Also special case group(0) and __getattr__(0) to
always return AnyStr. Use PEP 604 for unions in changed lines.
Comment thread stdlib/typing.pyi
@Akuli

Akuli commented May 30, 2021

Copy link
Copy Markdown
Collaborator

I'm arfaid this will cause dead code warnings in mypy, if you do something like this:

foo = match.group(1)
if foo is None:
    ...
else:
    ...

Having type checkers say "this is a string" has caused confusion before. See #5269 (comment) for example.

@srittau

srittau commented May 30, 2021

Copy link
Copy Markdown
Collaborator Author

Maybe we should just use AnyStr | Any here as well. I will try that.

@Akuli

Akuli commented May 30, 2021

Copy link
Copy Markdown
Collaborator

Seems like it would also be better for mypyc (it's just one tool that uses typeshed, but an important tool): #5269 (comment)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment thread stdlib/typing.pyi Outdated
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

sphinx (https://github.com/sphinx-doc/sphinx.git)
+ sphinx/ext/autodoc/__init__.py: note: In member "parse_name" of class "Documenter":
+ sphinx/ext/autodoc/__init__.py:405:23: error: Need type annotation for "parents"

pydantic (https://github.com/samuelcolvin/pydantic.git)
+ pydantic/networks.py:267: error: Item "None" of "Optional[Match[str]]" has no attribute "group"  [union-attr]

werkzeug (https://github.com/pallets/werkzeug.git)
+ src/werkzeug/http.py:456: error: Unsupported operand types for + ("str" and "None")
+ src/werkzeug/http.py:456: note: Right operand is of type "Optional[str]"
+ src/werkzeug/http.py:458: error: Incompatible types in assignment (expression has type "Optional[str]", target has type "str")

aiohttp (https://github.com/aio-libs/aiohttp.git)
+ aiohttp/client_reqrep.py:809: error: unused "type: ignore" comment
+ aiohttp/client_reqrep.py:813: error: Argument 1 to "add" of "MultiDict" has incompatible type "Union[str, Any, URL]"; expected "Union[str, istr]"  [arg-type]

@srittau

srittau commented May 30, 2021

Copy link
Copy Markdown
Collaborator Author

The primer output looks tolerable in my opinion, but we should wait to merge this to after the next mypy release, though, to minimize disruption.

@Akuli Akuli added the status: deferred Issue or PR deferred until some precondition is fixed label May 30, 2021
@srittau srittau removed the status: deferred Issue or PR deferred until some precondition is fixed label Jun 8, 2021
@srittau

srittau commented Jun 8, 2021

Copy link
Copy Markdown
Collaborator Author

Now that mypy has released, I'd consider this ready for merge.

@Akuli
Akuli merged commit 52dd232 into python:master Jun 8, 2021
@srittau
srittau deleted the re-match branch June 8, 2021 21:50
JukkaL added a commit to python/mypy that referenced this pull request Jul 28, 2021
When trying to match `list[T]` and `list[str] | Any`, previously we
gave up, because the union items caused conflicting inferred values
for `T` (`str` and `Any`). Work around the issue by dropping `Any`
constraints if there are multiple sets of contraints, since we
prefer more precise types.

This fixes false positives resulting from python/typeshed#5557,
which changed some return types to contain unions with `Any` items.
See mypy primer results in #10881 for a real-world example of this
in sphinx.
JukkaL added a commit to python/mypy that referenced this pull request Jul 28, 2021
)

When trying to match `list[T]` and `list[str] | Any`, previously we
gave up, because the union items caused conflicting inferred values
for `T` (`str` and `Any`). Work around the issue by dropping `Any`
constraints if there are multiple sets of contraints, since we
prefer more precise types.

This fixes false positives resulting from python/typeshed#5557,
which changed some return types to contain unions with `Any` items.
See mypy primer results in #10881 for a real-world example of this
in sphinx.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Match.group can return None

2 participants