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

[pylint] Implement import-private-name (C2701) #5920

Merged
merged 8 commits into from
Jan 16, 2024

Conversation

tjkuson
Copy link
Contributor

@tjkuson tjkuson commented Jul 20, 2023

Summary

Implements import-private-name (C2701) as import-private-name (PLC2701). Includes documentation.

Related to #970.

Closes #9138.

PEP 420 namespace package limitation

checker.module_path doesn't seem to support automatic detection of namespace packages (PEP 420). This leads to 'false' positives (Pylint allows both).

Currently, for this to work like Pylint, users would have to manually input known namespace packages.

Test Plan

cargo test

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.3±0.23ms     4.9 MB/sec    1.00      8.3±0.04ms     4.9 MB/sec
formatter/numpy/ctypeslib.py               1.00   1598.8±5.51µs    10.4 MB/sec    1.01  1618.3±12.99µs    10.3 MB/sec
formatter/numpy/globals.py                 1.00    171.0±1.75µs    17.3 MB/sec    1.00    171.1±0.30µs    17.2 MB/sec
formatter/pydantic/types.py                1.00      3.5±0.04ms     7.2 MB/sec    1.00      3.5±0.03ms     7.3 MB/sec
linter/all-rules/large/dataset.py          1.01     10.6±0.03ms     3.9 MB/sec    1.00     10.5±0.04ms     3.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.8±0.00ms     5.8 MB/sec    1.00      2.8±0.01ms     5.9 MB/sec
linter/all-rules/numpy/globals.py          1.01    318.7±0.98µs     9.3 MB/sec    1.00    316.4±1.32µs     9.3 MB/sec
linter/all-rules/pydantic/types.py         1.00      4.9±0.01ms     5.3 MB/sec    1.00      4.8±0.04ms     5.3 MB/sec
linter/default-rules/large/dataset.py      1.02      5.4±0.01ms     7.5 MB/sec    1.00      5.3±0.02ms     7.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1115.0±3.37µs    14.9 MB/sec    1.00   1101.9±3.31µs    15.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    114.1±0.33µs    25.9 MB/sec    1.00    114.0±0.19µs    25.9 MB/sec
linter/default-rules/pydantic/types.py     1.01      2.4±0.01ms    10.6 MB/sec    1.00      2.4±0.00ms    10.7 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.7±0.08ms     4.2 MB/sec    1.00      9.6±0.08ms     4.2 MB/sec
formatter/numpy/ctypeslib.py               1.00  1883.8±33.01µs     8.8 MB/sec    1.00  1879.0±27.07µs     8.9 MB/sec
formatter/numpy/globals.py                 1.00   213.3±11.27µs    13.8 MB/sec    1.00    214.1±8.20µs    13.8 MB/sec
formatter/pydantic/types.py                1.00      4.0±0.05ms     6.3 MB/sec    1.01      4.1±0.05ms     6.3 MB/sec
linter/all-rules/large/dataset.py          1.02     14.4±0.53ms     2.8 MB/sec    1.00     14.1±0.11ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.04      3.8±0.23ms     4.3 MB/sec    1.00      3.7±0.03ms     4.5 MB/sec
linter/all-rules/numpy/globals.py          1.05   467.7±24.58µs     6.3 MB/sec    1.00    444.9±5.85µs     6.6 MB/sec
linter/all-rules/pydantic/types.py         1.03      6.5±0.28ms     3.9 MB/sec    1.00      6.3±0.06ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.00      7.3±0.44ms     5.6 MB/sec    1.14      8.3±0.05ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1440.0±65.45µs    11.6 MB/sec    1.15  1651.3±12.67µs    10.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    159.1±3.10µs    18.5 MB/sec    1.13    180.5±2.43µs    16.3 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.09ms     8.3 MB/sec    1.16      3.6±0.04ms     7.2 MB/sec

@tjkuson
Copy link
Contributor Author

tjkuson commented Jul 20, 2023

Most of the triggers from the ecosystem check seem to be in test suites, which is fine. The others seem to be false positives from not checking if a module is importing something from itself.

@tjkuson
Copy link
Contributor Author

tjkuson commented Jul 20, 2023

Looking through the ecosystem check, the flagged violations are all

  • true positives,
  • issues with PEP 420 (which can be fixed using the namespace-packages setting,
  • or test suites importing private names for testing reasons (which makes sense).

@charliermarsh charliermarsh changed the title [pylint] Impliment import-private-name (C2701) [pylint] Implement import-private-name (C2701) Jul 24, 2023
@tjkuson
Copy link
Contributor Author

tjkuson commented Jul 27, 2023

Related: #6114.

@tjkuson tjkuson marked this pull request as ready for review August 2, 2023 10:55
@AiyionPrime
Copy link

@tjkuson Is there anything I can do to help moving this forward?
I'd really like to have this checker in ruff.

Or is all that's left to to a rebase and the review of @charliermarsh?

@tjkuson
Copy link
Contributor Author

tjkuson commented Oct 17, 2023

@tjkuson Is there anything I can do to help moving this forward? I'd really like to have this checker in ruff.

Or is all that's left to to a rebase and the review of @charliermarsh?

Wow, I forgot about this PR! If I recall correctly, this PR was working but required users to specify namespace packages (otherwise, it would false positive). I'm not a maintainer, but perhaps the team are waiting for #6114 to be solved before accepting this merge?

@charliermarsh I don't mind rebasing and tweaking the PR if you are open to it being merged (I have probably improved with Rust over these months).

@AiyionPrime
Copy link

If wanted I'd be open to test it prior merging.
Obviously I'm not a maintainer either, but to my understanding C2701 is working, but has a known limitation for projects with namespace packages, which are not all projects.

Might a note in the docs regarding this behavior be sufficient?
It feels like #6114 would be a nice addition but is a quite orthogonal feature?
Or are there likely changes necessary in this PR once it is merged?

Anyway, thanks for the fast response, let me know when I can help :)

@charliermarsh
Copy link
Member

I think we probably do need to support the type annotation thing, since we changed flake8-self to support this: #9036

@charliermarsh
Copy link
Member

How does this compare to the flake8-self rules?

@tjkuson
Copy link
Contributor Author

tjkuson commented Dec 12, 2023

I think we probably do need to support the type annotation thing, since we changed flake8-self to support this: #9036

Sure! It should be a simple change if you are still open to the rule being merged.

How does this compare to the flake8-self rules?

private-member-access doesn't check imports. For example,

from foo import _bar

bar = _bar(...)

won't trigger the rule, but would trigger private-import-name.

@charliermarsh
Copy link
Member

Thanks @tjkuson. I assume they would both trigger on:

from foo import _bar

bar = _bar._baz(...)

And that only flake8-self would trigger on:

from foo import bar

bar = bar._baz(...)

Is that right?

@tjkuson
Copy link
Contributor Author

tjkuson commented Dec 12, 2023

Yes! Are you thinking of merging them as one rule?

@charliermarsh
Copy link
Member

@tjkuson - Eventually maybe... For now, I'm okay with them being separate as long as they don't have overlap.

@tjkuson tjkuson marked this pull request as draft December 21, 2023 17:33
@tjkuson
Copy link
Contributor Author

tjkuson commented Dec 21, 2023

Rebased to main, everything seems to be working. Checking if an import is used as a type annotation will take longer as I need to make it a deferred rule…

Copy link
Contributor

github-actions bot commented Dec 21, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+473 -1002 violations, +0 -0 fixes in 12 projects; 31 projects unchanged)

DisnakeDev/disnake (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ disnake/ext/commands/base_core.py:26:27: PLC2701 Private name import `_generated` from external module `disnake.utils`
+ disnake/ext/commands/base_core.py:26:39: PLC2701 Private name import `_overload_with_permissions` from external module `disnake.utils`
+ disnake/ext/commands/core.py:31:5: PLC2701 Private name import `_generated` from external module `disnake.utils`
+ disnake/ext/commands/core.py:32:5: PLC2701 Private name import `_overload_with_permissions` from external module `disnake.utils`
+ disnake/ext/commands/flags.py:8:27: PLC2701 Private name import `_generated` from external module `disnake.utils`
+ docs/extensions/attributetable.py:13:27: PLC2701 Private name import `_` from external module `sphinx.locale`

apache/airflow (+98 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ airflow/api/auth/backend/kerberos_auth.py:55:51: PLC2701 Private name import `_request_ctx_stack` from external module `flask`
+ airflow/io/path.py:29:52: PLC2701 Private name import `_CloudAccessor` from external module `upath.implementations.cloud`
+ airflow/metrics/otel_logger.py:30:56: PLC2701 Private name import `_internal` from external module `opentelemetry.sdk.metrics`
+ airflow/metrics/otel_logger.py:30:79: PLC2701 Private name import `_internal` from external module `opentelemetry.sdk.metrics`
+ airflow/providers/google/cloud/hooks/gcs.py:858:49: PLC2701 Private name import `_blobs_page_start` from external module `google.cloud.storage.bucket`
+ airflow/providers/google/cloud/hooks/gcs.py:858:68: PLC2701 Private name import `_item_to_blob` from external module `google.cloud.storage.bucket`
+ airflow/providers/google/common/hooks/base_google.py:42:35: PLC2701 Private name import `_http_client` from external module `google.auth.transport`
+ airflow/providers/google/common/utils/id_token_credentials.py:149:29: PLC2701 Private name import `_cloud_sdk` from external module `google.auth`
+ airflow/providers/google/common/utils/id_token_credentials.py:175:48: PLC2701 Private name import `_metadata` from external module `google.auth.compute_engine`
+ airflow/providers/google/common/utils/id_token_credentials.py:178:39: PLC2701 Private name import `_http_client` from external module `google.auth.transport`
... 88 additional changes omitted for project

aws/aws-sam-cli (+180 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ schema/make_schema.py:12:32: PLC2701 Private name import `_SAM_CLI_COMMAND_PACKAGES` from external module `samcli.cli.command`
+ tests/integration/package/test_package_command_image.py:13:45: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/integration/sync/test_sync_adl.py:5:67: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py:7:68: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py:7:84: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/unit/commands/_utils/custom_options/test_option_nargs.py:4:64: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/unit/commands/_utils/custom_options/test_replace_help_summary_option.py:4:71: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/unit/commands/_utils/test_cdk_support_decorators.py:4:59: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/unit/commands/_utils/test_click_mutex.py:3:48: PLC2701 Private name import `_utils` from external module `samcli.commands`
+ tests/unit/commands/_utils/test_command_exception_handler.py:7:5: PLC2701 Private name import `_utils` from external module `samcli.commands`
... 170 additional changes omitted for project

bokeh/bokeh (+94 -1002 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ src/bokeh/sphinxext/bokeh_sampledata_xref.py:39:27: PLC2701 Private name import `_` from external module `sphinx.locale`
+ tests/unit/bokeh/command/subcommands/test_json__subcommands.py:29:31: PLC2701 Private name import `_util_subcommands`
+ tests/unit/bokeh/core/property/test_aliases.py:29:28: PLC2701 Private name import `_util_property`
+ tests/unit/bokeh/core/property/test_aliases.py:29:43: PLC2701 Private name import `_util_property`
+ tests/unit/bokeh/core/property/test_any.py:22:28: PLC2701 Private name import `_util_property`
+ tests/unit/bokeh/core/property/test_any.py:22:43: PLC2701 Private name import `_util_property`
... 89 additional changes omitted for rule PLC2701
- tests/unit/bokeh/plotting/test__decorators.py:10:1: E265 [*] Block comment should start with `# `
- tests/unit/bokeh/plotting/test__decorators.py:11:35: E261 [*] Insert at least two spaces before an inline comment
- tests/unit/bokeh/plotting/test__decorators.py:13:14: E203 [*] Whitespace before ';'
- tests/unit/bokeh/plotting/test__decorators.py:13:15: E702 Multiple statements on one line (semicolon)
- tests/unit/bokeh/plotting/test__decorators.py:13:17: B018 Found useless expression. Either assign it to a variable or remove it.
- tests/unit/bokeh/plotting/test__decorators.py:13:1: I001 Import block is un-sorted or un-formatted
- tests/unit/bokeh/plotting/test__decorators.py:15:1: E265 [*] Block comment should start with `# `
- tests/unit/bokeh/plotting/test__decorators.py:17:1: E265 [*] Block comment should start with `# `
- tests/unit/bokeh/plotting/test__decorators.py:1:1: D100 Missing docstring in public module
- tests/unit/bokeh/plotting/test__decorators.py:1:1: E265 [*] Block comment should start with `# `
- tests/unit/bokeh/plotting/test__decorators.py:1:1: INP001 File `tests/unit/bokeh/plotting/test__decorators.py` is part of an implicit namespace package. Add an `__init__.py`.
- tests/unit/bokeh/plotting/test__decorators.py:20:1: E402 Module level import not at top of file
- tests/unit/bokeh/plotting/test__decorators.py:23:1: E402 Module level import not at top of file
- tests/unit/bokeh/plotting/test__decorators.py:24:1: E402 Module level import not at top of file
- tests/unit/bokeh/plotting/test__decorators.py:25:1: E402 Module level import not at top of file
- tests/unit/bokeh/plotting/test__decorators.py:28:1: E402 Module level import not at top of file
- tests/unit/bokeh/plotting/test__decorators.py:28:41: E261 [*] Insert at least two spaces before an inline comment
- tests/unit/bokeh/plotting/test__decorators.py:30:1: E265 [*] Block comment should start with `# `
- tests/unit/bokeh/plotting/test__decorators.py:32:1: E265 [*] Block comment should start with `# `
... 99 additional changes omitted for rule E265
- tests/unit/bokeh/plotting/test__decorators.py:43:3: FIX002 Line contains TODO, consider resolving the issue
- tests/unit/bokeh/plotting/test__decorators.py:43:3: TD002 Missing author in TODO; try: `# TODO(): ...` or `# TODO @: ...`
- tests/unit/bokeh/plotting/test__decorators.py:43:3: TD003 Missing issue link on the line following this TODO
- tests/unit/bokeh/plotting/test__decorators.py:46:25: C408 Unnecessary `dict` call (rewrite as a literal)
- tests/unit/bokeh/plotting/test__decorators.py:56:26: PT006 Wrong name(s) type in `@pytest.mark.parametrize`, expected `tuple`
- tests/unit/bokeh/plotting/test__decorators.py:56:26: Q000 [*] Single quotes found but double quotes preferred
- tests/unit/bokeh/plotting/test__decorators.py:57:39: ANN001 Missing type annotation for function argument `arg`
- tests/unit/bokeh/plotting/test__decorators.py:57:44: ANN001 Missing type annotation for function argument `values`
- tests/unit/bokeh/plotting/test__decorators.py:57:5: D103 Missing docstring in public function
- tests/unit/bokeh/plotting/test__decorators.py:59:25: Q000 [*] Single quotes found but double quotes preferred
- tests/unit/bokeh/plotting/test__decorators.py:60:17: ANN202 Missing return type annotation for private function `foo`
- tests/unit/bokeh/plotting/test__decorators.py:60:23: ANN003 Missing type annotation for `**kw`
... 1059 additional changes omitted for project

freedomofpress/securedrop (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ securedrop/secure_tempfile.py:3:22: PLC2701 Private name import `_TemporaryFileWrapper` from external module `tempfile`
+ securedrop/tests/conftest.py:24:25: PLC2701 Private name import `_SourceScryptManager` from external module `source_user`
+ securedrop/tests/test_config.py:3:22: PLC2701 Private name import `_parse_config_from_file` from external module `sdconfig`
+ securedrop/tests/test_source_user.py:11:5: PLC2701 Private name import `_DesignationGenerator` from external module `source_user`
+ securedrop/tests/test_source_user.py:12:5: PLC2701 Private name import `_SourceScryptManager` from external module `source_user`

ibis-project/ibis (+10 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ docs/backends/app/backend_info_app.py:14:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/how-to/visualization/example_streamlit_app/example_streamlit_app.py:6:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/posts/pydata-performance-part2/datafusion_ibis.py:4:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/posts/pydata-performance-part2/duckdb_ibis.py:4:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/posts/pydata-performance-part2/polars_ibis.py:4:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/posts/pydata-performance/step0.py:4:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/posts/pydata-performance/step1.py:4:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/posts/pydata-performance/step2.py:4:18: PLC2701 Private name import `_` from external module `ibis`
+ docs/posts/pydata-performance/step3.py:4:18: PLC2701 Private name import `_` from external module `ibis`
+ ibis/backends/conftest.py:11:8: PLC2701 Private name import `_pytest`

... Truncated remaining completed project reports due to GitHub comment length restrictions

Changes by rule (45 rules affected)

code total + violation - violation + fix - fix
PLC2701 473 473 0 0 0
S101 230 0 230 0 0
Q000 150 0 150 0 0
E265 104 0 104 0 0
D102 63 0 63 0 0
ANN101 60 0 60 0 0
PLR6301 60 0 60 0 0
PLR2004 47 0 47 0 0
SLF001 38 0 38 0 0
E402 23 0 23 0 0
C408 22 0 22 0 0
E275 18 0 18 0 0
PT011 17 0 17 0 0
E231 16 0 16 0 0
E261 15 0 15 0 0
ANN001 15 0 15 0 0
ERA001 14 0 14 0 0
D101 12 0 12 0 0
N801 12 0 12 0 0
E203 8 0 8 0 0
E702 7 0 7 0 0
B018 7 0 7 0 0
I001 7 0 7 0 0
D100 7 0 7 0 0
INP001 7 0 7 0 0
D103 6 0 6 0 0
A002 6 0 6 0 0
E202 4 0 4 0 0
E201 4 0 4 0 0
N802 3 0 3 0 0
FIX002 2 0 2 0 0
TD002 2 0 2 0 0
TD003 2 0 2 0 0
PT018 2 0 2 0 0
DTZ001 2 0 2 0 0
PT006 1 0 1 0 0
ANN202 1 0 1 0 0
ANN003 1 0 1 0 0
ARG001 1 0 1 0 0
PT012 1 0 1 0 0
ANN201 1 0 1 0 0
PLC0415 1 0 1 0 0
E225 1 0 1 0 0
E241 1 0 1 0 0
PLR0402 1 0 1 0 0

@tjkuson tjkuson marked this pull request as ready for review December 21, 2023 19:36
@tjkuson
Copy link
Contributor Author

tjkuson commented Dec 21, 2023

@charliermarsh The PR has been rebased and now forgives private name imports that are used for type annotations!

Copy link

codspeed-hq bot commented Dec 29, 2023

CodSpeed Performance Report

Merging #5920 will improve performances by ×4.9

Comparing tjkuson:import-private-name (c98ef43) with main (c6d8076)

Summary

⚡ 15 improvements
✅ 15 untouched benchmarks

Benchmarks breakdown

Benchmark main tjkuson:import-private-name Change
linter/all-rules[numpy/globals.py] 4.2 ms 3 ms +38.1%
linter/default-rules[large/dataset.py] 92.7 ms 19 ms ×4.9
linter/all-rules[unicode/pypinyin.py] 16.8 ms 12.3 ms +36.93%
linter/all-rules[pydantic/types.py] 77 ms 48.5 ms +58.64%
linter/all-rules[numpy/ctypeslib.py] 36.1 ms 23.3 ms +54.91%
linter/default-rules[numpy/globals.py] 1,799.7 µs 635.5 µs ×2.8
linter/all-with-preview-rules[numpy/ctypeslib.py] 39.3 ms 26.5 ms +48.23%
linter/all-with-preview-rules[pydantic/types.py] 85.3 ms 55.9 ms +52.41%
linter/all-with-preview-rules[numpy/globals.py] 4.5 ms 3.4 ms +34.15%
linter/default-rules[numpy/ctypeslib.py] 16.9 ms 4 ms ×4.2
linter/default-rules[unicode/pypinyin.py] 6 ms 1.5 ms ×4.1
linter/all-rules[large/dataset.py] 176 ms 102.5 ms +71.67%
linter/default-rules[pydantic/types.py] 36.2 ms 8.4 ms ×4.3
linter/all-with-preview-rules[unicode/pypinyin.py] 17.9 ms 13.4 ms +33.11%
linter/all-with-preview-rules[large/dataset.py] 198.4 ms 122 ms +62.57%

@charliermarsh
Copy link
Member

Sorry, this is blocked on me, I've just been slow to get to it.

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 16, 2024
@charliermarsh
Copy link
Member

Great work @tjkuson.

@charliermarsh charliermarsh enabled auto-merge (squash) January 16, 2024 05:11
@charliermarsh charliermarsh merged commit 2b60552 into astral-sh:main Jan 16, 2024
15 of 16 checks passed
@MichaReiser
Copy link
Member

I'm going to revert this change. It's panicking in the pydantic benchmark run (see https://github.com/astral-sh/ruff/actions/runs/7537305800/job/20516059364?pr=5920).

tjkuson added a commit to tjkuson/ruff that referenced this pull request Jan 16, 2024
Includes fix for incorrect slice in astral-sh#5920.
charliermarsh pushed a commit that referenced this pull request Jan 16, 2024
## Summary

#5920 with a fix for the erroneous slice in `module_name`. Fixes #9547.

## Test Plan

Added `import bbb.ccc._ddd as eee` to the test fixture to ensure it no
longer panics.

`cargo test`
@tjkuson tjkuson deleted the import-private-name branch January 16, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
5 participants