intellij-community PY-60660 - conditional stub definition (typeshed)

Hi, I would like to implement a fix for conditional typeshed stubs in PyCharm (PY-60660).

Some stubs are defined by using checks for a specific Python version or the operating system as you can see here.

I already started a PoC which can identify the correct stub depending on the os-platform and Python version. In case one of the conditions for a stub can't be evaluated, the code falls back to the previous behavior - resolves to multiple definitions if there is more then one defined.

Can you take a look at the current implementation and hopefully answer the following questions?

Where would you expect/implement such a check for conditional stub definitions if the current one is not a good choice?
The current implementation requires excluding of pyi-files from assertNotParsed. Because, these files are now parsed to check if there are conditional definitions. Is this OK for you? If not, what would be the way to go to implement checks for conditional definitions without breaking the current behavior of assertNotParsed (some codepointer could help)?
To resolve the correct stub definition, the implementation needs to know the Python version (checked by sys.version_info) and the operating system (checked by sys.platform). What would be the correct way to get the necessary information to implement a correct check with sys.version_info and sys.platform (which also works for all kind of Python interpreters supported by PyCharm)? I use SystemInfo to get the os-platform. But this class can't be used for a Python remote interpreter if the remote operating system is different.
The test Py3QuickDocTest::testTypingExtensionsSelf now fails, because of my changes, with:

com.intellij.rt.execution.junit.FileComparisonFailure: Expected:

<html>
    <body>
        <div class="bottom">
            <icon src="AllIcons.Nodes.Class" />&nbsp;<code><a href="psi_element://#typename#TypingExtensionsSelf.A">TypingExtensionsSelf.A</a></code>
        </div>
        <div class="definition">
            <pre><span style="color:#000080;font-weight:bold;">def </span><span style="color:#000000;">foo</span><span style="">(</span><span style="color:#94558d;">self</span><span style="">)</span> -&gt; <span style="color:#000000;">Self</span></pre>
        </div>
    </body>
</html>

Actual:

<html>
    <body>
        <div class="bottom">
            <icon src="AllIcons.Nodes.Class" />&nbsp;<code><a href="psi_element://#typename#TypingExtensionsSelf.A">TypingExtensionsSelf.A</a></code>
        </div>
        <div class="definition">
            <pre><span style="color:#000080;font-weight:bold;">def </span><span style="color:#000000;">foo</span><span style="">(</span><span style="color:#94558d;">self</span><span style="">)</span> -&gt; <span style="color:#000000;"><a href="psi_element://#typename#typing._SpecialForm">Self</a></span></pre>
        </div>
    </body>
</html>

The question is, is the expected value right or is now (after my changes) the actual one right? The difference between the values is the resolved definition.

Jun 05 '23 21:06 rendner

Hi! Thanks for having interest in fixing the issue. The underlying problem is actually a part of a larger, long requested issue PY-34617 we're working on right now. We can try integrating your PoC with our prototype, but it might be difficult depending on the current state of the latter. @Elizaveta239 can tell more.

As for your questions,

Where would you expect/implement such a check for conditional stub definitions if the current one is not a good choice?

PyResolveProcessor is one possible place. I don't see any glaring problems with it, except that some resolve API (such as PyClass.find* family of methods) might ignore it.

The current implementation requires excluding of pyi-files from assertNotParsed

This part is problematic. Our idea was that such checks should become part of PSI-stubs so that they could be persisted and extracted from the indexes without re-parsing the corresponding physical files, similarly to PyExceptPart (see PyExceptPartElementType).

What would be the correct way to get the necessary information to implement a correct check with sys.version_info and sys.platform?

LanguageLevel can be extracted from any PSI element in a project with LanguageLevel#forElement. The underlying OS of a configured project SDK in theory could be retrieved from the corresponding Run Target request TargetEnvironmentRequest#getTargetPlatform, but it seems that it doesn't distinguish between macOS and linux. We actually didn't want to support sys.platform checks right away and focus on sys.version_info first, because those are causing more problems.

Another difference with how we were going to support these is that we planned to consider the checks in .py files as well, not only in .pyi stubs. Also, there is a harder problem of supporting them in the compatibility inspection that should deal with a few possible Python versions, not only the one currently configured for the project, but we wanted to postpone this one for the time being.

Overall, let's check up with @Elizaveta239 first, to make sure your proposal is compatible with what we already have in progress.

Jun 07 '23 10:06 east825

Hi @rendner! Thank you very much for your pull request! Yes, we're currently working on this issue, on a more wide solution which was mentioned by @east825. This solution doesn't parse corresponding files (and stores info inside Psi-stubs) and works not only with .pyi, but also with .py files. Currently, I'm not ready to give any ETA and I'm not sure if we manage to re-use any parts of your implementation, but we'll try! (with saving authorship, of course). Thank you again!

Jun 07 '23 16:06 Elizaveta239

Hi @east825 and @Elizaveta239,

This part is problematic. Our idea was that such checks should become part of PSI-stubs so that they could be persisted and extracted from the indexes without re-parsing the corresponding physical files, similarly to PyExceptPart (see PyExceptPartElementType).

Your solution will then implement such a stub for the whole if-statement or at least for the if and elif parts of a such a statement. This sounds good. I was already thinking that there must be a better way than my current approach (from a performance standpoint).

LanguageLevel can be extracted from any PSI element in a project with LanguageLevel#forElement.

I started with that method, but it doesn't work as expected for elements of a pyi-file, because PyiFile.getLanguageLevel returns always the latest LanguageLevel. For these files PyiUtil.getOriginalLanguageLevel does the right job. In the end I decided to take the information from the project Sdk, because I expected all elements would return the same language level as the used Sdk.

Yes, we're currently working on this issue, on a more wide solution which was mentioned by @east825. This solution doesn't parse corresponding files (and stores info inside Psi-stubs) and works not only with .pyi, but also with .py files.

I will keep an eye on the original problem to see when you have finished your solution. Maybe in the meantime I will also find some time and try to implement the psi-stubs approach for myself, at least for pyi files.

Thank you very much for your detailed answers and the extended insight into the overall problem.

The PR can therefore be closed, right?

Jun 09 '23 23:06 rendner

The PR can therefore be closed, right?

Let's leave it opened so far, until the solution is ready.

Thank you very much for your contribution!

Jun 11 '23 17:06 Elizaveta239

intellij-community intellij-community copied to clipboard

PY-60660 - conditional stub definition (typeshed)

intellij-community
intellij-community copied to clipboard