Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use dynamic builtins list based on Python version #13172

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

charliermarsh
Copy link
Member

Summary

Closes #13037.

@charliermarsh
Copy link
Member Author

I used this Python script then did some manual editing:

import subprocess
import json

def enumerate_builtins_by_version(python_version):
    cmd = [
        "uv", "run", "--python", f"{python_version}", "--no-project", "python",
        "-c",
        """
import sys
import builtins
import json

python_version = f"{sys.version_info.major}.{sys.version_info.minor}"
builtin_list = sorted(dir(builtins))

print(json.dumps(builtin_list))
        """
    ]
    result = subprocess.run(cmd, check=True, capture_output=True, text=True)
    return json.loads(result.stdout)

if __name__ == "__main__":
    python_versions = ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13"]
    all_results = {}
    for version in python_versions:
        print(f"\nRunning for Python {version}")
        result = enumerate_builtins_by_version(version)
        all_results[version] = result

    print("\nRust code for listing builtins:")
    print("match (major, minor) {")
    for version, builtins in all_results.items():
        major, minor = version.split('.')
        print(f"    ({major}, {minor}) => &[")
        for builtin in builtins:
            print(f'        "{builtin}",')
        print("    ],")
    print("    _ => &[],")
    print("}")

    print("\nRust code for checking if a string is a builtin:")
    print("matches!(minor, {")

    # Collect all unique builtins across versions
    all_builtins = set()
    for builtins in all_results.values():
        all_builtins.update(builtins)

    print("pub fn is_known_standard_library(minor_version: u8, module: &str) -> bool {")
    print("    matches!(")
    print("        (minor_version, module),")
    print("        (")

    # Print the builtins that are common to all versions
    common_builtins = set.intersection(*map(set, all_results.values()))
    for builtin in sorted(common_builtins):
        print(f'            (_, "{builtin}"),')

    # Print version-specific builtins
    for version, builtins in all_results.items():
        _, minor = version.split('.')
        version_specific = set(builtins) - common_builtins
        if version_specific:
            for builtin in sorted(version_specific):
                print(f'            ({minor}, "{builtin}"),')

    print("        )")
    print("    )")
    print("}")

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Aug 30, 2024
@charliermarsh charliermarsh force-pushed the charlie/builtins branch 2 times, most recently from 580290f to 73f80b6 Compare August 30, 2024 23:23
Copy link

codspeed-hq bot commented Aug 30, 2024

CodSpeed Performance Report

Merging #13172 will not alter performance

Comparing charlie/builtins (de081b6) with main (3abd5c0)

Summary

✅ 32 untouched benchmarks

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I wonder if we should check the script you used into the repo, similar to what we do for scripts/generate_known_standard_library.py and scripts/generate_builtin_modules.py? But it also shouldn't be too hard to update when 3.14 comes around

Comment on lines +11 to +12
if is_python_builtin(name, python_version.minor())
|| source_type.is_ipynb() && is_ipython_builtin(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case (when checking a Jupyter notebook) for distinguishing between Python builtins and IPython builtins? Since we're refactoring the is_python_builtin function anyway, should we just make it a function that also accepts the PySourceType as well as the Python minor version? (And get rid of the is_ipython_builtin function?)

It seems less error-prone: currently you have to remember to call both functions if you want to know if a symbol is a builtin symbol in a notebook file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try this out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think the motivation here is that the builtins check is in uv_python_stdlib which doesn't depend on PySourceType.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. You could probably just do it with a boolean include_ipython_builtins parameter... but also this doesn't need to be done in this PR!

crates/ruff_python_stdlib/src/builtins.rs Outdated Show resolved Hide resolved
crates/ruff_python_stdlib/src/builtins.rs Outdated Show resolved Hide resolved
@charliermarsh
Copy link
Member Author

I think I'm gonna punt on adding the script. It was mostly written by GPT and still requires some massaging of the outputs, and future updates to the generated output should be easy, I think...

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though the CI has a complaint about the docs somewhere

@charliermarsh charliermarsh enabled auto-merge (squash) September 1, 2024 16:58
@charliermarsh charliermarsh merged commit c4aad4b into main Sep 1, 2024
18 checks passed
@charliermarsh charliermarsh deleted the charlie/builtins branch September 1, 2024 17:03
Copy link
Contributor

github-actions bot commented Sep 1, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+1 -0 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)

pytest-dev/pytest (+1 -0 violations, +0 -0 fixes)

+ testing/_py/test_local.py:22:45: F821 Undefined name `EncodingWarning`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
F821 1 1 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+1 -14 violations, +0 -0 fixes in 2 projects; 52 projects unchanged)

python-trio/trio (+0 -14 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- src/trio/_core/_run.py:54:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin
- src/trio/_core/_tests/test_exceptiongroup_gc.py:16:32: A004 Import `ExceptionGroup` is shadowing a Python builtin
- src/trio/_core/_tests/test_run.py:49:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin
- src/trio/_core/_tests/test_run.py:49:52: A004 Import `ExceptionGroup` is shadowing a Python builtin
- src/trio/_highlevel_open_tcp_listeners.py:16:32: A004 Import `ExceptionGroup` is shadowing a Python builtin
- src/trio/_highlevel_open_tcp_stream.py:15:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin
- src/trio/_highlevel_open_tcp_stream.py:15:52: A004 Import `ExceptionGroup` is shadowing a Python builtin
- src/trio/_tests/test_highlevel_open_tcp_listeners.py:27:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin
- src/trio/_tests/test_highlevel_open_tcp_stream.py:25:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin
- src/trio/_tests/test_testing_raisesgroup.py:14:32: A004 Import `ExceptionGroup` is shadowing a Python builtin
- src/trio/_tests/type_tests/raisesgroup.py:24:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin
- src/trio/_tests/type_tests/raisesgroup.py:24:52: A004 Import `ExceptionGroup` is shadowing a Python builtin
- src/trio/testing/_check_streams.py:30:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin
- src/trio/testing/_raises_group.py:45:32: A004 Import `BaseExceptionGroup` is shadowing a Python builtin

pytest-dev/pytest (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ testing/_py/test_local.py:22:45: F821 Undefined name `EncodingWarning`

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
A004 14 0 14 0 0
F821 1 1 0 0 0

@bonastreyair
Copy link

for some reason running using python 3.11 anext does not get properly detected.
I am getting this error:

F821 Undefined name anext

"abs",
"aiter",
"all",
"anext",

This comment was marked as resolved.

Comment on lines +110 to +115
"abs",
"all",
"any",
"ascii",
"bin",
"bool",

This comment was marked as resolved.

@AlexWaygood
Copy link
Member

@bonastreyair, what --target-version do you set when running Ruff? anext was added in Python 3.10. If you look closely you can see that we do still recognize anext as a Python builtin, but only if --target-version is set to Python 3.10 or newer. If it's set to an older version, we assume that you'll be interested in knowing that anext doesn't exist on that older version :-)

if minor_version >= 10 {
builtins.extend(&["EncodingWarning", "aiter", "anext"]);
}

];

if minor >= 10 {
builtins.extend(vec!["EncodingWarning", "aiter", "anext"]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bonastreyair anext is in the list if targeting python 3.10 or newer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the confusion did not spot that in the code I will investigate further why it is not working on my end then...

@bonastreyair
Copy link

@bonastreyair, what --target-version do you set when running Ruff? anext was added in Python 3.10. If you look closely you can see that we do still recognize anext as a Python builtin, but only if --target-version is set to Python 3.10 or newer. If it's set to an older version, we assume that you'll be interested in knowing that anext doesn't exist on that older version :-)

if minor_version >= 10 {
builtins.extend(&["EncodingWarning", "aiter", "anext"]);
}

I am using pre-commit with python target version 3.11 and in pyproject.toml with python3.11 so I don't know what is going wrong (even during CI it fails where only python3.11 is installed)

@bonastreyair
Copy link

fixed adding target-version = "py310" in the pyproject.toml under the [tool.ruff] section

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A004 Preview Mode ExceptionGroup import in pre-3.11
5 participants