Convert `EMPTY_LINE_AFTER_OUTER_ATTR` and `EMPTY_LINE_AFTER_OUTER_ATTR` lint into early lints #13658

GuillaumeGomez · 2024-11-05T20:49:37Z

This is needed to help the compiler attributes API rewrite.

changelog: Convert EMPTY_LINE_AFTER_OUTER_ATTR and EMPTY_LINE_AFTER_OUTER_ATTR lint into early lints

rustbot · 2024-11-05T20:49:43Z

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

jdonszelmann · 2024-11-05T20:50:29Z

yayyy

jdonszelmann · 2024-11-05T20:51:38Z

clippy_lints/src/doc/mod.rs

@@ -690,7 +611,7 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[
        Some(("fake".into(), "fake".into()))
    }

-    if suspicious_doc_comments::check(cx, attrs) || empty_line_after::check(cx, attrs) || is_doc_hidden(attrs) {


It said this check was to avoid spurious warnings somewhere, but we don't do it anymore. Is that a problem?

I ran cargo uibless so I think it's ok if nothing new got picked up. To be confirmed by someone more knowledgeable though.

It's intended for e.g. #12917

So what should we do here?

It may cause some confusion but if attrs are being reworked it's something we could live without unless we want to add another early -> late storage hack

jdonszelmann · 2024-11-05T20:52:31Z

clippy_lints/src/empty_line_after.rs

+impl Stop {
+    fn convert_to_inner(&self) -> (Span, String) {
+        let inner = match self.kind {
+            // #|[...]


nitpick, but this should be #! not #| I think

jdonszelmann · 2024-11-05T20:54:09Z

tests/ui/empty_line_after/doc_comments.stderr

@@ -5,7 +5,7 @@ LL | / /// for the crate
 LL | |
   | |_
 LL |   fn first_in_crate() {}
-   |   ------------------- the comment documents this function
+   |   ---------------------- the comment documents this function


this now points to the entire function, while previously it pointed to the signature of the function. That'd mean that if the function is longer than 0 lines it'd also highlight the block. Do we want that?

I supposed it was fine but I can try to see if it can be shortened somehow.

rustc_ast::FnSig has a span

jdonszelmann · 2024-11-05T20:54:35Z

tests/ui/empty_line_after/outer_attribute.stderr

@@ -71,8 +74,11 @@ error: empty line after outer attribute
 LL | / #[repr(C)]
 LL | |
   | |_
-LL |   struct Foo {
-   |   ---------- the attribute applies to this struct
+LL | / struct Foo {


same here I guess, you can see it better here

…R` lint into early lints

jdonszelmann · 2024-11-05T21:02:39Z

much better

Alexendoo · 2024-11-05T21:44:24Z

Is there a link to why this is needed?

jdonszelmann · 2024-11-05T21:50:35Z

So as part of rust-lang/compiler-team#796, I bumped into a few issues with clippy where some lints are late, but use a lot of information that mostly relates to the AST, and have very little reason to interact with the hir. After lowering, clippy is the only part of the compiler using information such as the AttrId, AttrStyle and Span on attributes.

By moving these lints to be early:

attributes, after lowering, can be only 8 bytes instead of their current 48
the refactor to pre-parsed attributes becomes easier
moving one of the lints to early (not this PR) we actually solved a bug with attributes being emitted twice

Hope that clears some things up :)

jdonszelmann · 2024-11-05T21:53:03Z

for 2), that might mean that at some point in the future I will revisit these lints and make them use the new attribute parsing logic. By making them early, they can enjoy the old parsing logic for a little longer while the new parsing logic can be implemented without having to fix clippy at the same time as introducing a large refactoring in the compiler.

flip1995 · 2024-11-06T12:23:41Z

Lintcheck diff is quite big and some of them look like FPs. Now I'm even more concerned that rust-lang/rust#132598 might've introduced FPs... We will see tomorrow, once I can do the sync.

GuillaumeGomez · 2024-11-06T14:19:21Z

A lot of them are because of message changes, like:

 8 |   macro_rules! test_for_each_provider {
-  |   ----------------------------------- the comment documents this macro
+  |   ----------------------------------- the comment documents this macro definition

or of span changes like:

 51 |   struct FlatMapConsumer<'f, C, F> {
-   |   -------------------------------- the comment documents this struct
+   |   ---------------------- the comment documents this struct

For changes like:

Added clippy::doc_lazy_continuation at [url-2.5.2/src/lib.rs:379](https://docs.rs/url/2.5.2/src/url/lib.rs.html#379)

```
warning: doc list item without indentation
   --> target/lintcheck/sources/url-2.5.2/src/lib.rs:379:9
    |
379 |     /// Without it, the last path component is considered to be a “file” name
    |         ^
    |
    = help: if this is supposed to be its own paragraph, add a blank line
    = note: `--force-warn clippy::doc-lazy-continuation` implied by `--force-warn clippy::all`
help: indent this line
    |
379 |     ///   Without it, the last path component is considered to be a “file” name
    |         ++
```

are because of what was discussed here. In short: since it's not part of the doc lints anymore (one of the 3 that was triggering an early return), some more lints are emitted. It's not incorrect but will generate more warnings.

flip1995 · 2024-11-06T18:00:28Z

Just went through all of them: There's actually only 1 FP and I'm not sure if it is related to this PR:

warning: backticks are unbalanced
   --> target/lintcheck/sources/chrono-0.4.38/src/month.rs:191:9
    |
191 |       /// `Month::from_i64(n: i64)`: | `1`                  | `2`                   | ... | `12`
    |  _________^
192 | |     /// ---------------------------| -------------------- | --------------------- | ... | -----
193 | |     /// ``:                        | Some(Month::January) | Some(Month::February) | ... | Some(Month::December)
    | |_______________________________________________________________________________________________________________^
    |
    = help: a backtick may be missing a pair

In chrono-0.4.38/src/month.rs:191. Might be because of the double backticks in the 3rd line. But the span is then wrong.

GuillaumeGomez · 2024-11-07T19:43:25Z

That seems like a bug into another lint that might have been uncovered by this PR.

bors · 2024-11-21T22:12:06Z

☔ The latest upstream changes (presumably 8298da7) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned y21 Nov 5, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 5, 2024

GuillaumeGomez force-pushed the migrate-empty_after branch from b7f66a1 to 0f8ba1c Compare November 5, 2024 20:50

jdonszelmann reviewed Nov 5, 2024

View reviewed changes

GuillaumeGomez force-pushed the migrate-empty_after branch from 5a15864 to 7089e72 Compare November 5, 2024 21:01

GuillaumeGomez added 3 commits November 5, 2024 22:02

Convert EMPTY_LINE_AFTER_OUTER_ATTR and `EMPTY_LINE_AFTER_OUTER_ATT…

394503c

…R` lint into early lints

Update UI tests

07f97a4

Fix typo

1f9ccc2

GuillaumeGomez force-pushed the migrate-empty_after branch from 7089e72 to 1f9ccc2 Compare November 5, 2024 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `EMPTY_LINE_AFTER_OUTER_ATTR` and `EMPTY_LINE_AFTER_OUTER_ATTR` lint into early lints #13658

Convert `EMPTY_LINE_AFTER_OUTER_ATTR` and `EMPTY_LINE_AFTER_OUTER_ATTR` lint into early lints #13658

GuillaumeGomez commented Nov 5, 2024

rustbot commented Nov 5, 2024

jdonszelmann commented Nov 5, 2024

jdonszelmann Nov 5, 2024

GuillaumeGomez Nov 5, 2024

Alexendoo Nov 5, 2024

GuillaumeGomez Nov 5, 2024

Alexendoo Nov 18, 2024

jdonszelmann Nov 5, 2024

GuillaumeGomez Nov 5, 2024

jdonszelmann Nov 5, 2024

GuillaumeGomez Nov 5, 2024

jdonszelmann Nov 5, 2024 •

edited

Loading

jdonszelmann Nov 5, 2024

jdonszelmann commented Nov 5, 2024

Alexendoo commented Nov 5, 2024

jdonszelmann commented Nov 5, 2024

jdonszelmann commented Nov 5, 2024

flip1995 commented Nov 6, 2024 •

edited

Loading

GuillaumeGomez commented Nov 6, 2024

flip1995 commented Nov 6, 2024

GuillaumeGomez commented Nov 7, 2024

bors commented Nov 21, 2024

Convert EMPTY_LINE_AFTER_OUTER_ATTR and EMPTY_LINE_AFTER_OUTER_ATTR lint into early lints #13658

Are you sure you want to change the base?

Convert EMPTY_LINE_AFTER_OUTER_ATTR and EMPTY_LINE_AFTER_OUTER_ATTR lint into early lints #13658

Conversation

GuillaumeGomez commented Nov 5, 2024

rustbot commented Nov 5, 2024

jdonszelmann commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdonszelmann Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdonszelmann commented Nov 5, 2024

Alexendoo commented Nov 5, 2024

jdonszelmann commented Nov 5, 2024

jdonszelmann commented Nov 5, 2024

flip1995 commented Nov 6, 2024 • edited Loading

GuillaumeGomez commented Nov 6, 2024

flip1995 commented Nov 6, 2024

GuillaumeGomez commented Nov 7, 2024

bors commented Nov 21, 2024

Convert `EMPTY_LINE_AFTER_OUTER_ATTR` and `EMPTY_LINE_AFTER_OUTER_ATTR` lint into early lints #13658

Convert `EMPTY_LINE_AFTER_OUTER_ATTR` and `EMPTY_LINE_AFTER_OUTER_ATTR` lint into early lints #13658

jdonszelmann Nov 5, 2024 •

edited

Loading

flip1995 commented Nov 6, 2024 •

edited

Loading