microsoft / pylance-release

Documentation and issues for Pylance
Creative Commons Attribution 4.0 International
1.71k stars 765 forks source link

Can't navigate to class method #5376

Closed f3rn4nd0-c354r closed 9 months ago

f3rn4nd0-c354r commented 9 months ago

Type: Bug

Behaviour

Expected vs. Actual

When I CTRL + Click method "retrieve_data" in the last statement of the below code (bank.retrieve_data()), I would expect to go to the method definition, but nothing happens.

Steps to reproduce:

Use the following code:

from abc import ABC
from typing import List, Optional, Type

class Bank(ABC):
    @classmethod
    def recognises(cls):
        raise NotImplemented()

    def retrieve_data(self):
        pass

class B(Bank):
    def retrieve_data(self):
        print("oi")

    @classmethod
    def recognises(cls,):
        return B()

if __name__ == "__main__":
    bank: Optional[Bank] = None
    known_banks: List[Type[Bank]] = [B]
    for current_bank in known_banks:
        bank = current_bank.recognises()

    bank.retrieve_data()

Diagnostic data

Output for Python in the Output panel (ViewOutput, change the drop-down the upper-right of the Output panel to Python)

``` XXX ```

User Settings

``` venvPath: "" venvFolders: "" languageServer: "Pylance" ```

Extension version: 2023.22.1 VS Code version: Code 1.85.2 (8b3775030ed1a69b13e4f4c628c612102e30a681, 2024-01-18T06:40:19.222Z) OS version: Linux x64 5.15.0-91-generic snap Modes:

System Info |Item|Value| |---|---| |CPUs|Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz (8 x 3028)| |GPU Status|2d_canvas: unavailable_software
canvas_oop_rasterization: disabled_off
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: disabled_software
multiple_raster_threads: enabled_on
opengl: disabled_off
rasterization: disabled_software
raw_draw: disabled_off_ok
video_decode: disabled_software
video_encode: disabled_software
vulkan: disabled_off
webgl: unavailable_software
webgl2: unavailable_software
webgpu: disabled_off| |Load (avg)|2, 2, 3| |Memory (System)|31.20GB (25.48GB free)| |Process Argv|--no-sandbox --force-user-env --unity-launch --crash-reporter-id 4cd79d6f-f1f2-4022-9998-d5c0418d3d8e| |Screen Reader|no| |VM|0%| |DESKTOP_SESSION|plasma| |XDG_CURRENT_DESKTOP|KDE| |XDG_SESSION_DESKTOP|KDE| |XDG_SESSION_TYPE|x11|
A/B Experiments ``` vsliv368cf:30146710 vspor879:30202332 vspor708:30202333 vspor363:30204092 vswsl492cf:30256860 vscod805:30301674 binariesv615:30325510 vsaa593:30376534 py29gd2263:30899288 vsclangdc:30486549 c4g48928:30535728 azure-dev_surveyone:30548225 2i9eh265:30646982 962ge761:30933248 pythongtdpath:30769146 welcomedialog:30910333 pythonidxpt:30866567 pythonnoceb:30805159 asynctok:30898717 pythontestfixt:30902429 pythonregdiag2:30936856 pyreplss1:30897532 pythonmypyd1:30879173 pythoncet0:30885854 pythontbext0:30879054 accentitlementsc:30887149 dsvsc016:30899300 dsvsc017:30899301 dsvsc018:30899302 dsvsc019acf:30933242 ```
debonte commented 9 months ago

There are two issues:

  1. Pylance thinks that all implementations of Bank.recognises will return NoReturn (i.e. they all raise an error).

You can fix this by giving Bank.recognises a return annotation of "Bank":

class Bank(ABC):
    @classmethod
    def recognises(cls) -> "Bank":
        raise NotImplemented()

Btw, you may also have intended to decorate Bank.recognises with @abstractmethod:

from abc import ABC, abstractmethod

class Bank(ABC):
    @classmethod
    @abstractmethod
    def recognises(cls) -> "Bank":
        raise NotImplemented()
  1. After making the change above, go to definition will work on retrieve_data but you'll see a diagnostic: "retrieve_data" is not a known member of "None".

Although it's obvious to humans, Pylance doesn't know that after the for loop, bank is guaranteed to be non-None.

This is by design. There's a short explanation of why here.

You can work around this by testing bank after the loop:

    if bank:
        bank.retrieve_data()

Btw, you probably want to be raising NotImplementedError rather than the NotImplemented singleton.

f3rn4nd0-c354r commented 9 months ago

Hi! Thanks for all the details and for helping me improve my code. Replacing NotImplemented by NotImplementedError was enought, no need to add the test. By added the other suggestions. Thanks!