Skip to content
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

Simple in based type-narrowing Literals. #15226

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

alanhdu
Copy link
Contributor

@alanhdu alanhdu commented May 11, 2023

This adds simple support for #9718, where you can use <lh> in <rh> statements to narrow from str/int down into the actual Literal type.

This is a very simple version that only does this narrowing in two cases:

  • <rh> is a literal type (e.g. [1, 2, 3] or {"a", "b", "c"})
  • <rh> is a Tuple of Literals (which gives limited support for Final variables).

I think that should handle the majority of use-cases, even if it doesn't support everything.

I'm definitely a noob with the codebase (for instance, I definitely did not grok the difference between Type and ProperType and just inserted a bunch of casts everywhere) and would be happy to refactor the code.

@alanhdu alanhdu force-pushed the alan/literal-narrow branch from a590fd3 to a946a6f Compare May 11, 2023 19:33
reveal_type(c) # N: Revealed type is "Any"
if d in (1, 2):
reveal_type(d) # N: Revealed type is "__main__.A"
[builtins fixtures/ops.pyi]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no clue which fixtures to use, so just chose something that seemed to work.

from typing_extensions import Literal

def narrow(x: int, y: str, z: Literal[1, 2, 3]):
if x in (1, 2, "a", "b"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally I'd like to also test that this works for list constructors ([1, 2]) and set constructors ({"a", "b"}) but could not figure out how to get hte fixtures to play nicely with that.

Choose a reason for hiding this comment

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

As a relatively uninformed outsider - if you were to adjust this test to use if x == 1 or x == 2 or x == "a" or x == "b": will it narrow identically to what you have here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it would not work. I've only handled in expressions in this PR -- dealing with == checks is not supported in this PR.

Copy link

@gandhis1 gandhis1 May 12, 2023

Choose a reason for hiding this comment

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

Wouldn't you agree that inconsistent narrowing semantics with == could be confusing to users of Mypy?

As an even simpler example:

def test_narrowing(x: int) -> None:
    if x in (1,):
        reveal_type(x)  # note: Revealed type is "builtins.int" (but presumably would be "Literal[1] after this PR)
    
    if x == 1:
        reveal_type(x)  # note: Revealed type is "builtins.int"

For what it's worth, Pyright does this correctly and consistently:

def test_narrowing(x: int) -> None:
    if x in (1,):
        reveal_type(x)  # Type of "x" is "Literal[1]"
    
    if x == 1:
        reveal_type(x)  # Type of "x" is "Literal[1]"

Copy link
Contributor Author

@alanhdu alanhdu May 12, 2023

Choose a reason for hiding this comment

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

Wouldn't you agree that inconsistent narrowing semantics with == could be confusing to users of Mypy?

Not particularly, no. It's just a missing feature like any other (and there are plenty of other feature gaps I could point to), so I don't see why we'd block this PR on adding such a feature.

I'm not saying that "someone" shouldn't do implement ==-based typing, but I'm saying that I'm not planning on doing it in this PR. The relevant changes didn't look trivial to make, and we can always add it later. If I'm wrong and it is trivial, please leave a pointer to the relevant code correcting my understanding!

mypy/checker.py Outdated
Comment on lines 5634 to 5649
yes, no = self.conditional_types_with_intersection(
item_type, [TypeRange(ty, is_upper_bound=False)], node
)
# Intersection caluclates UninhabitedType too
# aggressively -- ignore them for now
if yes is not None and not isinstance(
get_proper_type(yes), UninhabitedType
):
if_map[operands[left_index]] = yes
if no is not None and not isinstance(
get_proper_type(no), UninhabitedType
):
else_map[operands[left_index]] = no
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if there's something else I should do here. I found that the UninhabitedType is often too aggressive (e.g. if item_type=float and ty=Literal[1], then yes = UninhabitedType. While that's technically true (in that float and Literal[1] are incompatible types), at runtime that code can still run.

options: list[LiteralType] = []
is_literal: bool = True

iterable_type = get_proper_type(iterable_type)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt these isinstance checks are what we want to be doing (since they force the get_proper_type calls everywhere), but I wasn't sure what a better alternative is.

Copy link
Contributor Author

@alanhdu alanhdu left a comment

Choose a reason for hiding this comment

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

General questions / areas that I thought weren't implemented nicely.

@github-actions

This comment has been minimized.

@alanhdu
Copy link
Contributor Author

alanhdu commented May 11, 2023

Oof... that's a very neat tool, but I'll take a look at the crashes at least to debug.

@alanhdu
Copy link
Contributor Author

alanhdu commented May 11, 2023

Ok -- that should fix the crashes (I forgot that (*args, 1, 2) was valid syntax for Python tuples).

It seems like most of the error problems are about unification. In the aiohttp one, for instance we have this error

"dict[Literal['http', 'https', 'ws', 'wss'], ProxyInfo]", expected "dict[str, ProxyInfo]"

which comes from L298 in this function which conceptually looks something like:

def proxies_from_env() -> Dict[str, ProxyInfo]:
    proxy_urls = {
        k: URL(v)
        for k, v in getproxies().items()
        if k in ("http", "https", "ws", "wss")
    }
    ret = {k: some_function_of(v) for k, v in proxy_urls.items()}
    return ret

Is there some way mypy to assign ret the "broader" type rather than the more specific one? One conceptual solution is to try and remember the "provenance" of the type (specifically whether it was inferred or explicitly annotated) and automatically broaden an inferred type? TBH I'm not sure what's best here (any way I think about it seems to violate edge-cases).

@github-actions

This comment has been minimized.

mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
Co-authored-by: Ilya Priven <ilya.konstantinov@gmail.com>
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

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

mitmproxy (https://github.com/mitmproxy/mitmproxy)
+ mitmproxy/net/server_spec.py:84: error: Unused "type: ignore" comment  [unused-ignore]

pandas (https://github.com/pandas-dev/pandas)
+ pandas/io/parsers/arrow_parser_wrapper.py:102: error: Invalid index type "Literal['strings_can_be_null']" for "dict[Literal['include_columns', 'null_values', 'true_values', 'false_values', 'decimal_point', 'timestamp_parsers'], Any]"; expected type "Literal['include_columns', 'null_values', 'true_values', 'false_values', 'decimal_point', 'timestamp_parsers']"  [index]
+ pandas/core/resample.py:2110: error: Unused "type: ignore" comment  [unused-ignore]
+ pandas/_testing/__init__.py:670: error: Argument 1 to "get" of "dict" has incompatible type "Literal['i', 'f', 's', 'u', 'dt', 'p', 'td'] | None"; expected "str"  [arg-type]
+ pandas/tests/extension/test_masked.py:281: error: Item "str" of "Literal['Int64', 'UInt64']" has no attribute "name"  [union-attr]

python-chess (https://github.com/niklasf/python-chess)
+ chess/engine.py:1747: error: Unused "type: ignore" comment  [unused-ignore]

pylint (https://github.com/pycqa/pylint)
+ pylint/utils/linterstats.py:208: error: Redundant cast to "Literal['argument', 'attr', 'class', 'class_attribute', 'class_const', 'const', 'inlinevar', 'function', 'method', 'module', 'variable', 'typevar', 'typealias']"  [redundant-cast]

paasta (https://github.com/yelp/paasta)
+ paasta_tools/kubernetes_tools.py:610: error: Incompatible return value type (got "List[Tuple[str, Literal['In', 'NotIn', 'Exists', 'DoesNotExist', 'Gt', 'Lt'], Any]]", expected "List[Tuple[str, str, List[str]]]")  [return-value]
+ paasta_tools/kubernetes_tools.py:610: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
+ paasta_tools/kubernetes_tools.py:610: note: Consider using "Sequence" instead, which is covariant
+ paasta_tools/kubernetes_tools.py:610: note: Perhaps you need a type annotation for "requirements"? Suggestion: "List[Tuple[str, str, List[str]]]"

aioredis (https://github.com/aio-libs/aioredis)
+ aioredis/client.py:1810: error: Argument 1 to "append" of "list" has incompatible type "int"; expected "Literal[0, 1] | bytes | str | memoryview"  [arg-type]
+ aioredis/client.py:1812: error: Argument 1 to "append" of "list" has incompatible type "int"; expected "Literal[0, 1] | bytes | str | memoryview"  [arg-type]

zetta_utils (https://github.com/ZettaAI/zetta_utils)
+ zetta_utils/tensor_ops/common.py:157: error: Unused "type: ignore" comment  [unused-ignore]

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/ext/viewcode.py: note: In function "collect_pages":
+ sphinx/ext/viewcode.py:270:21: error: Incompatible types in assignment (expression has type "Literal['python']", variable has type "Literal['default', 'none']")  [assignment]

pycryptodome (https://github.com/Legrandin/pycryptodome)
+ lib/Crypto/SelfTest/Signature/test_dss.py:334: error: Incompatible types in assignment (expression has type "None", variable has type "str")  [assignment]

operator (https://github.com/canonical/operator)
+ ops/model.py:3275: error: Redundant cast to "Literal['tcp', 'udp']"  [redundant-cast]

spark (https://github.com/apache/spark)
+ python/pyspark/mllib/clustering.py:1059: error: Incompatible types in assignment (expression has type "str", variable has type "Literal['batches', 'points']")  [assignment]

jax (https://github.com/google/jax)
+ jax/_src/random.py:374: error: Incompatible types in assignment (expression has type "Literal[8]", variable has type "Literal[16, 32, 64]")  [assignment]

alerta (https://github.com/alerta/alerta)
+ alerta/utils/logging.py:35: error: Incompatible types in assignment (expression has type "Literal['custom']", variable has type "Literal['default', 'simple', 'verbose', 'json', 'syslog']")  [assignment]

pydantic (https://github.com/samuelcolvin/pydantic)
- pydantic/v1/types.py:702: error: Unsupported operand types for >= ("Literal['n']" and "int")  [operator]
- pydantic/v1/types.py:702: error: Unsupported operand types for >= ("Literal['N']" and "int")  [operator]
- pydantic/v1/types.py:702: error: Unsupported operand types for >= ("Literal['F']" and "int")  [operator]
- pydantic/v1/types.py:702: note: Left operand is of type "Literal['n', 'N', 'F'] | int"
- pydantic/v1/types.py:704: error: Unsupported operand types for + ("int" and "Literal['n']")  [operator]
- pydantic/v1/types.py:704: error: Unsupported operand types for + ("int" and "Literal['N']")  [operator]
- pydantic/v1/types.py:704: error: Unsupported operand types for + ("int" and "Literal['F']")  [operator]
- pydantic/v1/types.py:704: note: Right operand is of type "Literal['n', 'N', 'F'] | int"
- pydantic/v1/types.py:712: error: Argument 1 to "abs" has incompatible type "Literal['n', 'N', 'F'] | int"; expected "SupportsAbs[int]"  [arg-type]
- pydantic/v1/types.py:713: error: Argument 1 to "abs" has incompatible type "Literal['n', 'N', 'F'] | int"; expected "SupportsAbs[int]"  [arg-type]
- pydantic/v1/types.py:716: error: Argument 1 to "abs" has incompatible type "Literal['n', 'N', 'F'] | int"; expected "SupportsAbs[int]"  [arg-type]

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/helpers.py:292:12: error: Incompatible return value type (got "dict[Literal['http', 'https', 'ws', 'wss'], ProxyInfo]", expected "dict[str, ProxyInfo]")  [return-value]
+ aiohttp/helpers.py:292:12: note: Perhaps you need a type annotation for "ret"? Suggestion: "dict[str, ProxyInfo]"
+ aiohttp/web_server.py:79:20: error: Keywords must be strings  [misc]

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.

4 participants