python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.21k stars 2.78k forks source link

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

Closed KotlinIsland closed 2 years ago

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

foo(1)

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

SwagatSBhuyan commented 2 years ago

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

sobolevn commented 2 years ago

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 commented 2 years ago

not sure if we can call this a bug

There isn't a 'glitch' issue category 🤣

SwagatSBhuyan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

i: float = 1
i.is_integer

😬😬😬😬😬😬😬

KotlinIsland commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

KotlinIsland commented 2 years ago

And there's the bytes nightmare

KotlinIsland commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

KotlinIsland commented 2 years ago

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

I recommend closing this in favour of #11516

DetachHead commented 2 years ago

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