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

Remove allow(pedantic) from formatter #6549

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 14, 2023

Summary

This PR removes the module level allow(pedantic) from the ruff_formatter crate and fixes (or allows) the clippy violations.

Why didn't we do this already? Computing the length of the input string required traversing the whole CST (nodes + tokens + trivia) in Rome. This by itself is much more expensive than resizing the buffer.

Test Plan

cargo test

@MichaReiser
Copy link
Member Author

MichaReiser commented Aug 14, 2023

@MichaReiser MichaReiser force-pushed the formatter-remove-allow-pedantic branch from d88c086 to 01d0c1d Compare August 14, 2023 07:08
@MichaReiser MichaReiser force-pushed the formatter-remove-allow-pedantic branch from 01d0c1d to 008773a Compare August 14, 2023 07:10
@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.03      4.1±0.03ms     9.9 MB/sec    1.00      4.0±0.01ms    10.3 MB/sec
formatter/numpy/ctypeslib.py               1.04    777.8±3.77µs    21.4 MB/sec    1.00    744.4±3.08µs    22.4 MB/sec
formatter/numpy/globals.py                 1.07     79.8±0.84µs    37.0 MB/sec    1.00     74.6±0.89µs    39.6 MB/sec
formatter/pydantic/types.py                1.03  1647.1±11.20µs    15.5 MB/sec    1.00   1594.9±9.28µs    16.0 MB/sec
linter/all-rules/large/dataset.py          1.00     10.3±0.03ms     4.0 MB/sec    1.00     10.3±0.02ms     4.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.8±0.02ms     6.0 MB/sec    1.00      2.8±0.01ms     6.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    394.5±2.07µs     7.5 MB/sec    1.01    396.8±2.71µs     7.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.3±0.02ms     4.8 MB/sec    1.00      5.4±0.03ms     4.8 MB/sec
linter/default-rules/large/dataset.py      1.00      5.4±0.01ms     7.5 MB/sec    1.00      5.4±0.02ms     7.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1198.7±9.92µs    13.9 MB/sec    1.01  1208.2±14.79µs    13.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    144.6±5.08µs    20.4 MB/sec    1.04    149.9±6.57µs    19.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.01ms    10.4 MB/sec    1.00      2.5±0.02ms    10.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.5±0.06ms     9.0 MB/sec    1.03      4.7±0.07ms     8.7 MB/sec
formatter/numpy/ctypeslib.py               1.00   852.1±14.67µs    19.5 MB/sec    1.01   863.6±13.21µs    19.3 MB/sec
formatter/numpy/globals.py                 1.00     87.6±2.67µs    33.7 MB/sec    1.01     88.8±2.94µs    33.2 MB/sec
formatter/pydantic/types.py                1.00  1865.7±36.06µs    13.7 MB/sec    1.02  1902.8±32.46µs    13.4 MB/sec
linter/all-rules/large/dataset.py          1.00     13.0±0.17ms     3.1 MB/sec    1.01     13.2±0.23ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.06ms     4.7 MB/sec    1.01      3.6±0.05ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    441.5±5.80µs     6.7 MB/sec    1.01    447.0±8.15µs     6.6 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.8±0.20ms     3.7 MB/sec    1.00      6.8±0.13ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      7.1±0.09ms     5.7 MB/sec    1.02      7.2±0.09ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1483.6±16.12µs    11.2 MB/sec    1.03  1522.9±22.67µs    10.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    173.9±2.43µs    17.0 MB/sec    1.02    177.9±4.60µs    16.6 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.04ms     8.0 MB/sec    1.02      3.2±0.05ms     7.9 MB/sec

@MichaReiser MichaReiser force-pushed the remove-unused-printer-fields branch from 7f83eeb to 055af52 Compare August 14, 2023 08:21
@MichaReiser MichaReiser force-pushed the formatter-remove-allow-pedantic branch from 008773a to 70c5ff5 Compare August 14, 2023 08:21
@MichaReiser MichaReiser mentioned this pull request Aug 14, 2023
@MichaReiser MichaReiser added the internal An internal refactor or improvement label Aug 14, 2023
@MichaReiser MichaReiser force-pushed the remove-unused-printer-fields branch from 055af52 to 8eb36fa Compare August 14, 2023 08:26
@MichaReiser MichaReiser force-pushed the formatter-remove-allow-pedantic branch from 70c5ff5 to 206b837 Compare August 14, 2023 08:26
Base automatically changed from remove-unused-printer-fields to main August 14, 2023 09:08
@MichaReiser MichaReiser force-pushed the formatter-remove-allow-pedantic branch from 206b837 to 301fe5c Compare August 14, 2023 09:59
@MichaReiser MichaReiser requested a review from konstin August 14, 2023 10:00
@MichaReiser MichaReiser merged commit 9584f61 into main Aug 14, 2023
@MichaReiser MichaReiser deleted the formatter-remove-allow-pedantic branch August 14, 2023 12:02
lifetime: PhantomData,
formatter: formatter::<F, Context>,
}
}

/// Formats the value stored by this argument using the given formatter.
#[allow(clippy::inline_always)]
Copy link
Member

Choose a reason for hiding this comment

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

The docs says (https://rust-lang.github.io/rust-clippy/v0.0.212/index.html#inline_always):

While there are valid uses of this annotation (and once you know when to use it, by all means allow this lint), it’s a common newbie-mistake to pepper one’s code with it. As a rule of thumb, before slapping #[inline(always)] on a function, measure if that additional function call really affects your runtime profile sufficiently to make up for the increase in compile time.

Do we have measurements that #[inline(always)] is better than #[inline]?

err.type_id(),
(*err).type_id(),
Copy link
Member

Choose a reason for hiding this comment

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

Do you know which rule triggered here?

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 think it was calling type_id on Box<dyn >

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants