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

Fix ArgWithDefault comments handling #5204

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 20, 2023

Summary

This PR fixes a few issues with formatting arguments that were introduced when upgrading to the latest RustPython parser (#5192).

  • It upgrades RustPython again to pull in the fix that includes the range of the default argument in ArgWithDefault Fix ArgWithDefault TextRange RustPython-Parser#13
  • It overrides the visit_arg_with_default function in the CommentsVisitor
  • It fixes a nasty issue where both the ArgWithDefault and Arg resolved to the same comments. My understanding of the issue is that Rust re-orders the fields in ArgWithDefault so that Arg becomes its first field. The result is that both the ArgWithDefault and Arg are stored at the same memory offset (but they don't end at the same memory offset). I wonder if we have the same problem with RefEquality (CC: @charliermarsh). The way I fixed the issue is by including the node kind in the equality check. This is safe because we only have this problem for nodes that don't box their children (which is why `BinaryExpression isn't affected by it) and recursive types need boxing (the only way that the parent and child have the same kind).

Test Plan

I added a new test for formatting arguments.

@MichaReiser
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser requested a review from konstin June 20, 2023 06:41
@MichaReiser MichaReiser added the formatter Related to the formatter label Jun 20, 2023
@MichaReiser MichaReiser force-pushed the 06-20-Fix_ArgWithDefault_comments_handling branch from 7657211 to 84dd7db Compare June 20, 2023 06:45
@MichaReiser MichaReiser marked this pull request as ready for review June 20, 2023 06:45
@github-actions
Copy link
Contributor

github-actions bot commented Jun 20, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      6.4±0.01ms     6.4 MB/sec    1.00      6.3±0.01ms     6.4 MB/sec
formatter/numpy/ctypeslib.py               1.00   1343.8±1.72µs    12.4 MB/sec    1.00   1341.7±2.65µs    12.4 MB/sec
formatter/numpy/globals.py                 1.00    131.3±0.15µs    22.5 MB/sec    1.01    132.2±0.74µs    22.3 MB/sec
formatter/pydantic/types.py                1.00      2.6±0.00ms     9.7 MB/sec    1.00      2.6±0.01ms     9.7 MB/sec
linter/all-rules/large/dataset.py          1.00     12.9±0.02ms     3.1 MB/sec    1.00     12.9±0.01ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.3±0.00ms     5.1 MB/sec    1.00      3.3±0.01ms     5.1 MB/sec
linter/all-rules/numpy/globals.py          1.01    430.7±0.73µs     6.9 MB/sec    1.00    426.8±0.56µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.8±0.01ms     4.4 MB/sec    1.00      5.8±0.01ms     4.4 MB/sec
linter/default-rules/large/dataset.py      1.00      6.6±0.01ms     6.2 MB/sec    1.01      6.6±0.02ms     6.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1444.9±1.48µs    11.5 MB/sec    1.01   1461.0±1.71µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    164.1±0.22µs    18.0 MB/sec    1.03    168.9±0.26µs    17.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.0±0.00ms     8.5 MB/sec    1.01      3.0±0.00ms     8.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.5±0.08ms     5.4 MB/sec    1.00      7.6±0.05ms     5.4 MB/sec
formatter/numpy/ctypeslib.py               1.00  1551.8±18.30µs    10.7 MB/sec    1.01  1568.6±16.86µs    10.6 MB/sec
formatter/numpy/globals.py                 1.00    153.2±2.36µs    19.3 MB/sec    1.02    155.6±3.78µs    19.0 MB/sec
formatter/pydantic/types.py                1.00      3.1±0.06ms     8.2 MB/sec    1.03      3.2±0.09ms     7.9 MB/sec
linter/all-rules/large/dataset.py          1.00     15.3±0.17ms     2.7 MB/sec    1.01     15.5±0.17ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.0±0.07ms     4.1 MB/sec    1.01      4.1±0.05ms     4.1 MB/sec
linter/all-rules/numpy/globals.py          1.00   499.3±23.35µs     5.9 MB/sec    1.00    497.3±6.41µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.8±0.14ms     3.7 MB/sec    1.00      6.8±0.08ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      7.9±0.07ms     5.1 MB/sec    1.01      8.0±0.08ms     5.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1704.4±17.27µs     9.8 MB/sec    1.00  1711.2±18.88µs     9.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    198.0±3.97µs    14.9 MB/sec    1.01    199.7±3.42µs    14.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.03ms     7.1 MB/sec    1.00      3.6±0.03ms     7.1 MB/sec

@charliermarsh
Copy link
Member

Is this upstream or downstream of the other RustPython upgrade PRs I have up?

@MichaReiser
Copy link
Member Author

Is this upstream or downstream of the other RustPython upgrade PRs I have up?

Depends on who merges first ;) The PR is astral-sh/RustPython-Parser#13

@charliermarsh
Copy link
Member

Looks like I win

@MichaReiser MichaReiser force-pushed the 06-20-Fix_ArgWithDefault_comments_handling branch from 84dd7db to bc67cd0 Compare June 20, 2023 16:24
@MichaReiser MichaReiser enabled auto-merge (squash) June 20, 2023 16:24
@MichaReiser MichaReiser force-pushed the 06-20-Fix_ArgWithDefault_comments_handling branch from bc67cd0 to 511b47f Compare June 20, 2023 20:33
@MichaReiser MichaReiser merged commit e520a3a into main Jun 20, 2023
32 checks passed
@MichaReiser MichaReiser deleted the 06-20-Fix_ArgWithDefault_comments_handling branch June 20, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants