empty_enum_variants_with_brackets: Do not lint reachable enums and enum variants used as functions in the same crate

[ARandomDev99]

Fixes #12551

changelog: [empty_enum_variants_with_brackets]: Do not lint reachable enums or enums which are used as functions within the same crate.

r? @xFrednet

[xFrednet]

@y21 Do you want to take over the review, since you already seems to be in the domain? Otherwise, I'm happy to take it.

Assuming that @ARandomDev99 didn't have a specific reason to r? me in the description.

[ARandomDev99]

I'm having trouble emitting a multipart suggestion that removes the parentheses only.

[y21]

You can make that is_callee function return an Option<Span>. Probably makes sense to also rename the function at that point (maybe call_parentheses_span?).

parent.span.with_lo(expr.span.hi()) should give you the parentheses only so you can return that from the function and push it into the vec

[ARandomDev99]

I just ran cargo test after cloning my fork into a new directory. All the tests passed. I wonder why it failed on GitHub.

[y21]

Sorry, completely missed the notification here. Does it repro if you rebase onto master? There have been a bunch of changes to uitest since then

[y21]

I just looked at the lintcheck CI results and looks like there's a fair amount of false positives: https://github.com/rust-lang/rust-clippy/actions/runs/10405234866
E.g.

warning: enum variant has empty brackets
  --> target/lintcheck/sources/addr2line-0.24.0/src/frame.rs:40:11
   |
40 |     Frames(FrameIterFrames<'ctx, R>),
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
help: remove the brackets
   |
40 ~     Frames,
41 | }

I think this might be because we're inserting new Usage::Unused into the map in check_expr, regardless of whether the variant actually has no fields. I imagine we could simply add a new variant to Usage for "found a use but not its definition", which is ignored in check_crate_post and "upgraded" to Usage::Unused in check_variant when we've verified it has no fields?
Or we could also add the has_no_fields condition in check_expr as well (before inserting Usage::Unused), essentially mirroring check_variant. Might be easier.

[ARandomDev99]

I fail to understand why false positives like this show up. There's even passing tests for exactly this situation.

[y21]

Why pass None here instead of the variant?

[y21]

Also, the span that's passed here is the wrong one. It expects the parentheses at the variant definition site, but this is passing the parentheses of the function call.

It doesn't explain the false positives, but passing this span makes has_no_fields check for the wrong thing (since it uses the span for checking for #[cfg]s).
TBH, I'd just get rid of the empty_parentheses function entirely and use !span_contains_cfg() instead.

[y21]

The false positives seem to happen when the constructor call comes from a derive macro like #[derive(Clone)] with its output tokens all having the same span, which makes the parentheses_span empty.
So a test case that runs into it would be something like

#[derive(Clone)]
enum Foo {
  Variant(i32)
}

---

Thinking more about it, it might actually be better and easier to have a new Usage::NoDefinition { redundant_use_sites: Vec<Span> } or something as mentioned in my other comment (which is ignored in check_crate_post but would get turned into Usage::Unused in check_variant after having done all checks), so we wouldn't need to repeat the logic in check_expr. Otherwise one would need to remember to update both functions check_variant and check_expr whenever more logic is added.

[ARandomDev99]

In addition to the changes you suggested, ensuring that expressions from macro expansions aren't included in redundant_use_sites seems to have fixed the issue.

[ARandomDev99]

@rustbot ready

[y21]

Sorry for taking a bit longer 😅 Apart from one small thing this LGTM, @xFrednet since you're also assigned, do you want to look over this too?

I do want to look at a lintcheck run here, but it seems like the last one expired and can't be viewed anymore, so I'll also look at it the next push to see if there are remaining FPs

[y21]

Seems like this function is always called with Some(..), can it just be &VariantData like before?

[xFrednet]

@y21 Thanks for the ping, I fully trust your review! So, don't wait for me to take a second look at it :)

[rustbot]

☔ The latest upstream changes (possibly d28d234) made this pull request unmergeable. Please resolve the merge conflicts.