Simplify LinterResult, avoid cloning ParseError
#11903
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dhruvmanila
force-pushed
the
dhruv/syntax-error-3
branch
3 times, most recently
from
085e9a4
to
9bc3076
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-4
branch
from
ae00c4e
to
e4c6822
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-3
branch
from
9bc3076
to
c54a6c1
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-4
branch
from
e4c6822
to
4ea7ff5
Compare
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| ANN101 | 35381 | 0 | 35381 | 0 | 0 |
| ANN102 | 674 | 0 | 674 | 0 | 0 |
| S603 | 454 | 227 | 227 | 0 | 0 |
| E721 | 207 | 188 | 19 | 0 | 0 |
| S610 | 22 | 22 | 0 | 0 | 0 |
| S602 | 21 | 11 | 10 | 0 | 0 |
| S604 | 16 | 8 | 8 | 0 | 0 |
| S605 | 8 | 4 | 4 | 0 | 0 |
| F811 | 8 | 8 | 0 | 0 | 0 |
| PLR1701 | 3 | 0 | 3 | 0 | 0 |
| D107 | 2 | 1 | 1 | 0 | 0 |
| RUF100 | 2 | 2 | 0 | 0 | 0 |
| NPY201 | 1 | 1 | 0 | 0 | 0 |
| RUF024 | 1 | 1 | 0 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+261 -256 violations, +0 -0 fixes in 12 projects; 1 project error; 37 projects unchanged)
PlasmaPy/PlasmaPy (+1 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ src/plasmapy/diagnostics/langmuir.py:1396:23: NPY201 `np.trapz` will be removed in NumPy 2.0. Use `numpy.trapezoid` on NumPy 2.0, or ignore this warning on earlier versions.
apache/airflow (+171 -177 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ airflow/cli/commands/dag_command.py:309:14: S603 `subprocess` call: check for execution of untrusted input - airflow/cli/commands/dag_command.py:309:31: S603 `subprocess` call: check for execution of untrusted input + airflow/cli/commands/info_command.py:199:18: S603 `subprocess` call: check for execution of untrusted input - airflow/cli/commands/info_command.py:199:35: S603 `subprocess` call: check for execution of untrusted input + airflow/cli/commands/internal_api_command.py:166:17: S603 `subprocess` call: check for execution of untrusted input - airflow/cli/commands/internal_api_command.py:166:34: S603 `subprocess` call: check for execution of untrusted input ... 297 additional changes omitted for rule S603 + airflow/example_dags/example_kubernetes_executor.py:132:35: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` - airflow/example_dags/example_kubernetes_executor.py:132:45: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + airflow/example_dags/example_kubernetes_executor.py:94:27: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` - airflow/example_dags/example_kubernetes_executor.py:94:37: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + airflow/providers/apache/beam/hooks/beam.py:575:25: S604 Function call with `shell=True` parameter identified, security issue - airflow/providers/apache/beam/hooks/beam.py:577:13: S604 Function call with `shell=True` parameter identified, security issue - airflow/providers/microsoft/azure/hooks/msgraph.py:327:12: PLR1701 [*] Merge `isinstance` calls: `isinstance(data, (BytesIO, bytes, str))` - airflow/serialization/pydantic/dag.py:45:9: PLR1701 [*] Merge `isinstance` calls - airflow/serialization/pydantic/taskinstance.py:70:8: PLR1701 [*] Merge `isinstance` calls: `isinstance(x, (BaseOperator, MappedOperator))` - airflow/www/extensions/init_appbuilder.py:1:1: CPY001 Missing copyright notice at top of file - airflow/www/fab_security/manager.py:1:1: CPY001 Missing copyright notice at top of file + dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1082:13: S604 Function call with `shell=True` parameter identified, security issue - dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1091:17: S604 Function call with `shell=True` parameter identified, security issue + dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1094:13: S604 Function call with `shell=True` parameter identified, security issue - dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1103:17: S604 Function call with `shell=True` parameter identified, security issue ... 11 additional changes omitted for rule S604 + hatch_build.py:660:13: S602 `subprocess` call with `shell=True` identified, security issue - hatch_build.py:660:59: S602 `subprocess` call with `shell=True` identified, security issue + hatch_build.py:673:13: S602 `subprocess` call with `shell=True` identified, security issue - hatch_build.py:673:59: S602 `subprocess` call with `shell=True` identified, security issue + scripts/ci/pre_commit/ruff_format.py:26:1: S602 `subprocess` call with `shell=True` identified, security issue - scripts/ci/pre_commit/ruff_format.py:26:33: S602 `subprocess` call with `shell=True` identified, security issue ... 11 additional changes omitted for rule S602 + tests/dags/test_on_kill.py:44:13: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` - tests/dags/test_on_kill.py:44:23: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` ... 3 additional changes omitted for rule S605 - tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:1:1: CPY001 Missing copyright notice at top of file ... 318 additional changes omitted for project
bokeh/bokeh (+38 -38 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ examples/output/apis/server_document/flask_server.py:45:17: S603 `subprocess` call: check for execution of untrusted input - examples/output/apis/server_document/flask_server.py:46:5: S603 `subprocess` call: check for execution of untrusted input + release/system.py:43:18: S602 `subprocess` call with `shell=True` identified, security issue - release/system.py:43:34: S602 `subprocess` call with `shell=True` identified, security issue - scripts/hooks/install.py:5:20: S603 `subprocess` call: check for execution of untrusted input + scripts/hooks/install.py:5:5: S603 `subprocess` call: check for execution of untrusted input + scripts/hooks/protect_branches.py:10:22: S603 `subprocess` call: check for execution of untrusted input - scripts/hooks/protect_branches.py:10:26: S603 `subprocess` call: check for execution of untrusted input ... 69 additional changes omitted for rule S603 ... 68 additional changes omitted for project
freedomofpress/securedrop (+2 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ devops/scripts/verify-mo.py:116:16: S602 `subprocess` call with `shell=True` identified, security issue + devops/scripts/verify-mo.py:120:26: RUF100 [*] Unused `noqa` directive (unused: `S602`)
fronzbot/blinkpy (+2 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ blinksync/blinksync.py:58:53: F811 Redefinition of unused `working` from line 55 + blinksync/blinksync.py:81:53: F811 Redefinition of unused `working` from line 78
ibis-project/ibis (+3 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ ibis/expr/types/strings.py:1678:9: F811 Redefinition of unused `__mul__` from line 638 + ibis/expr/types/strings.py:417:9: F811 Redefinition of unused `initcap` from line 412 + ibis/tests/benchmarks/test_benchmarks.py:720:34: F811 Redefinition of unused `con` from line 705
prefecthq/prefect (+1 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ src/prefect/cli/cloud/__init__.py:269:42: F811 Redefinition of unused `timeout_scope` from line 244
rotki/rotki (+4 -3 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ packaging/docker/entrypoint.py:144:11: S603 `subprocess` call: check for execution of untrusted input - packaging/docker/entrypoint.py:144:28: S603 `subprocess` call: check for execution of untrusted input - packaging/docker/entrypoint.py:166:26: S603 `subprocess` call: check for execution of untrusted input + packaging/docker/entrypoint.py:166:9: S603 `subprocess` call: check for execution of untrusted input - packaging/docker/entrypoint.py:174:52: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + packaging/docker/entrypoint.py:174:9: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + rotkehlchen/tests/exchanges/test_kraken.py:654:58: F811 Redefinition of unused `cursor` from line 653
zulip/zulip (+37 -38 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ scripts/lib/check_rabbitmq_queue.py:143:26: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/check_rabbitmq_queue.py:144:9: S603 `subprocess` call: check for execution of untrusted input + scripts/lib/check_rabbitmq_queue.py:160:23: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/check_rabbitmq_queue.py:161:9: S603 `subprocess` call: check for execution of untrusted input + scripts/lib/hash_reqs.py:38:12: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/hash_reqs.py:38:36: S603 `subprocess` call: check for execution of untrusted input ... 69 additional changes omitted for rule S603 - zerver/lib/markdown/fenced_code.py:1:1: CPY001 Missing copyright notice at top of file ... 68 additional changes omitted for project
agronholm/anyio (+1 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ tests/test_taskgroups.py:557:27: RUF100 [*] Unused `noqa` directive (unknown: `ASYNC101`)
python-trio/trio (+0 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
encode/httpx (+1 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ tests/client/test_auth.py:371:73: F811 Redefinition of unused `client` from line 366
demisto/content (error)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
ruff failed
Cause: Failed to parse /home/runner/work/ruff/ruff/checkouts/demisto:content/pyproject.toml
Cause: TOML parse error at line 92, column 1
|
92 | [tool.ruff]
| ^^^^^^^^^^^
Unknown rule selector: `E999`
Changes by rule (9 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| S603 | 454 | 227 | 227 | 0 | 0 |
| S602 | 21 | 11 | 10 | 0 | 0 |
| S604 | 16 | 8 | 8 | 0 | 0 |
| S605 | 8 | 4 | 4 | 0 | 0 |
| F811 | 8 | 8 | 0 | 0 | 0 |
| CPY001 | 4 | 0 | 4 | 0 | 0 |
| PLR1701 | 3 | 0 | 3 | 0 | 0 |
| RUF100 | 2 | 2 | 0 | 0 | 0 |
| NPY201 | 1 | 1 | 0 | 0 | 0 |
dhruvmanila
commented
Jun 19, 2024
crates/ruff_linter/src/linter.rs
Outdated
Comment on lines
39
to
44
| /// A [`Result`]-like type that returns both data and an error. Used to return | ||
| /// diagnostics even in the face of parse errors, since many diagnostics can be | ||
| /// generated without a full AST. | ||
| pub struct LinterResult<T> { | ||
| pub data: T, | ||
| pub error: Option<ParseError>, | ||
| } | ||
|
|
||
| impl<T> LinterResult<T> { | ||
| const fn new(data: T, error: Option<ParseError>) -> Self { | ||
| Self { data, error } | ||
| } | ||
|
|
||
| fn map<U, F: FnOnce(T) -> U>(self, f: F) -> LinterResult<U> { | ||
| LinterResult::new(f(self.data), self.error) | ||
| } | ||
| pub struct LinterResult { | ||
| /// A collection of diagnostic messages generated by the linter. | ||
| pub messages: Vec<Message>, | ||
| /// A flag indicating the presence of syntax errors in the source file. | ||
| /// If `true`, at least one syntax error was detected in the source file. | ||
| pub has_error: bool, | ||
| } |
There was a problem hiding this comment.
The main change is here where we avoid the generic T and instead use a concrete type for the linter result.
Comment on lines
-83
to
89
| if let Some(error) = error { | ||
| if has_error { | ||
| // abort early if a parsing error occurred | ||
| return Err(anyhow::anyhow!( | ||
| "A parsing error occurred during `fix_all`: {error}" | ||
| )); | ||
| return Err(anyhow::anyhow!("A parsing error occurred during `fix_all`")); | ||
| } |
Comment on lines
-139
to
+140
| let mut diagnostics = Diagnostics::default(); | ||
| let mut diagnostics_map = DiagnosticsMap::default(); |
There was a problem hiding this comment.
This rename is mainly to avoid confusing it with the actual list of diagnostics which is termed as "diagnostics" above.
dhruvmanila
force-pushed
the
dhruv/syntax-error-3
branch
from
c54a6c1
to
c531897
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-4
branch
from
4ea7ff5
to
d7346a0
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-3
branch
from
c531897
to
7591502
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-4
branch
from
d7346a0
to
59c7f0b
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-3
branch
from
7591502
to
0fe0d6d
Compare
dhruvmanila
force-pushed
the
dhruv/syntax-error-4
branch
from
59c7f0b
to
e04b401
Compare
CodSpeed Performance ReportMerging #11903 will degrade performances by 4.15%Comparing Summary
Benchmarks breakdown
|
dhruvmanila
added a commit
that referenced
this pull request
Jun 27, 2024
## Summary Follow-up to #11902 This PR simplifies the `LinterResult` struct by avoiding the generic and not store the `ParseError`. This is possible because the callers already have access to the `ParseError` via the `Parsed` output. This also means that we can simplify the return type of `check_path` and avoid the generic `T` on `LinterResult`. ## Test Plan `cargo insta test`
dhruvmanila
added a commit
that referenced
this pull request
Jun 27, 2024
## Summary Follow-up to #11902 This PR simplifies the `LinterResult` struct by avoiding the generic and not store the `ParseError`. This is possible because the callers already have access to the `ParseError` via the `Parsed` output. This also means that we can simplify the return type of `check_path` and avoid the generic `T` on `LinterResult`. ## Test Plan `cargo insta test`
MichaReiser
pushed a commit
that referenced
this pull request
Jun 27, 2024
## Summary Follow-up to #11902 This PR simplifies the `LinterResult` struct by avoiding the generic and not store the `ParseError`. This is possible because the callers already have access to the `ParseError` via the `Parsed` output. This also means that we can simplify the return type of `check_path` and avoid the generic `T` on `LinterResult`. ## Test Plan `cargo insta test`
MichaReiser
pushed a commit
that referenced
this pull request
Jun 27, 2024
## Summary Follow-up to #11902 This PR simplifies the `LinterResult` struct by avoiding the generic and not store the `ParseError`. This is possible because the callers already have access to the `ParseError` via the `Parsed` output. This also means that we can simplify the return type of `check_path` and avoid the generic `T` on `LinterResult`. ## Test Plan `cargo insta test`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #11902
This PR simplifies the
LinterResultstruct by avoiding the generic and not store theParseError.This is possible because the callers already have access to the
ParseErrorvia theParsedoutput. This also means that we can simplify the return type ofcheck_pathand avoid the genericTonLinterResult.Test Plan
cargo insta test