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

Rework how we refine information from callable() #4343

Merged
merged 9 commits into from Dec 19, 2017

Conversation

Projects
None yet
4 participants
@msullivan
Collaborator

msullivan commented Dec 12, 2017

Don't ever assume that something is not callable; basically anything
could be, and getting it wrong leaves code to not be typechecked.

When operating on an Instance that is not clearly callable, construct
a callable version of it by generating a fake subclass with a __call__
method to be used.

Fixes #3605

This is being posted to solicit feedback, I expect it might need some large changes.
The hack perpetrated here is somewhat grevious, and I suspect I missed something, so I am interested to find out what :).
Also, to reduce the change size during development I threaded a TypeChecker through a bunch of functions; the right thing might be to move all of those functions to be TypeChecker methods.

@msullivan msullivan requested review from JukkaL and gvanrossum Dec 12, 2017

if not callable(b):
5 + 'test'
if callable(c):
reveal_type(c) # E: Revealed type is '__main__.B'
reveal_type(c) # E: Revealed type is 'Union[<callable subtype of A>, __main__.B]'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 12, 2017

Collaborator

Just a random comment: we need to be sure that this type <callable subtype of A> will never appear in error messages (apart from actually reveal_type), since this can be confusing. I think it can be formatted just as A in other cases

This comment has been minimized.

@msullivan

msullivan Dec 12, 2017

Collaborator

Does there exist mechanism for having different internal and external names for instances?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 12, 2017

Collaborator

Yes, see messages.py. There is a method MessageBuilder.format that controls most of the formatting. But, see other comments for this code that propose more radical solutions.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Dec 12, 2017

the right thing might be to move all of those functions to be TypeChecker methods.

I remember I also wanted this few times.

@JukkaL

This is a reasonable first try at this! I didn't do a detailed review yet but left some comments.

if not callable(b):
5 + 'test'
if callable(c):
reveal_type(c) # E: Revealed type is '__main__.B'
reveal_type(c) # E: Revealed type is 'Union[<callable subtype of A>, __main__.B]'

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

Hmm this might be pretty surprising for users. I wonder if we should special case unions and only consider items with a known __call__ here, so that the inferred type would continue to be just __main__.B here. Yes, it would be ugly, but I'd say that pragmatism beats purity here, since the whole callable check is a legacy thing.

Random though: One way of thinking about this would be to consider this as similar to hasattr(c, '__call__'). We could perhaps eventually extend the support for callable to handle hasattr, and for hasattr I feel that__main__.B would be the correct type to infer here.

This comment has been minimized.

@gvanrossum

gvanrossum Dec 12, 2017

Member

I think nearly all users will be surprised by this. And I think in actual code it is very rare to find a situation where c (defined as Union[A, B]) was an instance of a callable subclass of A. We may see the occasional non-callable class that has a callable subclass, but it's unlikely to figure in type discriminations like this. The realistic scenarios probably involve either a union of callable and non-callable classes, where there is no concern that one of non-callable classes has a callable subclass; or a non-callable class that has a callable subclass (but then the discrimination is just between the base class and the callable subclass).

if callable(o):
o()
1 + 'boom' # E: Unsupported operand types for + ("int" and "str")
o()

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

Maybe test calling with some randon arguments? Also consider doing reveal_type(o).

def g(o: Foo) -> None:
o.bar()
if callable(o):
o.bar()

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

Check that o.foo() is rejected here.

o.bar()
if callable(o):
o.bar()
o()

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

Again, also test call with arguments.

return Instance(info, [])
def make_fake_callable(type: Instance, typechecker: TypeChecker) -> Type:

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

Passing TypeChecker instances around is against the prevailing style in the codebase. Instead, it looks like you could just pass a callable corresponding to typechecker.named_type. However, other changes may introduce additional dependencies to TypeChecker and it may be better to turn this and other functions into type checker methods.

@@ -227,7 +227,7 @@ T = Union[Union[int, Callable[[], int]], Union[str, Callable[[], str]]]
def f(t: T) -> None:
if callable(t):
reveal_type(t()) # E: Revealed type is 'Union[builtins.int, builtins.str]'
reveal_type(t()) # E: Revealed type is 'Union[Any, builtins.int, builtins.str]'

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

Similar to above, if we special cased unions this would remain as previously, which arguably matches the likely intent of the check.

@@ -253,7 +253,7 @@ T = TypeVar('T', int, Callable[[], int], Union[str, Callable[[], str]])
def f(t: T) -> None:
if callable(t):
reveal_type(t()) # E: Revealed type is 'builtins.int' # E: Revealed type is 'builtins.str'
reveal_type(t()) # E: Revealed type is 'Any' # E: Revealed type is 'builtins.int' # E: Revealed type is 'Union[Any, builtins.str]'

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

This change seems okay, but can you split the long line using \?

@@ -2883,7 +2883,45 @@ def conditional_type_map(expr: Expression,
return {}, {}
def partition_by_callable(type: Type) -> Tuple[List[Type], List[Type]]:
def intersect_instance_callable(type: Instance, callable_type: CallableType) -> Type:

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

Style nit: we generally prefer typ instead of type since type is a builtin :-( This doesn't apply to attribute access, since it's clear that foo.type doesn't refer to the builtin.

# Build the fake ClassDef and TypeInfo together.
# The ClassDef is full of lies and doesn't actually contain a body.
cdef = ClassDef("<callable subtype of {}>".format(type.type.name()), Block([]))

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2017

Collaborator

This is the trickiest thing in this PR. However, there generally seems to be no better way.

There are several things that should be handled here, such as:

  • The original class may be generic.
  • The original class may inherit from Tuple[...] and in that case we should return TupleType, not Instance.
  • If the original class inherits from Any, the new class should also do that.

Basically you should check that every TypeInfo attribute is initialized correctly.

You may be able to share code with basic_new_typeinfo in mypy.semanal, but you shouldn't add a dependency from type checker to the semantic analyzer. You could move basic_new_typeinfo to some third module, perhaps.

Additionally, it's possible to leak these types through type inference to class and module top level scopes. This means that the types can be serialized in incremental mode, and you should test that it works. It may be necessary to store the created class in a symbol table (with a generated unique dummy name) for serialization to work, but I'm not sure about this.

@@ -208,13 +208,13 @@ b = B() # type: B
c = A() # type: Union[A, B]
if callable(a):
5 + 'test'
5 + 'test' # E: Unsupported operand types for + ("int" and "str")

This comment has been minimized.

@gvanrossum

gvanrossum Dec 12, 2017

Member

Nit of nits: our coding standard has two spaces before #.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Dec 12, 2017

@JukkaL

This comment has been minimized.

Collaborator

JukkaL commented Dec 13, 2017

On second thought, creating a new subclass might be bit of an overkill considering that this is not a very common use case. Here's an idea about a simpler "good enough" fix:

  • Only do anything with callable if the target type is a union. For unions, use the existing approach to narrow them down.
  • If the target is not a union, ignore the callable check -- the original type stays unmodified. This will fix the false negatives but can generate unwanted errors, but that's acceptable.
@msullivan

This comment has been minimized.

Collaborator

msullivan commented Dec 13, 2017

Oops, I wish I had noticed your last comment this morning.
That seems like a reasonable behavior, though this is somewhat nicer behavior to provide?

Pushed changes to address most of the comments. I still need to move the functions into the TypeChecker class, but I've put that off to keep the diffs easy to read.

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Dec 14, 2017

Another way to structure this would be to return just a Callable from conditional_callable_type_map and push the generation of the intersecting class into narrow_declared_type. That would be a start towards implementing make_fake_intersection as discussed in #3603.

@JukkaL

Thanks for the updates! This is looking pretty good (once you refactor the TypeChecker arguments away).

The following TypeInfo attributes might still need processing, though some of these are kind of unlikely to have an effect so just adding a TODO comment may suffice:

  • declared_metaclass
  • metaclass_type
  • is_abstract (say, if one does type(x)())
  • is_protocol (not sure about this)
  • runtime_protocol (if one does isinstance(y, type(x)) which would be pretty perverse)
  • protocol_members (not sure)
  • is_enum (if it's an enum)
  • _promote (you can test by doing if callable(x) when x: int and checking that the narrowed type is compatible with float)
  • tuple_type (discussed previously)
def intersect_instance_callable(type: Instance, callable_type: CallableType,
typechecker: TypeChecker) -> Type:
"""Creates a fake type that represents the intersection of an

This comment has been minimized.

@JukkaL

JukkaL Dec 14, 2017

Collaborator

Style nit: Format multi-line docstrings so that there is an empty lines after the first one and the final """" is on a line by its own. Example:

def foo(x):
    """Do stuff (a 1-line summary).

    More elaborate description of stuff. This
    can span multiple lines.
    """
o.bar()
[builtins fixtures/callable.pyi]
[case testCallableObjectAny]

This comment has been minimized.

@JukkaL

JukkaL Dec 14, 2017

Collaborator

Style nit: Add empty line before each test case.

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Dec 14, 2017

I think the is_abstract and protocol related fields aren't important. Doing type(x)() wouldn't try creating the abstract class, it would create the concrete class that x actually is. Similar with isinstance checks for a protocol type. The other stuff I think is now all either computed or correctly left blank. _promote being set in a parent class seems to do the right thing?

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Dec 14, 2017

There are certain checks for protocols. For example, isinstance(x, P) is prohibited if P is a protocol (unless it is decorated with @runtime).

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Dec 14, 2017

Right, but the concern here was isinstance(y, type(x)) where x is a protocol---but this isn't a problem, because type(x) is a regular concrete type.

@JukkaL

JukkaL approved these changes Dec 18, 2017

LGTM! Just one minor nit. Feel to merge once you've fixed the typo and tested this against internal Dropbox codebases.

# Build the fake ClassDef and TypeInfo together.
# The ClassDef is full of lies and doesn't actually contain a body.
# Use format_bare to generate a nice name for error messages.
# We skip filling fully filling out a handful of TypeInfo fields because they

This comment has been minimized.

@JukkaL

JukkaL Dec 18, 2017

Collaborator

Typo: 'filling fully filling out'.

msullivan added some commits Dec 12, 2017

Rework how we refine information from callable()
Don't ever assume that something is *not* callable; basically anything
could be, and getting it wrong leaves code to not be typechecked.

When operating on an Instance that is not clearly callable, construct
a callable version of it by generating a fake subclass with a __call__
method to be used.

Fixes #3605

@msullivan msullivan merged commit 38337a8 into master Dec 19, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gvanrossum gvanrossum deleted the callable_func_bad branch Dec 21, 2017

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Dec 21, 2017

🥇

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