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

Disallow unsafe cast() uses. #14797

Closed
jenstroeger opened this issue Feb 27, 2023 · 7 comments
Closed

Disallow unsafe cast() uses. #14797

jenstroeger opened this issue Feb 27, 2023 · 7 comments

Comments

@jenstroeger
Copy link

jenstroeger commented Feb 27, 2023

Feature

Certain uses of the cast() function should be disallowed and rejected by the type checker. In fact, I think that “unsafe” uses of cast() should be an explicit opt-in feature.

This feature request seems related to #5756

Pitch

Today I came across the following statement (yes, this is production code):

foo: Foo = cast(Foo, None)

embedded in a function roughly like so:

def get_foo() -> Foo:
    for foo in some_collection:
        if condition:
            return foo
    return cast(Foo, None)

Allowing to cast None to another type is the essence of an “unsafe” cast and should not be allowed. Furthermore, I’d consider the following uses of the cast() function nonsense as well:

s: str = typing.cast(str, 5)
reveal_type(s)

n: None = typing.cast(None, "foo")
reveal_type(n)

i: int = typing.cast(int, 3.1415)
reveal_type(i)

class Foo:
    pass

f: Foo = typing.cast(Foo, None)
reveal_type(f)

class Bar:
    pass

b: Bar = typing.cast(Bar, Foo())
reveal_type(b)

which actually gives me

note: Revealed type is "builtins.str"
note: Revealed type is "None"
note: Revealed type is "builtins.int"
note: Revealed type is "test.Foo"
note: Revealed type is "test.Bar"
Success: no issues found in 1 source file

If somebody — for whatever reason — insists on such an unsafe type cast, then they should make their intentions clear, e.g.

foo: Foo = cast(Foo, None)  # type: ignore[unsafe-cast]

Allowing the above casts, to me, defeats the purpose of using a type checker as a code QA tool which is supposed to help prevent bugs.

@sobolevn
Copy link
Member

Leaving None examples aside (they are complicated because of implicit_optional), I think that certain types of cast can surely be forbidden. But, the biggest problem is to define: what is an unsafe cast?

For example, these casts are unsafe:

  • cast(int, 'abc')
  • cast(list[int | float], [object()])

So, what can we cast then?

  • Any casts from Any or to Any
  • Casts to super-types and sub-types
  • Casting list[X] to list[X | whatever]
  • I usually cast dict to some kind of TypedDict, for example when parsing JSON responses
  • NewTypes

This can be an opt-in error code if we define what "unsafe" casts are 👍

@jenstroeger
Copy link
Author

jenstroeger commented Feb 28, 2023

But, the biggest problem is to define: what is an unsafe cast?

Isn’t that question somewhat answered when checking whether the LHS and the RHS of an assignment are “compatible”? From my list of unsafe examples or yours:

class Foo:
    pass

f: Foo = None  # error: Incompatible types in assignment (expression has type "None", variable has type "Foo")
f: Foo = cast(Foo, None)  # Success: no issues found in 1 source file

i: int = "abc"  # error: Incompatible types in assignment (expression has type "str", variable has type "int")  [assignment]
i: int = cast(int, "abc")  # Success: no issues found in 1 source file

and so forth — so if I can assign it, then it’s probably a “safe” cast and I should be allowed to cast it. (Which sort-of defeats the purpose of cast() though.)

Perhaps the builtin types should be considered more carefully with cast()? I’m sure there’s a bunch of corner cases and features of the type system that will conflict with my assumption 🤓

So, what can we cast then?

  • Any casts from Any or to Any

This sort-of opens the door to the same issue described above, but is probably strongly tied to how well mypy is able to examine the value flow:

n: Any = None
f: Foo = n  # Success: no issues found in 1 source file

But I’d probably agree: Any is castable.

  • Casts to super-types and sub-types

Up-casts, yes, down-casts are problematic (and mypy disallows them in assignments):

class Foo:
    pass

class Bar(Foo):
    def do_bar(self) -> None: ...

b: Bar = cast(Bar, Foo())  # b.do_bar() will fail.

If somebody casts to a super-type then the cast is redundant, I think, and maybe mypy could suggest a simple assignment. The down-cast should maybe be flagged, and users need to # type: ignore[unsafe-cast] it?

  • Casting list[X] to list[X | whatever]

You mean allowing additional types for a list? Wouldn’t that be similar to “widening” (as opposed to narrowing)? But I guess that’s why I want to cast().

  • I usually cast dict to some kind of TypedDict, for example when parsing JSON responses

That’s with the intention to define the shape of the dictionary, right? It’s also a down-cast but a special one.

  • NewTypes

You mean treating NewType calls as a safe cast?

So the more I think about this, the more I’m tempted to distinguish three use-cases for the cast() function:

  • “safe” casts that can be replaced by assignments
  • “unsafe” casts that the user should be required to ignore explicitly (e.g. down-cast or widening)
  • “nonsense” casts that are plain errors (like the initial cast(Foo, None)) which mypy should always reject

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Feb 28, 2023

I broadly agree.

There are two uses of safe casts:

  • The first is an upcast, which allows the user to safely erase a narrowed type Safe cast (upcast) #5756. I think this is particularly useful in the context of disallowing narrowing to mutable types (see also interaction with Types not added to binder on initial assignment #2008)
  • The second is just providing a type context to help mypy's inference (similar to how PEP 526 annotations provide type context)

Everything else is unsafe.

If you think of types as sets, I can see that it could be useful to disambiguate casts to an overlapping type (where there is a value that can possibly belong to the casted type), such as downcasts, NewType, cast to TypedDict, etc — from your "nonsense" casts where the sets of values are completely disjoint.

I would potentially be okay with including a (non-standard) opt-in check to detect nonsense casts in mypy.

But I do kind of want to pursue adding differently named functions for these cases, at least for the safe cast vs unsafe cast. In particular, I find users are sometimes completely unaware of how unsafe cast is; I think the name can be somewhat innocuous for users who haven't worked with C / C++.

@jenstroeger
Copy link
Author

But I do kind of want to pursue adding differently named functions for these three cases, or at least for the safe cast vs unsafe cast.

I agree with adding a safe_cast() and maybe unsafe_cast() function and have mypy raise issue when safe_cast() is used for unsafe casts. And I guess the existing cast() function should then be considered “unsafe” (although it does enable the nonsense category of casts 😉)?

And we’re circling in on discussion #5756

In particular, I find users are sometimes completely unaware of how unsafe cast is; I think the name can be somewhat innocuous for users who haven't worked with C / C++.

Hah, yeah. The initial cast(Foo, None) came from somebody whose background is C/C++

@erictraut
Copy link

The semantics and behavior for the existing cast are well established, so I don't think it would be a good idea to redefine the behavior, even as an optional setting. I agree that cast is very dangerous, and I frequently encourage Python developers to use it only as a last resort.

Adding a safer form of cast is a reasonable idea, but it would be better to move that discussion to the python/typing discussion forum because such an addition would presumably need to be added to the python type system and would not be specific to mypy.

@hauntsaninja
Copy link
Collaborator

Yeah, let's move discussion to python/typing#565

@hauntsaninja hauntsaninja closed this as not planned Won't fix, can't repro, duplicate, stale Mar 1, 2023
@jenstroeger
Copy link
Author

@erictraut Adding a safer form of cast is a reasonable idea, but it would be better to move that discussion to the python/typing discussion forum because such an addition would presumably need to be added to the python type system and would not be specific to mypy.

Fair enough. Should I open a discussion with the above blurb there again, or what’s the recommended approach?

@hauntsaninja Yeah, let's move discussion to python/typing#565

Thanks for the reference, that issue is interesting and quite related; I might chime in for a moment. So we now have #5756 and python/typing#565 and the python/typing discussion forum to continue this conversation 🤓

An alternative would be a writing a flake8 plugin or a pylint plugin instead… at least for the aforementioned uses of cast() involving builtin types.

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

No branches or pull requests

5 participants