Pyright should not report "obscured def" errors for conditionally defined functions

[gwk]

Consider the following:

  if cond:
    def f() -> str:
      return "YES"
  else:
    def f() -> str:
      return "NO"

Pyright issues an error: Function declaration "f" is obscured by a declaration of the same name.

However this is perfectly valid, well typed and useful code. For example, conditional defs like this can be used to lift a conditional test out of a performance-sensitive function. Mypy accepts this code without errors.

Here is a complete repro:

from sys import argv

def main() -> None:
  _, cond = argv

  if bool(cond):
    def f() -> str:
      return "YES"
  else:
    def f() -> str:
      return "NO"

  print(f())

if __name__ == '__main__': main()

[erictraut]

This is by design. You are redefining a symbol in the same namespace in two different ways. It is equivalent to the following:

if cond:
    f: str = ""
else:
    f: int = 2

I think you would agree that this should generate an error.

I would recommend against using the pattern of declaring functions within conditional statements, but if there's no way around it, then here's the recommended workaround:

if 1 + 1:
    def f_yes() -> str:
        return "YES"
    f = f_yes
else:
    def f_no() -> str:
        return "NO"
    f = f_no

[gwk]

@erictraut Respectfully, I think you have mischaracterized my concern. I am conditionally defining a function with the same signature. Your first example defines a binding with different types. There is no debate that the latter is undesirable.

Your second example is accurate; the debate is whether I should have to add these extra assignments just to mollify pyright.

To put this into context, I have been using mypy to typecheck work for many years, and I have been a happy user of vscode for even longer. Recently vscode has started showing many thousands of pyright error messages rejecting code that mypy can prove is well-typed. To simply write this off as "by design" makes me think that I should not even bother reporting these discrepancies. Does this project have any public stance on how it will approach compatibility with mypy? I searched around a bit but couldn't find anything.

[erictraut]

If you're not sure whether a particular behavior is intended, feel free to post an issue or a question in the discussion section.

Pyright attempts to conform with all of the typing standards. However, these standards leave freedom for individual type checkers in terms of specific errors, inference behaviors, type narrowing behaviors, constraint solving heuristics, etc. We've intentionally deviated from mypy's behaviors in cases where we think it makes sense. It is not a goal to be 100% compatible with the behavior of mypy. When we receive a bug report or feature request, we take into account mypy's behavior, but we don't consider ourselves beholden to it. Of the four most popular non-mypy Python type checkers (pyright, pyre, pytype and PyCharm), pyright is closest to mypy in terms of behavior (or at least I have been told as such by others).

If you like the way mypy works and you have already accommodated its behaviors (and eccentricities) in a large code base, then you may find that it's not worth using pyright.

If you are using VS Code, then I recommend you use pylance rather than pyright. Pylance is a superset of pyright and adds a bunch more useful language server features. By default, type checking is disabled in pylance, so you will not see any type errors by default. You can use pylance alongside mypy if you want.

[JelleZijlstra]

For what it's worth, I agree with @gwk here that it's better not to show an error here. The code is type-safe and clean as long as the two signatures are exactly the same.

[sirosen]

Is this issue (even though it's several months closed) the appropriate place to try to make the case that pyright should change this behavior? As a user, I'd like to check the integrity of my library's types against multiple type-checkers. Variation between checkers is understood as unavoidable. But this is an avoidable difference.

This sort of conditional definition of a callable is quite normal. It might be done, for example, on sys.version_info() for compatibility.

The rule makes def and class behave differently from assignment. I can do assignments which are not type safe, but I can't do def or class definitions which are type-safe. e.g.

# passes pyright
class X:
    y: int = 1
class Y:
    y: str = "oh hi"
if compat_mode():
    Z = X
else:
    Z = Y

# fails pyright
if compat_mode():
    class X:
        y: int = 1
else:
    class X:
        y: int = 0

I'm happy to be educated if this rule provides type-safety, but it doesn't seem to me like it does. It looks like this is just a linting rule as we'd expect from pylint, flake8, or other tools. I'd argue that it therefore ought not to be included in a type checker.

[erictraut]

When you define a class using the class keyword, you are declaring a type for a symbol. In other words, you are telling the type checker "this is the type associated with this symbol, and any attempt to redeclare it or assign it a value with a different type should be an error". This isn't a code style (linting) issue, it's the core responsibility of a static type checker to enforce type consistency issues like this!

Consider this case involving the redeclaration of a variable symbol type, which I think you would agree should be flagged as an error by a type checker.

if compat_mode():
    x: int = 3 # Error: x is obscured by a declaration of the same name
else:
    x: str = "hi"

For this same reason, it's an error to redeclare a class X within the same scope. The two classes are distinct types, even though you may happen to know that it's safe to substitute one for the other.

I think you'd agree this should generate an error, right?

class X1: ...
class X2: ...
if compat_mode():
    X1 = X2  # Error: Incompatible types

And this?

class X1: ...
class X2: ...
Z: Type[X1] = X2  # Error: Incompatible types

Your first sample type checks without errors because X and Y are distinct, and you're assigning them to a new variable Z. The type of Z is not declared, so its type is inferred. In this particular case, the type inferred by pyright is Type[X] | Type[Y].

if compat_mode():
    Z = X
else:
    Z = Y

reveal_type(Z) # Type[X] | Type[Y]

Note that the type inference rules in pyright are different from mypy. Mypy infers the type of a variable based on the first assignment whereas pyright infers the type based on the union of all assignments. Both provide type safety but mypy's approach is (IMO) unnecessarily restrictive and results in many false positive errors that pyright avoids. Your first sample above provides a good example of this.

For full details about pyright's inference rules, refer to this documentation and this documentation.

Perhaps your argument is not so much about classes but instead about function declarations? I think there's a slightly stronger argument in allowing function symbols to be redeclared as long as the signatures match exactly. That was Jelle's point above. That would be consistent with the fact that pyright allows redundant type annotations on variables as long as they match exactly.

if compat_mode():
    x: int = 3
else:
    x: int = 4

Interestingly, mypy generates an error in this case whereas pyright does not. :)

[sirosen]

Mypy infers the type of a variable based on the first assignment whereas pyright infers the type based on the union of all assignments.

This is the key thing I was missing. Thank you for taking the time to explain.

Perhaps your argument is not so much about classes but instead about function declarations?

Yes, the case I care about is function definition. Conditional class definition is quite rare by comparison. I'm accustomed to understanding both class and def as syntaxes which (1) create new objects and (2) bind those objects to names. I neglected the difference between them from the perspective of the type system and, as a result, chose a bad example.

class X: ... does two things:

1. Define a new class as we would with type("X", ...)
2. Bind a variable name to that type

def f(...): ... also does two things:

1. Define a new function as we would with lambda ...
2. Bind a variable name to that function

It gets muddled with class because a class is also a type which we'd like to use in the type system. I don't seriously want to make the case that pyright and mypy should understand arbitrary metaprogramming. lambda and type aren't completely the same as def and class -- they're just the terms we have for function and class creation in python as distinct from naming.

We can set most of these issues aside, however, when dealing with def. def does not create new types, it only binds objects to names. We can take the (reductive) stance that there is no difference between

def f(x: int, /) -> str:
    return str(x)

and

f = cast(Callable[[int], str], lambda x: str(x))

Sure, def is more expressive and can do things that lambda can't, but there's not a terribly "deep" difference between these two ways of declaring and assigning a callable.

So I see a very coherent argument that I'm wrong to suggest that conditional class redefinition should be ignored (and besides, it's quite rare). But I still think that conditional function declarations should be allowed.

[erictraut]

That's a reasonable argument. I've updated the logic to accommodate same-signature function declarations when the declarations are located within different statement suites (e.g. one within an "if" suite and a second in the "else" suite).

This addresses the original issue posted by @gwk in this thread.

This change will be in the next release of pyright.

[erictraut]

In my testing, I uncovered an inconsistency in mypy's handling of callable redefinitions. It allows a function to override a variable of the same type, but it doesn't allow the converse.

def func1(cond: bool):
    if cond:
        def a(a: int, /) -> None:
            return None

        b: Callable[[int], None] = lambda a: None

    else:
        a: Callable[[int], None] = lambda a: None  # Mypy error: Name "a" already defined

        def b(a: int, /) -> None:
            return None

I've filed a bug in the mypy issue tracker.

[erictraut]

This is addressed in pyright 1.1.187, which I've just published. It will also be included in the next release of pylance.

[francipvb]

Hello,

I have this report when a decorator other than the property decorator retrieve a new descriptor, see the case of SQLAlchemy hybrid property and hybrid method decorators. What about this case?

[erictraut]

@francipvb, this issue has been closed for a year and a half. If you would like to report a new issue, please open a new bug report. Or if you have questions, please start a discussion topic and include a minimal, self-contained code sample that demonstrates what you're trying to do. I can't tell from your description above what you have in mind.