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

Is it sus that fake unreachable blocks aren't type checked? #11511

Closed
KotlinIsland opened this issue Nov 10, 2021 · 16 comments
Closed

Is it sus that fake unreachable blocks aren't type checked? #11511

KotlinIsland opened this issue Nov 10, 2021 · 16 comments
Labels
bug mypy got something wrong

Comments

@KotlinIsland
Copy link
Contributor

KotlinIsland commented Nov 10, 2021

def foo(i: float) -> None:
    if not isinstance(i, float):
        i.AMONGUS  # SUS ALERT!

foo(1)

AttributeError: 'int' object has no attribute 'AMONGUS'

@KotlinIsland KotlinIsland added the bug mypy got something wrong label Nov 10, 2021
@SwagatSBhuyan
Copy link

Nice one, lol.
Could reproduce the bug though (Don't even know if I can call this a bug).

@sobolevn
Copy link
Member

sobolevn commented Nov 10, 2021

This is an "int / float" subtyping glitch (still not sure if we can call this a bug).

Normally this would either be:

  1. Argument error when calling this with different argument type
  2. Attibute error when using i.AMONGUS
  3. Unreachable error on i.AMONGUS

I will see if there's anything I can do.

@KotlinIsland
Copy link
Contributor Author

not sure if we can call this a bug

There isn't a 'glitch' issue category 🤣

@SwagatSBhuyan
Copy link

not sure if we can call this a bug

There isn't a 'glitch' issue category 🤣

Any heads up as to where I can start looking for the relevant code? Again, I'm new here. Not exactly my position to state this as 'not a bug' lol.

@sobolevn
Copy link
Member

There isn't a 'glitch' issue category
Not exactly my position to state this as 'not a bug' lol.

Sorry for the confusion! I need to explain better what I meant.
int and float during runtime are not subtypes. But, they are kinda similar.
And most of the time you can use float when int is expected and vice a versa.

Since mypy uses nominal subtyping, it was decided to create new "fake" type hierarchy for float, int, and bool during type checking. Showcase, runtime:

>>> int.mro()
[<class 'int'>, <class 'object'>]
>>> float.mro()
[<class 'float'>, <class 'object'>]
>>> bool.mro()
[<class 'bool'>, <class 'int'>, <class 'object'>]

Typechecking:

float -> int -> bool

So, all these types can still be used just the way we actually use them. This is kinda special.

But, we still have a narrowing problem with float in this example. Why?
Because runtime behavior and mypy's type hierarchy do not match.

When we run this with --warn-unreachable, we can see that the narrowed type under if is <nothing>:

def foo(i: float) -> None:
    if not isinstance(i, float):
        i.AMONGUS  # E: Statement is unreachable

But, in runtime - this is reachable.

The only thing we can do here is to special case narrowing of float / int / bool. That's what I originally meant 🙂
And that's exactly what I am trying to do now 👍

@KotlinIsland
Copy link
Contributor Author

KotlinIsland commented Nov 10, 2021

Yeah, I know the hot mess of float int duck typing

i: float = 1
i.is_integer

😬😬😬😬😬😬😬

@KotlinIsland
Copy link
Contributor Author

KotlinIsland commented Nov 10, 2021

I'd agree that special casing reachability of float is the best way to handle this situation, but honestly I don't see it as all that big of an issue.

@sobolevn
Copy link
Member

sobolevn commented Nov 10, 2021

I've made a simple PoC:

    def process_fake_type_hierarchies(
        self,
        expr_node: Expression,
        expr_type: Type,
        type_maps: Tuple[TypeMap, TypeMap],
    ) -> Tuple[TypeMap, TypeMap]:
        """Forcefully adds fake type hierarchies into a type map.

        For example, we add `builtins.int` to `no_type_map` when `isinstance(some, float)` is used.
        Because in runtime `int` is not a subclass of `float`.
        """
        yes_map, no_map = type_maps

        for union_item in union_items(expr_type):
            if isinstance(union_item, Instance) and union_item.type.fullname == 'builtins.float':
                    if no_map is None:
                        no_map = {}
                    current_type = no_map.get(expr_node)
                    runtime_types = [
                        self.named_type('builtins.int'), 
                        self.named_type('builtins.bool'),
                    ]
                    if current_type is not None:
                        runtime_types.append(current_type)
                    no_map[expr_node] = UnionType(runtime_types)
            return yes_map, no_map

With this change it works like this:

Снимок экрана 2021-11-10 в 22 34 52

I am going to test this a bit more and then send a PR.

@KotlinIsland
Copy link
Contributor Author

Don't forget about complex it's also a fake float

@KotlinIsland
Copy link
Contributor Author

And there's the bytes nightmare

@KotlinIsland
Copy link
Contributor Author

KotlinIsland commented Nov 10, 2021

This ducktyping stuff is extremely unintuitive, what do you think about reveal_type revealing all the ducklings?

a: float = 1
reveal_type(a) # float | int | complex

@sobolevn
Copy link
Member

what do you think about reveal_type revealing all the ducklings?

I believe that this can resolve in quite a lot of possible problems in the future. I will keep this change as simple as possible 🙂

@sobolevn
Copy link
Member

My current implementation went out of control. It is so complex and probably incorrect, that I am not going to continue.
This edge case is not worth adding so much stuff to the subtile type-narrowing algorithm.

Sorry! Someone else might try fixing this!

@KotlinIsland
Copy link
Contributor Author

I think it would be better to make float as an annotation just mean float | int | complex

@KotlinIsland
Copy link
Contributor Author

KotlinIsland commented Nov 11, 2021

It would remove the need for special casing to infinity and reflect the actual behaviour.

I recommend closing this in favour of #11516

@DetachHead
Copy link
Contributor

i think it's an issue that unreachable code isn't type-checked at all, regardless of whether or not it's actually unreachable, see #11649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

No branches or pull requests

4 participants