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 tests for literals and generics #6035

Merged

Conversation

Michael0x2a
Copy link
Collaborator

@Michael0x2a Michael0x2a commented Dec 9, 2018

This pull request adds some tests verifying that basic interactions between literals and generics work as expected.

It also tweaks checkexpr so it adds another exception for type variable return vs a Literal[...] context.

This pull request adds some tests verifying that basic interactions
between literals and generics work as expected.

It also mildly tweaks checkexpr so it actually considers the *whole*
type context instead of just part of it when deciding whether some
literal expression ought to be a Literal[...] type or not.
@Michael0x2a Michael0x2a mentioned this pull request Dec 9, 2018
42 tasks
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Mostly looks good. I have just one question.

@@ -3346,6 +3346,10 @@ def narrow_type_from_binder(self, expr: Expression, known_type: Type) -> Type:
return ans
return known_type

def context_contains_literal_like_type(self) -> bool:
"""Returns 'true' if the context contains anything that resembles a LiteralType"""
return any(is_literal_type_like(item) for item in self.type_context)
Copy link
Member

Choose a reason for hiding this comment

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

Why are you checking all items in the type context? I think only type_context[-1] should be checked.

Maybe add a test that checks that in this case literal type for 42 is not inferred:

def foo(x: T) -> T: ...
x: Union[List[int], Literal['bad']] = foo([42])

(i.e. this example should type-check without 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.

I made that change to support this case:

def expects_literal(x: Literal[3]) -> None: pass
def foo(x: T) -> T: ...
expects_literal(foo(3))  # Should not have an error

I found that without checking the entire context, we got an error on that final line since type_context[-1] is T, not Literal[3].

Maybe I could modify context_contains_literal_like_type so that it starts by checking type_context[-1] and only tries moving backwards if type_context[-1] happened to be a TypeVar or something?

Or alternatively, modify this method so that it only tries inferring a Literal if the fallback matches -- so since 42 is an int and Literal['bad']'s fallback is str, we wouldn't error in your example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But what if the return type of foo is something other than T? Then it might not be correct to infer a literal type, right? I suspect that accessing type context below the top item does not really make sense, since we don't know what's "between" the type context items.

Copy link
Member

Choose a reason for hiding this comment

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

@Michael0x2a The outer context doesn't propagate inside in your example because there is special casing for functions that return a plain type variable, for them, only generic instance context is used for inference. If you really want your example to work, then you can add or isinstance(ctx, LiteralType) (the current check is anyway pure heuristic). As @JukkaL explained, checking all context stack doesn't really make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JukkaL and @ilevkivskyi -- ok, I updated the PR. I ended up using basically the fix Ivan suggested (and was pleasantly surprised to discover this seemed to fix all but one minor edge case).

test-data/unit/check-literal.test Outdated Show resolved Hide resolved
@Michael0x2a
Copy link
Collaborator Author

Windows test failure appears to be a flake.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good now. If necessary, we can adjust these heuristics later, when we will have more experience with literals in large code-bases.

@ilevkivskyi ilevkivskyi merged commit cf25319 into python:master Dec 18, 2018
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.

3 participants