microsoft / pylance-release

Documentation and issues for Pylance
Creative Commons Attribution 4.0 International
1.72k stars 769 forks source link

Code marked unreachable incorrectly #5647

Closed sztomi closed 8 months ago

sztomi commented 8 months ago

Environment data

Code Snippet

def foo(lst: list | str | None, separator=" ") -> list:
    if lst is None:
        return []
    elif isinstance(lst, list):
        processed = []
        return processed    # <-----
    elif isinstance(lst, str):
        return [it.strip() for it in lst.split(separator)]

    # This is marked inaccessible in vscode
    msg = f"Invalid composite list: {lst}"
    raise ValueError(msg)

Repro Steps

  1. Write the above code in vscode

Expected behavior

I would expect the code to not contain unreachable lines. Even though the type annotation says list | str | None and those types are indeed covered by the if-elif-elif conditions, that has no bearing on the runtime accessibility of these lines. I can call foo(5) and get the exception. Yes, of course I would get a type lint for that from Pylance, but the lines are still reachable at runtime. If I removed those lines thinking they have no effect, the code would behave differently.

Actual behavior

The lines after the if-elif-elif block are considered inaccessible.

Logs

https://gist.github.com/sztomi/710d900440184965b7cfec0f2f959193 (the code is in utils/common.py and the function's original name is process_composite_list)

Groni3000 commented 8 months ago

Just add else and your code will become reachable again.

It's working as intended and should be like that. If you cover all possible types - code below shouldn't be reachable.

debonte commented 8 months ago

This is by design. Reachability is determined by the type checker. If callers of foo pass other types, for example foo(5) as you suggested above, Pylance will report diagnostics on those calls prompting you to fix them:

image

If you intend for callers to be able to pass other types to foo, you should modify the annotation of lst to fully describe the types that foo is intended to support.

debonte commented 8 months ago

@erictraut, as @Groni3000 pointed out, if the inaccessible code block is moved into an else block, it isn't marked as unreachable. Is that a bug or somehow intentional?

def foo(lst: list | str | None, separator=" ") -> list:
    if lst is None:
        return []
    elif isinstance(lst, list):
        processed = []
        return processed    # <-----
    elif isinstance(lst, str):
        return [it.strip() for it in lst.split(separator)]
    else:
        # This is NOT marked inaccessible in vscode
        msg = f"Invalid composite list: {lst}"
        raise ValueError(msg)
erictraut commented 8 months ago

This is by design, not a bug. When there's a dangling elif (without an else), pyright performs an analysis step referred to as narrowing for implied else. This step is relatively expensive, but it's necessary in some cases to avoid false positive "potentially unbound" errors. The technique involves analyzing the condition for the last if or elif to determine whether any of the expressions in the condition can be narrowed to Never in the implied "else" (fall-through) case. If so, we know that the fall-through case is unreachable.

A similar analysis is not done if there is an explicit "else" because it would be equally costly and wouldn't improve type evaluations or eliminate any false positives. It's assumed that if a developer added an "else" clause, that was intended.

sztomi commented 8 months ago

But the analysis is wrong, the execution can fall through. Yes, there is a correct diagnostic when passing a type other than str | list | NoneType, but that has absolutely no bearing on the runtime behavior of the code. If code is unreachable it should be possible to remove it without any effect on runtime behavior but that's not the case here.

erictraut commented 8 months ago

Based on the type annotations provided, the code analysis is correct. The type annotation for parameter lst indicates that only values of type list, str or None can be passed to the function. If your function accommodates values of other types by design, you should annotate it as such (e.g. include an object in the union).

sztomi commented 8 months ago

Type analysis does not happen at runtime, nothing is stopping anyone from passing another type (this is library code). The lines are reachable. In fact, this exception is precisely there for that case. The diagnostic I would get for the function call would be emitted regardless of these lines being there.

Groni3000 commented 8 months ago

@sztomi As I understand, pylance uses pyright under the hood. And pyright is a static type checker. From a statically typed language perspective - it is correct behavior, code below if-elif is unreachable.

Ofc, in python, you can pass integer, for example, and you will reach that code part, but pyright will tell you that you messed up function usage.

That's what for pyright. To make your code reliable as much as it possibly can be.

Currently, that code snippet has logic issues. It can happen in every library. But pylance/pyright is working correctly.

sztomi commented 8 months ago

@Groni3000 I fully understand the reasoning, but my stance is that this notion of "unreachable code" is not useful (and misleading).