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

Add support for literal addition #18004

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

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Oct 21, 2024

Add support for addition of Literal values and str, int and bytes expressions. This is especially useful when working with TypedDicts. Previously this code would have emitted a literal-required error.

from typing import Literal, TypedDict

class LookupDict(TypedDict):
    top_var: str
    bottom_var: str
    var: str

def func(d: LookupDict, pos: Literal["top_", "bottom_", ""]) -> str:
    return d[pos + "var"]

Refs #11616

This comment has been minimized.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 21, 2024

Can you look at the new errors reported by mypy_primer? If we infer literal types more eagerly, they could generate new false positives.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Oct 21, 2024

Can you look at the new errors reported by mypy_primer?

The primer results look quite promising IMO. For starlette the issue is something like this:

from typing import Literal

class SessionMiddleware:
    def __init__(
        self,
        same_site: Literal["lax", "strict", "none"] = "lax",
        https_only: bool = False,
    ) -> None:
        ...
        self.security_flags = "httponly; samesite=" + same_site
        if https_only:  # Secure flag can be used with HTTPS only
            self.security_flags += "; secure"  # <-- error

It can be resolved by annotation self.security_flags as str.

--
For AutoSplit the issue looks like this

class CaptureMethodBase:
    description = ""

class BitBltCaptureMethod(CaptureMethodBase):
    description = (
        "\nThe best option when compatible. But it cannot properly record "
        + "\nOpenGL, Hardware Accelerated or Exclusive Fullscreen windows. "
        + "\nThe smaller the selected region, the more efficient it is. "
    )

class ForceFullContentRenderingCaptureMethod(BitBltCaptureMethod):
    description = (
        "\nUses BitBlt behind the scene, but passes a special flag "
        + "\nto PrintWindow to force rendering the entire desktop. "
        + "\nAbout 10-15x slower than BitBlt based on original window size "
        + "\nand can mess up some applications' rendering pipelines. "
    )

Just removing the + symbols and using implicit string concatenation would resolve the issue here.

--

Not completely sure whats going on with trio why the type ignore there is no longer required. However, it's in a test file.
https://github.com/python-trio/trio/blob/3d592af32b4a6dae78525fb5103ac2e58020774a/src/trio/_tests/test_fakenet.py#L185-L189

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 21, 2024

Just removing the + symbols and using implicit string concatenation would resolve the issue here.

I think implicit and explicit string concatenation should behave the same. Otherwise it would seem inconsistent. I also know that some people prefer explicit string concatenation as a style thing, since it's easy to accidentally use implicit string concatenation (e.g. omit a comma at the end).

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 21, 2024

What about inferring a literal addition only if one of the operands is not a literal expression? We have tried to adhere to an informal policy where literal types are not inferred for variables "out of thin air", i.e. an explicit Literal[...], Final or similar needs to be somewhere.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 21, 2024

In particular, this feels inconsistent to me:

class A:
    a = ""

class B(A):
    a = "a" + "b"

class C:
    a = "a" + "b"

reveal_type(A.a)  # Revealed type is "builtins.str"
reveal_type(B.a)  # Revealed type is "Literal['ab']"  (why literal here?)
reveal_type(C.a)  # Revealed type is "builtins.str"

This comment has been minimized.

This comment has been minimized.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Oct 21, 2024

What about inferring a literal addition only if one of the operands is not a literal expression? We have tried to adhere to an informal policy where literal types are not inferred for variables "out of thin air", i.e. an explicit Literal[...], Final or similar needs to be somewhere.

Makes sense. I missed the "a" + "b" case when I was refining the implementation. Should be fixed with the latest commits.

This comment has been minimized.

@brianschubert
Copy link
Collaborator

Related issue: #11616

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.

Did a quick pass, mainly to look at the big picture. I can also test this manually later.

mypy/checkexpr.py Outdated Show resolved Hide resolved

values: list[int | str] = sorted(
{
val[0] + val[1] # type: ignore[operator]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you avoid the type: ignore here? In the future we will probably disallow many ignores in code compiled with mypyc, since some type ignores can cause erratic behavior, such as C compilation errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you avoid the type: ignore here?

Not in an elegant way. The values could be either str or int (for int and bytes). With the comparison of the fallback types we know that left and right values have the same type but not which one. If necessary, it would be possible to split these into two cases with cast and set comprehensions for each one. Don't think that it would be an improvement though.

test-data/unit/check-literal.test Show resolved Hide resolved
reveal_type(d + 'foo') # N: Revealed type is "builtins.str"
reveal_type('foo' + d) # N: Revealed type is "builtins.str"
reveal_type(d + 'bar') # N: Revealed type is "Literal['foobar']"
reveal_type('bar' + d) # N: Revealed type is "Literal['barfoo']"

reveal_type(a.__add__(b)) # N: Revealed type is "builtins.int"
reveal_type(b.__add__(a)) # N: Revealed type is "builtins.int"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check a.__add__(a). It would be nice if it was consistent with a + a, though it's probably not important.

Copy link
Collaborator Author

@cdce8p cdce8p Nov 20, 2024

Choose a reason for hiding this comment

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

The result will be int even if a: Literal[3]. The narrowing is only applied for the a + a case currently. The method call to __add__ is handled by self.check_op as the fallback case which I haven't modified here.

Test case added in 36f5e2e

bytes_a: Literal[b"a"]
bytes_b: Literal[b"b"]
bytes_union_1: Literal[b"a", b"b"]
bytes_union_2: Literal[b"d", b"c"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test union with literal type and non-literal type -- e.g. user-defined class that defines __add__ that accepts str. Also test with union of literal type and Any.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The narrowing is only applied if all Union items are Literals itself. Thus the inference for these cases didn't change.

Test cases added in 36f5e2e

This comment has been minimized.

@cdce8p cdce8p requested a review from JukkaL November 20, 2024 14:29
Copy link
Contributor

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

starlette (https://github.com/encode/starlette)
+ starlette/middleware/sessions.py:34: error: Incompatible types in assignment (expression has type "Literal['httponly; samesite=lax; secure', 'httponly; samesite=none; secure', 'httponly; samesite=strict; secure']", variable has type "Literal['httponly; samesite=lax', 'httponly; samesite=none', 'httponly; samesite=strict']")  [assignment]
+ starlette/middleware/sessions.py:36: error: Incompatible types in assignment (expression has type "str", variable has type "Literal['httponly; samesite=lax', 'httponly; samesite=none', 'httponly; samesite=strict']")  [assignment]

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

Successfully merging this pull request may close these issues.

3 participants