don't lint [mixed_attributes_style] when mixing docs and other attrs

[J-ZhengLi]

fixes: #12435
fixes: #12436
fixes: #12530

---

changelog: don't lint [mixed_attributes_style] when mixing different kind of attrs; and move it to late pass;

[rustbot]

r? @y21

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

[J-ZhengLi]

I know ui-cargo is probably not a good place to put these tests, but I don't know where else should I put them.

[GuillaumeGomez]

Instead of changing the lint pass and having to handle new cases, why not handle doc comments separately from the other attributes? Should be simpler, no?

[y21]

Pre expansion passes are broken and basically-deprecated afaik. Lint level attrs dont really work well on them so they should be avoided, and moving away from it is necessary for fixing #12436.

See #6610 for more context and where this has caused issues in the past

[GuillaumeGomez]

Oh I see, EarlyAttributes was in the pre-expansion pass. So let me correct my comment: why not keeping it into the early pass but as a post-expansion lint? (And why not doing the same for all other attribute lints?)

[y21]

why not keeping it into the early pass but as a post-expansion lint?

I wasn't sure if a post-expansion early pass wouldn't just run into the same issue.

I could be wrong on this, but my understanding of the issue here is that outlined mod foo; declarations start out as ModKind::Unloaded and so their inner attributes are not yet filled in when pre-expansion lints are run (and would be why it didn't lint on that initially).
But when changing it to a post-expansion pass, the macro expander will have expanded the outlined mod declarations and filled in the attributes. So I assumed that post-expansion early pass vs. late pass wouldn't make a difference in that regard; you'd see both inner and outer attributes for outlined mod decls no matter what.

(And why not doing the same for all other attribute lints?)

I'm not sure. One "downside" I suppose is that post-expansion lints don't see the disabled #[cfg] attributes anymore, so you wouldn't see all of the #[cfg] lints unless they're all active. But I don't know if that's the main reason (this argument could be made for all other lints).
For lints like mismatched_target_os that definitely sounds like an issue, since that one looks for cfgs that would never be true

[GuillaumeGomez]

I could be wrong on this, but my understanding of the issue here is that outlined mod foo; declarations start out as ModKind::Unloaded and so their inner attributes are not yet filled in when pre-expansion lints are run (and would be why it didn't lint on that initially).
But when changing it to a post-expansion pass, the macro expander will have expanded the outlined mod declarations and filled in the attributes. So I assumed that post-expansion early pass vs. late pass wouldn't make a difference in that regard; you'd see both inner and outer attributes for outlined mod decls no matter what.

I'm surprised that modules would get inlined in the AST pass. Would be worth checking it.

[y21]

I do think we should avoid linting on differing attributes, but I would also be fine with just landing the PR with allowing doc comments for now if that makes it easier (like it does rn), just so the lint level attribute bug is fixed and people who disagree with the lint can at least allow it properly

[y21]

I also tested it locally and yes, a post-expansion early lint pass also seems to have the same issue (inner attributes of the other file being part of the mod declaration), so I don't think we can really get around doing it like that

[J-ZhengLi]

I do think we should avoid linting on differing attributes, but I would also be fine with just landing the PR with allowing doc comments for now if that makes it easier (like it does rn), just so the lint level attribute bug is fixed and people who disagree with the lint can at least allow it properly

Make sense, I'll open another issue addressing the other problem

[J-ZhengLi]

I do think we should avoid linting on differing attributes, but I would also be fine with just landing the PR with allowing doc comments for now if that makes it easier (like it does rn)...

After experimenting on this a little bit, I ended up changes almost everything in the src file, might as well try to fix it...

The only problem was it still has some false-negative afaik. One of them being it's unable to detect the problem inside a nested module inside of test module, idk if it's by design or not, since is very specific. (If it's unexpected, I can put it in the lint's descriptions as well. I changed my mind, maybe an issue is better)
The other one is about the same attribute but with different path symbols, like #[foo] and #[krate::foo], that I have no idea how to prevent.

_{But hey, having FN is better than having FP right?!}

[workingjubilee]

Please prefer false negatives over false positives, especially for a novel lint!

[y21]

changes make sense to me, 👍 for also only linting on same attributes
just some minor comments and I think it's ready

[y21]

Inner proc macro attributes are unstable anyway so I would say that false negative is fine

[y21]

Looks good to me, just some doc nits I found
Can you also squash the commits?

[J-ZhengLi]

Looks good to me, just some doc nits I found Can you also squash the commits?

@y21 oops, mess up the commit message, should've put the last line first... but anyway, it should be good now, thank you for all the patience~

[y21]

yep, looks good, thank you for fixing these issues!

@bors r+

[bors]

📌 Commit e9f25b3 has been approved by y21

It is now in the queue for this repository.

[bors]

⌛ Testing commit e9f25b3 with merge db41621...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing db41621 to master...

[flip1995]

rust-lang/rust#124369