Skip to content

Test is_proper_subtype with string literals #14011

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Nov 5, 2022

This part is puzzling me:

lit_foo = LiteralType("foo", fx.str_type)
lit_bar = LiteralType("bar", fx.str_type)
lit_foo_context_sensitive = Instance(fx.str_type_info, [], last_known_value=lit_foo)
assert is_proper_subtype(lit_bar, lit_foo_context_sensitive)

Why is Literal['bar'] a subtype of Literal['Foo']??

@ikonst ikonst changed the title add test_is_proper_subtype_literal_str_types Test is_proper_subtype with string literals Nov 5, 2022
@ikonst
Copy link
Contributor Author

ikonst commented Nov 5, 2022

cc @ilevkivskyi being is_subtype's author :)

@ilevkivskyi
Copy link
Member

last_known_value should be ignored by is_subtype() and other type ops (join, meet, etc). It is an internal thing used to decide whether to infer a literal or other type depending on context where an expression appears. So lit_foo_context_sensitive is just an instance.

Btw I noticed that it is used currently in one place in is_subtype(). It looks like what @sobolevn did in #11236 is a bad hack, because it breaks transitivity of subtyping. Now Literal[1] <: Literal[2]? and Literal[2]? <: Literal[2], which is bad.

(Also this ? string repr creates confusion that it is kind of a literal, while it is actually an Instance.)

cc @JukkaL who approved the PR.

@ilevkivskyi
Copy link
Member

Fortunately, it looks like this issue doesn't leak to the user, since Literal[...]? should never appear in a supertype position (it should be erased on inference and is not expressible by user). We could potentially plug this hole by adding an assert in is_subtype() (e.g. in visit_literal_type()) that when Instance appears on the right, it always has last_known_value is None. I however tried this locally and it caused few crashes (because of is_same_type()).

@ikonst
Copy link
Contributor Author

ikonst commented Nov 12, 2022

Thanks for the thorough explanation.

My rationale is that I've been experimenting with narrowing expressions that are operands of ==. trying to leverage is_overlapping_types etc. I think the biggest surprise to me was that "a" is Literal["a"]? rather than Literal["a"]. I suppose this is so that on assignment it'd type a variable as Literal["a"]? and it can be still passed into a Literal["a"]...

Was this taken as a shortcut, or is there some principle I'm missing?

(Also this ? string repr creates confusion that it is kind of a literal, while it is actually an Instance.)

Would it break too many things to replace that repr?

@ilevkivskyi
Copy link
Member

I don't remember all details, but IIRC last_known_value was introduced to allow using Final both in literal and other contexts. For example

v: Final = 1
def foo(x: Literal[1, 2]) -> None: ...
foo(v)  # OK
def bar(x: list[int]) -> None: ...
w = [v]
bar(w)  # OK

Note that we can't infer type of v as just Literal[1], because then last call would fail to type-check (list is invariant). For the same reason type of plain literal is inferred as an instance (with a last_known_value), and not a Literal, otherwise lst = [1, 2]; bar(lst) would fail to type-check.

Would it break too many things to replace that repr?

You will probably need to update like 40-50 tests, also

$ grep -n -E "Literal\[.*\]\?" -r test-data | grep -v "\.pyc" | wc -l
138

But in general it will be OK. I would prefer something like builtins.int(value=1). cc @Michael0x2a who may have some opinion on this.

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