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

red-knot: adapt fuzz-parser to work with red-knot #14566

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

connorskees
Copy link
Contributor

@connorskees connorskees commented Nov 24, 2024

Adds a --bin argument to the fuzz-parser script, which makes it possible to fuzz either ruff or red_knot.

This fuzzing finds a few panics already in some seeds:

>>> python scripts/fuzz-parser/fuzz.py --bin red_knot 0-9
Running `cargo build --release` since no test executable was specified...
Concurrently running the fuzzer on 10 randomly generated source-code files...
Ran fuzzer successfully on seed 1                            [1/10]
Ran fuzzer successfully on seed 8                            [2/10]
Ran fuzzer successfully on seed 5                            [3/10]
Ran fuzzer successfully on seed 0                            [4/10]
Ran fuzzer successfully on seed 2                            [5/10]
Ran fuzzer successfully on seed 6                            [6/10]
Ran fuzzer successfully on seed 3                            [7/10]
Ran fuzzer on seed 7                                         [8/10]
The following code triggers a bug:

async def name_0[*name_1](name_2: name_0(), /):
    pass

Ran fuzzer on seed 9                                         [9/10]
The following code triggers a bug:

class name_5[**name_1](name_5[name_3]):
    pass

Ran fuzzer on seed 4                                        [10/10]
The following code triggers a bug:

class name_2[name_0](*name_2):
    pass

Bugs found in the following seeds:
4 7 9

This was tested manually by running the above command for --bin ruff and --bin red_knot.

Resolves #14157, though does not add the fuzzing to CI due to the existing failures.

@sharkdp
Copy link
Contributor

sharkdp commented Nov 24, 2024

This fuzzing finds a few panics already in some seeds:

class name_2[name_0](*name_2):

Probably similar to the panics here related to self-referential class bases:

// related to circular references in class definitions
("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, false),
("crates/ruff_linter/resources/test/fixtures/pyflakes/F811_19.py", true, false),
("crates/ruff_linter/resources/test/fixtures/pyupgrade/UP039.py", true, false),

And/or here: #14333

Copy link
Contributor

github-actions bot commented Nov 24, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood added testing Related to testing Ruff itself red-knot Multi-file analysis & type inference labels Nov 24, 2024
@MichaReiser
Copy link
Member

Nice, thank you. Do you think it would be hard to instead test if the exit code isn't 1 or zero (panic should use something else) instead of adding the new flag to red knot?

@connorskees
Copy link
Contributor Author

Oh I've always assumed panic! exited with 1, but that seems not to be the case. Great suggestion, thank you, that is a lot better

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.

Thanks @connorskees, this is great! I pushed a few nitpicks, but nothing major.

@AlexWaygood AlexWaygood enabled auto-merge (squash) November 25, 2024 13:11
@AlexWaygood AlexWaygood merged commit 66abef4 into astral-sh:main Nov 25, 2024
20 checks passed
AlexWaygood added a commit that referenced this pull request Nov 27, 2024
…#14606)

## Summary

This PR gets rid of the `requirements.in` and `requirements.txt` files
in the `scripts/fuzz-parser` directory, and replaces them with
`pyproject.toml` and `uv.lock` files. The script is renamed from
`fuzz-parser` to `py-fuzzer` (since it can now also be used to fuzz
red-knot as well as the parser, following
#14566), and moved from the
`scripts/` directory to the `python/` directory, since it's now a
(uv)-pip-installable project in its own right.

I've been resisting this for a while, because conceptually this script
just doesn't feel "complicated" enough to me for it to be a full-blown
package. However, I think it's time to do this. Making it a proper
package has several advantages:
- It means we can run it from the project root using `uv run` without
having to activate a virtual environment and ensure that all required
dependencies are installed into that environment
- Using a `pyproject.toml` file means that we can express that the
project requires Python 3.12+ to run properly; this wasn't possible
before
- I've been running mypy on the project locally when I've been working
on it or reviewing other people's PRs; now I can put the mypy config for
the project in the `pyproject.toml` file

## Test Plan

I manually tested that all the commands detailed in
`python/py-fuzzer/README.md` work for me locally.

---------

Co-authored-by: David Peter <sharkdp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference testing Related to testing Ruff itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[red-knot] Adapt the fuzz-parser script to fuzz red-knot too
5 participants