microsoft / pyright

Static Type Checker for Python
Other
13.04k stars 1.39k forks source link

`reportPrivateImportUsage` false negatives when running `pyright <directory>` but not when running `find <directory> -name '*.py' | xargs pyright` #8377

Closed tamird closed 1 month ago

tamird commented 1 month ago

In my project I have

├── lib/common
│   ├── common
│   │   ├── py.typed
│   │   ├── db
│   │   │   ├── sites
│   │   │   │   ├── models.py # from common.db.sites.objects import WeatherState
│   │   │   │   ├── objects.py # class WeatherState: ...
│   ├── tests
│   │   ├── external_clients
│   │   │   ├── openweather
│   │   │   │   ├── test_openweather.py # from common.db.sites.models import WeatherState
│   ├── pyproject.toml

If I run poetry run pyright lib/common I get no diagnostics. If I run find lib/common -name '*.py' | xargs -P24 -n1 poetry run pyright -p . --warnings I get:

<path-to-repo>/lib/common/tests/external_clients/openweather/test_openweather.py                                                                  
  <path-to-repo>/lib/common/tests/external_clients/openweather/test_openweather.py:8:36 - error: "WeatherState" is not exported from module "common.db.sites.models"                                                                                                                                                         
    Import from "common.db.sites.objects" instead (reportPrivateImportUsage)

I'm using pyright 1.1.371:

poetry run pyright --version
pyright 1.1.371
erictraut commented 1 month ago

I'm having trouble reproing the problem. I suspect it's because I don't know anything about poetry. Could you create a simpler repro case that doesn't require poetry? Or does this repro for you only if you're using poetry? I think poetry may do odd things with the working directory. Also, it would be useful to see the relevant contents of your pyproject.toml file.

tamird commented 1 month ago

I'm not sure how I'd go about creating a simpler repro (the skeleton I wrote above is my attempt to do so).

I believe poetry is just activating the venv and running pyright. I can reproduce just the same:

tamird@Tamirs-MBP python_monorepo % . .venv/bin/activate        
(root-py3.12) tamird@Tamirs-MBP python_monorepo % .venv/bin/pyright lib/common
0 errors, 0 warnings, 0 informations 
(root-py3.12) tamird@Tamirs-MBP python_monorepo % .venv/bin/pyright lib/common/tests/external_clients/openweather/test_openweather.py
<pwd>/lib/common/tests/external_clients/openweather/test_openweather.py
  <pwd>/lib/common/tests/external_clients/openweather/test_openweather.py:8:36 - error: "WeatherState" is not exported from module "common.db.sites.models"
    Import from "common.db.sites.objects" instead (reportPrivateImportUsage)
1 error, 0 warnings, 0 informations 

Here's the (hopefully relevant) content of the pyproject.toml file:

[tool.pyright]
# TODO: Remove these when we have property subclasses instead of type aliases of `Annotated`.
reportArgumentType = false # Argument of type "ProfileClass" cannot be assigned to parameter "cast_to" of type "type[CastT@require_single]" in function "require_single"
reportCallIssue = false    # Object of type "type[Annotated]" is not callable

# TODO: Figure out why pyright thinks Supply_Property/SupplyPropertyMixin/Supply_TPR_Property are unhashable and remove.
reportUnhashable = false # Type "Supply_Property" is not hashable (reportUnhashable)

really not much going on here.

erictraut commented 1 month ago

When I attempt to run with poetry, it complains that there's no poetry section in the pyproject.toml file. Good to hear that you can repro without poetry.

I'm still not able to repro it though. Here are the specific steps I've taken.

  1. Create your directory and file structure.
  2. From the shell, create a new venv: python3.12 -m venv .venv
  3. Activate the new venv: source .venv/bin/activate
  4. Install the latest version of pyright: pip install pyright
  5. Run pyright lib/common, which outputs zero errors.
  6. Run pyright lib/common/tests/external_clients/openweather/test_openweather.py, which outputs zero errors

The library in question is considered part of your project, not an external library. That means the "py.typed" marker doesn't apply unless the import resolution is being done via your Python environment. Perhaps you've done an editable install of this library within your environment? That might explain the symptoms you're seeing because import resolution will be affected by how you invoke pyright.

tamird commented 1 month ago

Ah, yes. That is the case. Here's more of the root-level pyproject.toml:

[tool.poetry]
authors = []
description = "Common tool configurations"
name = "root"
package-mode = false
readme = "README.md"
version = "0.1.0"

[tool.poetry.dependencies]
python = ">=3.12.3, <4" # AWS images appear to use 3.12.3 at the time of writing.

common = { path = "lib/common", develop = true }
erictraut commented 1 month ago

Pyright appears to be working correctly here, so I'm going to close out this issue.

As I mentioned above, the reportPrivateImportUsage check applies only to symbols imported from a "py.typed" external library. It doesn't apply to imports from modules within your project.

If you can provide additional details about your configuration, I'm happy to help you diagnose the issue.

tamird commented 1 month ago

The example I gave is just one of many. In our project we have several libraries which are all editable installed into the root; running pyright from the root across all files doesn't report reportPrivateImportUsage, but running it on just one library e.g. pyright lambdas/supplier does report violations of reportPrivateImportUsage where the import comes from lib/common.

For what it's worth the virtualenv we activate for pyright is always the root one, into which all the other libs are editable installed.

erictraut commented 1 month ago

When you invoke pyright with a directory, it assumes that directory is the root for your project. When you invoke pyright with individual file paths, it checks each one assuming that the directory of each file is the project root directory. This impacts import resolution behaviors because the project root directory is prepended to the list of directories used for import resolution. For details, refer to this documentation.

If you want consistent import resolution behavior, you should invoke pyright using your project root directory.

tamird commented 1 month ago

I see. Thanks for explaining.

Since solving this requires executing pyright once for each pyproject.toml, would it be possible to opt into a stricter version of reportPrivateImportUsage that applies to imports within the project?

erictraut commented 1 month ago

would it be possible to opt into a stricter version of reportPrivateImportUsage that applies to imports within the project?

That's something we'd be unlikely to ever add. The logic for when to pay attention to "py.typed" has impacts that go well beyond the reportPrivateImportUsage check. Even if this weren't an issue, we generally don't add diagnostics that are not enabled by default in strict mode because they are not discoverable and are rarely used by pyright users.

If you want reportPrivateImportUsage to check for inappropriate uses of exports from a "py.typed" library, you should configure pyright so it doesn't attempt to resolve these imports locally within your project and instead uses only the editable install paths. For example, you could rename the interior "common" directory to something else, like "_common" (with an underscore. Pyright would then never resolve these imports local to your project and would always fall back on your editable install.