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

Final annotation is not recognized across notebook cells #6455

Open
GF-Huang opened this issue Sep 25, 2024 · 21 comments
Open

Final annotation is not recognized across notebook cells #6455

GF-Huang opened this issue Sep 25, 2024 · 21 comments
Assignees
Labels
bug Something isn't working notebooks

Comments

@GF-Huang
Copy link

Environment data

  • Pylance version: v2024.9.2
  • OS and version: Win11 23H2
  • Python version (& distribution if applicable, e.g. Anaconda): Conda Python 3.11.9

Code Snippet

from typing import Final

test: Final[int] = 0
test = 1

image

Repro Steps

  1. XXX

Expected behavior

Show me some warning like: "you can't change constant".

Actual behavior

No warning.

Logs

XXX
@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Thanks for the issue. You have to set your typechecking mode higher than 'off' for an error to show up:

This is what I get with typeCheckingMode set to basic
Image

@rchiodo rchiodo closed this as completed Sep 25, 2024
@GF-Huang
Copy link
Author

Thanks for the issue. You have to set your typechecking mode higher than 'off' for an error to show up:

This is what I get with typeCheckingMode set to basic

Is it not works for jupyter notebook? I had set to basic but not work, same as previous.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

It works for me in a jupyter notebook. Is this an untitled notebook? The setting applies to things in the workspace, it might be the untitled notebook is not considered part of the same settings.

Image

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

It does work for me in the untitled case too though. Must be the typecheckingmode is not being applied for some reason to your notebook.

@GF-Huang
Copy link
Author

Not work. I had tried both untitled or saved file.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Can you include your logs after running 'Pylance : Start Logging'. It should tell us which workspace we think the notebook is in.

@GF-Huang
Copy link
Author

Hope these helpful.

pylance_2024.9.2_id_3.txt

1.mp4

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Hmm, I can reproduce it now too using workspace settings.json. Not sure why that matters.

@rchiodo rchiodo reopened this Sep 25, 2024
@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Yeah there's an error reading the settings for some reason. Here:

2024-09-25 11:38:05.018 [info] [Error - 11:38:05 AM] (17092) Error reading settings: TypeError: Cannot set properties of undefined (setting 'strictListInference')
2024-09-25 11:38:05.019 [info] (17092) Notebook settings returned for workspace: file:///c%3A/Users/rchiodo/source/testing/notebooks: {
  "autoSearchPaths": false,
  "openFilesOnly": true,
  "watchForConfigChanges": false,
  "watchForSourceChanges": false,
  "watchForLibraryChanges": false,
  "typeCheckingMode": "off", <-- This is coming out wrong
  "diagnosticSeverityOverrides": {
    "reportMissingImports": "none",
    "reportMissingModuleSource": "none"
  },
  "diagnosticBooleanOverrides": {
    "enableReachabilityAnalysis": false
  },
  "enableExtractCodeAction": false,
  "callArgumentNameInlayHints": "off",
  "variableInlayTypeHints": false,
  "pytestParametersInlayTypeHints": false,
  "functionReturnInlayTypeHints": false,
  "aiCodeActions": {}
}

@rchiodo rchiodo assigned rchiodo and unassigned StellaHuang95 Sep 25, 2024
@rchiodo rchiodo added the bug Something isn't working label Sep 25, 2024
@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Figured out the problem internally. We're not propagating the imports from the first cell to the second cell.

If you put the import in the same cell, the error shows up:

from typing import Final

x: Final[int] = 3
x = 212

@rchiodo rchiodo changed the title Changing final variable no warning Final annotation is not recognized with wildcard imports Sep 25, 2024
@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

I can reproduce this with two python files (no need for a notebook). Should be a pyright issue?

Create two files:

exporting_file.py:

from typing import Final

importing_file.py

from exporting_file import *

x: Final[int] = 1
x = 2 # should be an error here and there isn't

This is basically the internal structure of how notebook cells are linked together.

@rchiodo rchiodo transferred this issue from microsoft/pylance-release Sep 25, 2024
@rchiodo rchiodo removed their assignment Sep 25, 2024
@erictraut
Copy link
Contributor

@rchiodo, you can't re-export Final or ClassVar and alias them from another file. They must be imported directly from typing to work in pyright. This is a requirement of the binder, which needs to understand Final before type evaluation is possible. If you add from typing import Final to importing_file.py, it will work as you expect.

In most languages, Final and ClassVar would be keywords rather than regular identifiers, but unfortunately Python doesn't have such keywords, so we need to resort to some hacks in pyright's binder to handle these.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Hmm, that means in a notebook they have to be in the same cell then as our workaround for cells referencing each other doesn't work in this case.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Transferring back to Pylance. We might have to special case this for notebooks then.

@rchiodo rchiodo transferred this issue from microsoft/pyright Sep 25, 2024
@github-actions github-actions bot added the needs repro Issue has not been reproduced yet label Sep 25, 2024
@rchiodo rchiodo changed the title Final annotation is not recognized with wildcard imports Final annotation is not recognized across notebook cells Sep 25, 2024
@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

@erictraut is it only TypeAliases that can't be reexported? Or might there be other cases? I mean functions and variables obviously work and normal imports as well.

Like this is fine:

# Exporting_file.py
import pytest as py
# Importing_file.py
from exporting_file import *

py.foo()

@rchiodo rchiodo added notebooks and removed needs repro Issue has not been reproduced yet labels Sep 25, 2024
@erictraut
Copy link
Contributor

erictraut commented Sep 25, 2024

It's only the typing.Final and typing.ClassVar special forms (and any local aliases to these, such as _FinalAlias in the case of from typing import Final as _FinalAlias). [This list used to also include typing.Literal and typing.TypeAlias, but I found ways to defer the evaluation of these. Deferring evaluation of Final and ClassVar is much more problematic.]

How is pylance currently making symbols from one cell visible to subsequent cells? Are you synthesizing a wildcard import statement for pyright, like you show in the above sample?

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

How is pylance currently making symbols from one cell visible to subsequent cells? Are you synthesizing a wildcard import statement for pyright, like you show in the above sample?

Sort of? The binder binds them as implicit imports, but I'm having trouble finding how that affects the import resolver. Meaning how they are actually considered. Maybe we can tweak something in the import resolver for this special case.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 25, 2024

Oh it's the binding that's linking everything together. It's not the same as a wildcard import.

This line here:
https://github.com/microsoft/pyright/blob/a209885f0d612dfecbf6c624d162537cb7734f12/packages/pyright-internal/src/analyzer/program.ts#L1772

It specifies the previous cell as the list of builtins. This makes all of the symbols in the previous cell count as builtin symbols for the next cell.

@erictraut
Copy link
Contributor

OK, got it. I have some ideas for how to fix this, but they're going to require some explorations. I'll get back to you.

@erictraut
Copy link
Contributor

I've had a chance to explore some options.

The underlying issue here is that the binder needs to make some assumptions about Final because it can't evaluate types (since it runs before the type evaluator). It looks specifically for an import of Final from typing or typing_extension or a wildcard import from one of these two modules. This assumption is somewhat fragile but generally works. The pylance code that handles "chained files" breaks this assumption because it can introduce a Final symbol (or an alias thereof) into the namespace without the binder having any knowledge of it.

I explored a couple of options:

  1. Remove the fragility in the binder by pushing all evaluation of Final to the type evaluation phase. This is not only a very heavy lift, involving many code changes inside of pyright and pylance, but it also has a significant performance implications for language server features — and especially the symbol indexer in pylance. That's because the type evaluator would need to be consulted for every symbol with a variable declaration to determine whether it is final or not — and therefore whether it should be displayed as a "constant" or a "variable" in hover text and completion suggestions. If we dropped the feature that displays "constants" and "variables" differently, it would mitigate the performance impact somewhat, but I don't think we want to lose that feature.
  2. Plug the specific hole that breaks the assumption in the binder. This doesn't fully address the fragility, but it does allow the "chained files" mechanism to propagate information from the chained builtins module and any typing module imports (like Final) that were encountered when it was being bound.

Option 2 is more surgical, so I went with that solution. This will be included in the next release of pyright. @rchiodo, when you pull the code from pyright, please verify that it works with pylance's notebook support. You may want to add a pylance test case specifically for this.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 27, 2024

Option 2 is more surgical, so I went with that solution. This will be included in the next release of pyright. @rchiodo, when you pull the code from pyright, please verify that it works with pylance's notebook support. You may want to add a pylance test case specifically for this.

Thanks a lot. Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working notebooks
Projects
None yet
Development

No branches or pull requests

4 participants