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

tidy: enforce comment blocks to have an even number of backticks #108726

Merged
merged 6 commits into from
Mar 12, 2023

Conversation

est31
Copy link
Member

@est31 est31 commented Mar 4, 2023

After PR #108694, most unmatched backticks in compiler/ comments have been eliminated. This PR adds a tidy lint to ensure no new unmatched backticks are added, and either addresses the lint in the remaining instances it found, or allows it.

Very often, backtick containing sections wrap around lines, for example:

// This function takes a tuple `(Vec<String>,
// Box<[u8]>)` and transforms it into `Vec<u8>`.

The lint is implemented to work on top of blocks, counting each line with a // into a block, and counting if there are an odd or even number of backticks in the entire block, instead of looking at just a single line.

@rustbot
Copy link
Collaborator

rustbot commented Mar 4, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 4, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 4, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@est31
Copy link
Member Author

est31 commented Mar 4, 2023

r? @Nilstrieb

@rustbot rustbot assigned Noratrieb and unassigned albertlarsan68 Mar 4, 2023
@est31 est31 force-pushed the backticks_matchmaking_tidy branch from ac50e29 to 1af3b8c Compare March 4, 2023 06:02
src/tools/tidy/src/style.rs Outdated Show resolved Hide resolved
@the8472
Copy link
Member

the8472 commented Mar 4, 2023

Have you measured the runtime impact of this change? style.rs is one of the longer-running checks in tidy.

@est31
Copy link
Member Author

est31 commented Mar 4, 2023

Have you measured the runtime impact of this change?

Doing it now, I don't really see much of an impact.

The three times I measured all of tidy before:

real    0m3,520s
user    0m5,624s
sys     0m2,584s

real    0m3,588s
user    0m5,664s
sys     0m2,478s

real    0m3,558s
user    0m5,778s
sys     0m2,557s

real    0m3,563s
user    0m5,697s
sys     0m2,591s

after:

real    0m3,548s
user    0m5,876s
sys     0m2,587s

real    0m3,549s
user    0m5,670s
sys     0m2,646s

real    0m3,984s
user    0m5,574s
sys     0m2,653s

real    0m3,588s
user    0m5,783s
sys     0m2,650s

That is, running time ./x test tidy, after having commented out the rustfmt invocation in src/bootstrap/test.rs.

@est31 est31 force-pushed the backticks_matchmaking_tidy branch from 1af3b8c to 4612baf Compare March 4, 2023 20:41
@est31
Copy link
Member Author

est31 commented Mar 8, 2023

Regarding the commit which adds checking to the ftl files, the lint would have told about the ftl string bugs that were (among non-ftl issues) fixed by ef65890 .

For the other tidy checks, they would have told about the odd backtick numbers in comments as fixed by #108694 and #108685.

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

r=me with or without removing the FIXME

(I guess technically T-bootstrap is in charge of tidy? But this is just for the compiler, seems fine to me. whatever.)

@est31
Copy link
Member Author

est31 commented Mar 11, 2023

@Nilstrieb note that there is a second fixme, see the "add two fixmes" commit.

@est31 est31 force-pushed the backticks_matchmaking_tidy branch from ab70d3e to 4b04f76 Compare March 11, 2023 19:27
@Noratrieb
Copy link
Member

the second one can just be deleted i guess, that comment isn't complete anyways.

est31 and others added 6 commits March 11, 2023 20:40
Some comments may be formed like:

// This function takes a tuple `(Vec<String>,
// Box<[u8]>)` and transforms it into `Vec<u8>`.

where the "back-ticked" section wraps around.
Therefore, we can't make a single-line based
lint.

We also cannot make the lint paragraph based,
as it would otherwise complain about inline
code blocks:

/// ```
/// use super::Foo;
///
/// fn main() { Foo::new(); }
/// ```

For the future, one could introduce some checks
to treat code blocks specially, but such code
would make the check even more complicated.
Co-authored-by: nils <48135649+Nilstrieb@users.noreply.github.com>
@est31
Copy link
Member Author

est31 commented Mar 11, 2023

@Nilstrieb it should be ready now!

@est31 est31 force-pushed the backticks_matchmaking_tidy branch from 4b04f76 to b2aeb07 Compare March 11, 2023 21:07
@Noratrieb
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 11, 2023

📌 Commit b2aeb07 has been approved by Nilstrieb

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 12, 2023
…r=Nilstrieb

tidy: enforce comment blocks to have an even number of backticks

After PR rust-lang#108694, most unmatched backticks in `compiler/` comments have been eliminated. This PR adds a tidy lint to ensure no new unmatched backticks are added, and either addresses the lint in the remaining instances it found, or allows it.

Very often, backtick containing sections wrap around lines, for example:

```Rust
// This function takes a tuple `(Vec<String>,
// Box<[u8]>)` and transforms it into `Vec<u8>`.
```

The lint is implemented to work on top of blocks, counting each line with a `//` into a block, and counting if there are an odd or even number of backticks in the entire block, instead of looking at just a single line.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#108726 (tidy: enforce comment blocks to have an even number of backticks)
 - rust-lang#108797 (Allow binary files to go through the `FileLoader`)
 - rust-lang#108841 (Add suggestion to diagnostic when user has array but trait wants slice. (rebased))
 - rust-lang#108984 (bootstrap: document tidy)
 - rust-lang#109013 (Give proper error message when tcx wasn't passed to decoder)
 - rust-lang#109017 (remove duplicated calls to sort_string)
 - rust-lang#109018 (Expand on the allocator comment in `rustc-main`)
 - rust-lang#109028 (Add eslint checks for rustdoc-js tester)
 - rust-lang#109034 (Commit some tests for the new solver + lazy norm)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9668ae5 into rust-lang:master Mar 12, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants