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.

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

Ensure we always infer a valid fallback type for lambda callables #13576

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Ensure we always infer a valid fallback type for lambda callables #13576

merged 1 commit into from
Sep 5, 2022

Conversation

Michael0x2a
Copy link
Collaborator

Fixes #9234

This diff fixes a bug in infer_lambda_type_using_context where it blindly trusted and reused whatever fallback the context callable was using.

This causes mypy to crash in the case where the context was a dynamic constructor. This is because...

  1. The constructor has a fallback of builtins.type
  2. The Callable object infer_lambda_type_using_context returns uses this fallback as-is.
  3. The join of the LHS and RHS of the ternary ends up being a def (Any) -> Any with a fallback of builtins.type. See:

    mypy/mypy/join.py

    Line 578 in 7ffaf23

    def combine_similar_callables(t: CallableType, s: CallableType) -> CallableType:
  4. Later, we call CallableType.is_type_obj() and CallableType.type_object(). The former ends up succeeding due to the fallback, but the latter fails an assert because the return type is Any, not an Instance:

    mypy/mypy/types.py

    Line 1771 in 7ffaf23

    def is_type_obj(self) -> bool:

I opted to fix this by modifying infer_lambda_type_using_context so it overrides the fallback to always be builtins.function -- I don't think it makes sense for it to be anything else.

@Michael0x2a Michael0x2a changed the title Make infer_lambda_type_using_context always set lambda fallback Ensure we always infer a valid fallback type for lambda callables Sep 1, 2022
@github-actions

This comment has been minimized.

Sign in to view
@Michael0x2a
Make infer_lambda_type_using_context always set lambda fallback
09c1371 
Fixes python#9234

This diff fixes a bug in `infer_lambda_type_using_context` where
it blindly trusted and reused whatever fallback the context callable
was using.

This causes mypy to crash in the case where the context was a dynamic
constructor. This is because...

1.  The constructor has a fallback of `builtins.type`
2.  The `infer_lambda_type_using_context` returns a CallableType
    with this fallback.
3.  The join of the LHS and RHS of the ternary ends up being a
    `def (Any) -> Any` with a fallback of `builtins.type` -- see
    https://github.com/python/mypy/blob/7ffaf230a3984faaf848fe314cf275b854a0cdb0/mypy/join.py#L578
4.  Later, we call `CallableType.is_type_obj()` and
    `CallableType.type_object()`. The former is supposed to be a guard
    for the former, but what happens instead is that the former succeeds
    due to the fallback and the latter fails an assert because the
    return type is Any, not an Instance:
    https://github.com/python/mypy/blob/7ffaf230a3984faaf848fe314cf275b854a0cdb0/mypy/types.py#L1771

I opted to fix this by modifying `infer_lambda_type_using_context` so
it overrides the fallback to always be `builtins.function` -- I don't
think it makes sense for it to be anything else.
@Michael0x2a Michael0x2a force-pushed the fix-lambda-constructor-join branch from 9948b6b to 09c1371 Compare September 2, 2022 13:00
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

JukkaL
JukkaL approved these changes Sep 5, 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.

LGTM

@JukkaL JukkaL merged commit ad56164 into python:master Sep 5, 2022
@JukkaL
Copy link
Collaborator

JukkaL commented Sep 5, 2022

And thanks for the detailed explanation -- it made it very easy to review the PR with confidence.

Michael0x2a added a commit to Michael0x2a/mypy that referenced this pull request Sep 12, 2022
@Michael0x2a
Fix joining a function against metaclass-using object constructors
24d8733 
This pull request fixes python#9838.

It turns out that when an object is using a metaclass, it uses
that metaclass as the fallback instead of `builtins.type`.

This caused the `if t.fallback.type.fullname != "builtins.type"`
check we were performing in `join_similar_callables` and
`combine_similar_callables` to pick the wrong fallback in the case
where we were attempting to join a function against a constructor
for an object that used a metaclass.

This ended up causing a crash later for basically the exact same
reason python#13576 caused a crash: using `abc.ABCMeta` causes
`Callable.is_type_obj()` to return true, which causes us to enter
a codepath where we call `Callable.type_object()`. But this function
is not prepared to handle the case where the return type of the callable
is a Union, causing an assert to fail.

I opted to fix this by adjusting the join algorithm so it does
`if t.fallback.type.fullname == "builtins.function"`.

One question I did punt on -- what should happen in the case where
one of the fallbacks is `builtins.type` and the other is a metaclass?

I suspect it's impossible for this case to actually occur: I think
mypy would opt to use the algorithm for joining two `Type[...]` entities
instead of these callable joining algorithms. While I'm not 100% sure of
this, the current approach of just arbitrarily picking one of the two
fallbacks seemed good enough for now.
@Michael0x2a Michael0x2a mentioned this pull request Sep 12, 2022
Merged
Michael0x2a added a commit that referenced this pull request Sep 25, 2022
@Michael0x2a
Fix joining a function against metaclass-using object constructors (#…
f5ce4ee 
…13648)

This pull request fixes #9838.

It turns out that when an object is using a metaclass, it uses that
metaclass as the fallback instead of `builtins.type`.

This caused the `if t.fallback.type.fullname != "builtins.type"` check
we were performing in `join_similar_callables` and
combine_similar_callables` to pick the wrong fallback in the case where
we were attempting to join a function against a constructor for an
object that used a metaclass.

This ended up causing a crash later for basically the exact same reason
discussed in #13576: using `abc.ABCMeta` causes `Callable.is_type_obj()`
to return true, which causes us to enter a codepath where we call
`Callable.type_object()`. But this function is not prepared to handle
the case where the return type of the callable is a Union, causing an
assert to fail.

I opted to fix this by adjusting the join algorithm so it does `if
t.fallback.type.fullname == "builtins.function"`.

One question I did punt on -- what should happen in the case where one
of the fallbacks is `builtins.type` and the other is a metaclass?

I suspect it's impossible for this case to actually occur: I think mypy
would opt to use the algorithm for joining two `Type[...]` entities
instead of these callable joining algorithms. While I'm not 100% sure of
this, the current approach of just arbitrarily picking one of the two
fallbacks seemed good enough for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JukkaL JukkaL JukkaL approved these changes

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

Successfully merging this pull request may close these issues.

internal error when inferring type of callable resulting from untyped ternary expression
2 participants
@Michael0x2a @JukkaL