Skip to content

Add initial LiteralString support #13664

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add initial LiteralString support #13664

wants to merge 8 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 14, 2022 •
edited
Loading

I went for the smallest possible diff in this feature.
There are couple of other options I did not want to go with:

  • class LiteralStringType and using it in last_known_value. There are lots of places where .last_known_value is used for literal context checking. In this case we would also need to add new visitor methods to every type visitor, which is way harder
  • Fake LiteralString(str) type info
  • Fake LiteralString with _promote hackery

TODO in the next PRs:

  • Literal type math: Literal type math #11990 This will allow us to automatically support things like x: LiteralString = 'a' + 'b'
  • More tests and use-cases
  • Removing --enable-incomplete-features flag from this feature

Refs #12554

@sobolevn sobolevn requested review from JukkaL, ilevkivskyi and hauntsaninja and removed request for JukkaL and ilevkivskyi September 14, 2022 10:18
@sobolevn
Copy link
Member Author

Current failures show that we need to special case this:

x = 'a'
x = 'Value: {}'.format(x)

@github-actions

This comment has been minimized.

Sign in to view
ilevkivskyi
ilevkivskyi reviewed Sep 14, 2022
View reviewed changes
test-data/unit/check-literal-string.test

expects_literal_string('a')
expects_literal_string('a' + 'b')
expects_literal_string('a' * 2)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to care about this at all? Isn't this just a question of defining these correctly in typeshed? For example:

@overload
def __add__(self: LiteralStr, other: LiteralStr) -> LiteralStr: ...
@overload
def __add__(self, other: str) -> str: ...

It seems to be much easier to agree on this once and for all rather than special-casing all these in all type-checkers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! I think that we should have both.
Typeshed will store the base implementation for it.

But, mypy can go further with full literal type math.
Or we can just use the typeshed if ltm is never merged / implemented.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I see, but as I said below, we need to be sure "typeshed way" will work well. Having "a" + "b" accepted where Literal["ab"] is expected is lower priority IMO.

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@sobolevn
Copy link
Member Author

I consider the very first version done! 🎉
Feedback is more than welcome!

@github-actions

This comment has been minimized.

Sign in to view
@sobolevn
Copy link
Member Author

sobolevn commented Sep 14, 2022 •
edited
Loading

Primer output

sphinx

This is the case for sphinx:

@overload
def join(__a: LiteralString, *paths: LiteralString) -> LiteralString: ...
@overload
def join(__a: StrPath, *paths: StrPath) -> str: ...
@overload
def join(__a: BytesPath, *paths: BytesPath) -> bytes: ...

Looks like I need to add tests with @overload decorators just to be sure.
The case is that one argument is LiteralString, other is str. So, it cannot find an overload: https://github.com/sphinx-doc/sphinx/blob/5.x/sphinx/setup_command.py#L133

psycopg2

It is actually correct:

pyspark

Mypy is also correct here. Not all overloads are covered, string.Formatter has one more: https://github.com/apache/spark/blob/12e48527846d993a78b159fbba3e900a4feb7b55/python/pyspark/pandas/sql_formatter.py#L194

@github-actions

This comment has been minimized.

Sign in to view
1 similar comment
@github-actions

This comment has been minimized.

Sign in to view
@hauntsaninja
Copy link
Collaborator

Currently we patch typeshed... Can we revert #13093 in this PR and see if everything works?

JukkaL
JukkaL reviewed Sep 16, 2022
View reviewed changes
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 for working on this! This is an important feature to have now that 3.11 will be soon out. Not a full review .

test-data/unit/check-literal-string.test
T = TypeVar('T', bound=LiteralString)
def expects_literal_string(x: T): ...

expects_literal_string('a')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe also test something like this:

ls: Literal['a'] = expects_literal_string('a')

test-data/unit/check-literal-string.test
@overload
def some(x: LiteralString, y: LiteralString) -> LiteralString: ...
@overload
def some(x: str, y: str) -> str: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test also a str method with self annotated as self: LiteralString in one overload variant (e.g. for __add__)? This way we should be able to make 'x' + 'y' be a LiteralString without special casing?

Copy link
Member

Choose a reason for hiding this comment

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

I actually tried this and mypy gives an error like Self type is not a supertype of class. I think this is too strict, we already check self-type at call site. I would say we can simply skip this check for overloads (maybe only keep it for overload implementation).

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, I think we still need this test, see also my comment above #13664 (comment)

test-data/unit/fixtures/literal_string.pyi
@@ -0,0 +1,57 @@
# Builtins stub used in tuple-related test cases.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment is out of date.

@ilevkivskyi
Copy link
Member

Currently we patch typeshed... Can we revert #13093 in this PR and see if everything works?

+1 to this idea.

@sobolevn
Copy link
Member Author

I am away for a week, I will return to this shortly after I get home! Thanks for your feedback.

@sobolevn
Copy link
Member Author

Adding this back to my todo list :)

@github-actions
Copy link
Contributor

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

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/setup_command.py:133:43: error: Argument 1 to "join" has incompatible type "str"; expected "LiteralString"  [arg-type]

psycopg (https://github.com/psycopg/psycopg)
+ psycopg/psycopg/_typeinfo.py:93: error: Argument 1 to "execute" of "Cursor" has incompatible type "str"; expected "Union[LiteralString, bytes, SQL, Composed]"  [arg-type]
+ psycopg/psycopg/_typeinfo.py:112: error: Argument 1 to "execute" of "AsyncCursor" has incompatible type "str"; expected "Union[LiteralString, bytes, SQL, Composed]"  [arg-type]
+ psycopg/psycopg/server_cursor.py:170: error: Argument 1 to "SQL" has incompatible type "str"; expected "LiteralString"  [arg-type]
+ psycopg/psycopg/server_cursor.py:179: error: Incompatible types in assignment (expression has type "str", variable has type "Union[LiteralString, bytes, SQL, Composed]")  [assignment]
+ psycopg/psycopg/server_cursor.py:181: error: Argument 1 to "SQL" has incompatible type "Union[LiteralString, bytes]"; expected "LiteralString"  [arg-type]
+ psycopg/psycopg/server_cursor.py:188: error: Argument 1 to "SQL" has incompatible type "str"; expected "LiteralString"  [arg-type]
+ psycopg/psycopg/connection.py:1024: error: Argument 1 to "execute" of "Cursor" has incompatible type "str"; expected "Union[LiteralString, bytes, SQL, Composed]"  [arg-type]
+ psycopg/psycopg/connection_async.py:430: error: Argument 1 to "execute" of "AsyncCursor" has incompatible type "str"; expected "Union[LiteralString, bytes, SQL, Composed]"  [arg-type]
+ tests/scripts/dectest.py:31: error: Argument 1 to "copy" of "Cursor" has incompatible type "str"; expected "Union[LiteralString, bytes, SQL, Composed]"  [arg-type]
+ tests/scripts/dectest.py:40: error: Argument 1 to "executemany" of "Cursor" has incompatible type "str"; expected "Union[LiteralString, bytes, SQL, Composed]"  [arg-type]
+ tests/crdb/test_copy.py:209: error: Argument 1 to "SQL" has incompatible type "str"; expected "LiteralString"  [arg-type]
+ tests/crdb/test_copy_async.py:215: error: Argument 1 to "SQL" has incompatible type "str"; expected "LiteralString"  [arg-type]
+ tests/test_sql.py:466: error: Argument 1 to "SQL" has incompatible type "str"; expected "LiteralString"  [arg-type]
+ tests/test_sql.py:534: error: Argument 1 to "SQL" has incompatible type "str"; expected "LiteralString"  [arg-type]
+ tests/types/test_numeric.py:580: error: Argument 1 to "SQL" has incompatible type "str"; expected "LiteralString"  [arg-type]

spark (https://github.com/apache/spark)
+ python/pyspark/pandas/sql_formatter.py:194: error: Signature of "vformat" incompatible with supertype "Formatter"  [override]
+ python/pyspark/pandas/sql_formatter.py:194: note:      Superclass:
+ python/pyspark/pandas/sql_formatter.py:194: note:          @overload
+ python/pyspark/pandas/sql_formatter.py:194: note:          def vformat(self, format_string: LiteralString, args: Sequence[LiteralString], kwargs: Mapping[LiteralString, LiteralString]) -> LiteralString
+ python/pyspark/pandas/sql_formatter.py:194: note:          @overload
+ python/pyspark/pandas/sql_formatter.py:194: note:          def vformat(self, format_string: str, args: Sequence[Any], kwargs: Mapping[str, Any]) -> str
+ python/pyspark/pandas/sql_formatter.py:194: note:      Subclass:
+ python/pyspark/pandas/sql_formatter.py:194: note:          def vformat(self, format_string: str, args: Sequence[Any], kwargs: Mapping[str, Any]) -> str

@sobolevn
Copy link
Member Author

sobolevn commented Dec 9, 2022

Working on it!

@ilevkivskyi
Copy link
Member

@sobolevn It looks like you may have forgotten about this one :-) It would be great to finally finish this.

@sobolevn
Copy link
Member Author

Yes, I am sorry :(
This got lost in my backlog.

I will try to finish this next week!

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

@JukkaL JukkaL JukkaL left review comments

@ilevkivskyi ilevkivskyi ilevkivskyi left review comments

@hauntsaninja hauntsaninja Awaiting requested review from hauntsaninja

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sobolevn @hauntsaninja @ilevkivskyi @JukkaL