Reject more syntactically invalid Python programs by BurntSushi · Pull Request #8524 · astral-sh/ruff

BurntSushi · 2023-11-06T19:15:53Z

Summary

This commit adds some additional error checking to the parser such that assignments that are invalid syntax are rejected. This covers the obvious cases like 5 = 3 and some not so obvious cases like x + y = 42.

This does add an additional recursive call to the parser for the cases handling assignments. I had initially been concerned about doing this, but set_context is already doing recursion during assignments, so I didn't feel as though this was changing any fundamental performance characteristics of the parser. (Also, in practice, I would expect any such recursion here to be quite shallow since the recursion is done on the target of an assignment. Such things are rarely nested much in practice.)

Fixes #6895

Test Plan

I've added unit tests covering every case that is detected as invalid on an Expr.

charliermarsh

This is excellent! Welcome to the repo :)

crates/ruff_python_parser/src/invalid.rs

This commit adds some additional error checking to the parser such that assignments that are invalid syntax are rejected. This covers the obvious cases like `5 = 3` and some not so obvious cases like `x + y = 42`. This does add an additional recursive call to the parser for the cases handling assignments. I had initially been concerned about doing this, but `set_context` is already doing recursion during assignments, so I didn't feel as though this was changing any fundamental performance characteristics of the parser. (Also, in practice, I would expect any such recursion here to be quite shallow since the recursion is done on the target of an assignment. Such things are rarely nested much in practice.) Fixes #6895

This test previously asserted that, e.g., `a = 1, b = 2`, was valid Python code. But it turns out that it is not: >>> a = 1, b = 2 File "<input>", line 1 a = 1, b = 2 ^^^^^ SyntaxError: invalid syntax. Maybe you meant '==' or ':=' instead of '='? This commit flips the particular assert to check that no Python code is detected within the comment.

With the parser becoming a bit more strict, it discovered some invalid syntax in one of the formatter tests. In particular, `yield a = 1`: >>> (yield a, b) = (1, 2) File "<input>", line 1 (yield a, b) = (1, 2) ^^^^^^^^^^ SyntaxError: cannot assign to yield expression here. Maybe you meant '==' instead of '='? We just remove that bit from the `yield` test.

charliermarsh

Nice work.

BurntSushi · 2023-11-06T19:58:00Z

@charliermarsh Can you check out the two commits I've just pushed? In particular, the stricter parser caused two tests to fail. I think my changes to those tests are correct, but I'd like to be cautious here!

charliermarsh · 2023-11-06T19:58:46Z

Taking a closer look at those changes...

charliermarsh · 2023-11-06T20:01:40Z

@BurntSushi - Those both look correct to me. I actually don't fully understand why app_label=\"core\", model=\"user\"", was previously marked as valid code, what was the "target" and the "value"?

BurntSushi · 2023-11-06T20:05:28Z

was previously marked as valid code, what was the "target" and the "value"?

For x="a", y="b", it looks like it parsed as two targets, x and ("a", y) with a single value of b. It's not totally clear to me how it gets that particular parse. That first equals sign, I'd imagine, would inhibit picking up the second target.

charliermarsh · 2023-11-06T20:06:33Z

Ahh ok, so it's like x = ("a", y) = "b". Makes sense-ish.

github-actions · 2023-11-06T20:11:44Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+0 -27 violations, +0 -0 fixes in 41 projects)

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

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

- airflow/config_templates/default_webserver_config.py:84:1: ERA001 Found commented-out code
- airflow/example_dags/tutorial.py:63:9: ERA001 Found commented-out code
- airflow/serialization/serialized_objects.py:1195:13: ERA001 Found commented-out code
- airflow/serialization/serialized_objects.py:1211:13: ERA001 Found commented-out code
- airflow/settings.py:633:1: ERA001 Found commented-out code
- airflow/utils/log/secrets_masker.py:174:13: ERA001 Found commented-out code
- tests/system/providers/amazon/aws/example_eks_templated.py:45:1: ERA001 Found commented-out code
- tests/system/providers/amazon/aws/example_eks_templated.py:49:1: ERA001 Found commented-out code

bokeh/bokeh (+0 -6 violations, +0 -0 fixes)

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

- examples/integration/layout/plot_fixed_frame_size.py:40:1: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:114:9: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:167:9: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:221:9: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:269:9: ERA001 Found commented-out code
- tests/unit/bokeh/plotting/test__renderer.py:125:1: ERA001 Found commented-out code

zulip/zulip (+0 -13 violations, +0 -0 fixes)

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

- zerver/data_import/slack.py:581:13: ERA001 Found commented-out code
- zerver/lib/digest.py:89:1: ERA001 Found commented-out code
- zerver/lib/display_recipient.py:106:5: ERA001 Found commented-out code
- zerver/lib/email_notifications.py:309:5: ERA001 Found commented-out code
- zerver/lib/email_notifications.py:317:5: ERA001 Found commented-out code
- zerver/lib/email_notifications.py:321:5: ERA001 Found commented-out code
- zerver/middleware.py:152:13: ERA001 Found commented-out code
- zerver/middleware.py:175:13: ERA001 Found commented-out code
- zerver/tests/test_events.py:548:13: ERA001 Found commented-out code
- zerver/tests/test_events.py:550:13: ERA001 Found commented-out code
- zerver/tests/test_events.py:556:17: ERA001 Found commented-out code
- zerver/tests/test_events.py:727:9: ERA001 Found commented-out code
- zproject/prod_settings_template.py:264:1: ERA001 Found commented-out code

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
ERA001	27	0	27	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -25 violations, +0 -0 fixes in 41 projects)

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

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

- airflow/config_templates/default_webserver_config.py:84:1: ERA001 Found commented-out code
- airflow/example_dags/tutorial.py:63:9: ERA001 Found commented-out code
- airflow/settings.py:633:1: ERA001 Found commented-out code
- airflow/utils/log/secrets_masker.py:174:13: ERA001 Found commented-out code
- tests/system/providers/amazon/aws/example_eks_templated.py:45:1: ERA001 Found commented-out code
- tests/system/providers/amazon/aws/example_eks_templated.py:49:1: ERA001 Found commented-out code

bokeh/bokeh (+0 -6 violations, +0 -0 fixes)

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

- examples/integration/layout/plot_fixed_frame_size.py:40:1: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:114:9: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:167:9: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:221:9: ERA001 Found commented-out code
- tests/integration/widgets/tables/test_source_updates.py:269:9: ERA001 Found commented-out code
- tests/unit/bokeh/plotting/test__renderer.py:125:1: ERA001 Found commented-out code

zulip/zulip (+0 -13 violations, +0 -0 fixes)

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

- zerver/data_import/slack.py:581:13: ERA001 Found commented-out code
- zerver/lib/digest.py:89:1: ERA001 Found commented-out code
- zerver/lib/display_recipient.py:106:5: ERA001 Found commented-out code
- zerver/lib/email_notifications.py:309:5: ERA001 Found commented-out code
- zerver/lib/email_notifications.py:317:5: ERA001 Found commented-out code
- zerver/lib/email_notifications.py:321:5: ERA001 Found commented-out code
- zerver/middleware.py:152:13: ERA001 Found commented-out code
- zerver/middleware.py:175:13: ERA001 Found commented-out code
- zerver/tests/test_events.py:548:13: ERA001 Found commented-out code
- zerver/tests/test_events.py:550:13: ERA001 Found commented-out code
- zerver/tests/test_events.py:556:17: ERA001 Found commented-out code
- zerver/tests/test_events.py:727:9: ERA001 Found commented-out code
- zproject/prod_settings_template.py:264:1: ERA001 Found commented-out code

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
ERA001	25	0	25	0	0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

charliermarsh · 2023-11-06T20:16:06Z

Those ecosystem changes look like strict improvements to me.

zanieb

Nice :)

dhruvmanila

Thanks for writing such a thorough test suite! :)

BurntSushi requested a review from charliermarsh November 6, 2023 19:15

BurntSushi force-pushed the ag/fix-6895 branch from bd787b4 to db76759 Compare November 6, 2023 19:19

charliermarsh reviewed Nov 6, 2023

View reviewed changes

charliermarsh added the bug Something isn't working label Nov 6, 2023

BurntSushi added 3 commits November 6, 2023 14:54

BurntSushi force-pushed the ag/fix-6895 branch from db76759 to 9142dae Compare November 6, 2023 19:55

charliermarsh approved these changes Nov 6, 2023

View reviewed changes

zanieb approved these changes Nov 6, 2023

View reviewed changes

dhruvmanila approved these changes Nov 7, 2023

View reviewed changes

BurntSushi merged commit 6a1fa47 into main Nov 7, 2023

BurntSushi deleted the ag/fix-6895 branch November 7, 2023 12:16

miccal mentioned this pull request Nov 8, 2023

ruff 0.1.5 Homebrew/homebrew-core#153715

Merged

BurntSushi mentioned this pull request Jan 4, 2024

pep440: some minor refactoring, mostly around error types astral-sh/uv#780

Merged

Conversation

BurntSushi commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

charliermarsh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

charliermarsh left a comment

Choose a reason for hiding this comment

Uh oh!

BurntSushi commented Nov 6, 2023

Uh oh!

charliermarsh commented Nov 6, 2023

Uh oh!

charliermarsh commented Nov 6, 2023

Uh oh!

BurntSushi commented Nov 6, 2023

Uh oh!

charliermarsh commented Nov 6, 2023

Uh oh!

github-actions bot commented Nov 6, 2023

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

charliermarsh commented Nov 6, 2023

Uh oh!

zanieb left a comment

Choose a reason for hiding this comment

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BurntSushi commented Nov 6, 2023 •

edited

Loading

`ruff-ecosystem` results