chore: enforce clippy::allow_attributes for 7 crates

[chakkk309]

Which issue does this PR close?

Part of #18881

Rationale for this change

Implement clippy::allow_attributes lint for 7 crates

What changes are included in this PR?

Core crates modified:

• datafusion-doc
• datafusion-execution
• datafusion-expr
• datafusion-expr-common
• datafusion-catalog-listing
• datafusion-core
• datafusion-proto-common

Are these changes tested?

yes

Are there any user-facing changes?

No

[Jefffrey]

Is removing the existing allow for needless_pass_by_value in tests intentional?

[chakkk309]

Yes, the CI was failing with "unfulfilled lint expectations" because the needless_pass_by_value lint wasn't actually being triggered in the test code.

[Jefffrey]

We might need to think of a way to keep this, as the intention is that for any future tests we have this lint disabled

[Jefffrey]

Is changing the allow to expect is strictly necessary here?

[chakkk309]

Not required here, but i think the expect gives us better feedback when code improves.

[alamb]

This PR looks better than what is on main to me. Thank you @chakkk309 and @Jefffrey

[alamb]

why not change this

#[expect(clippy::derived_hash_with_manual_eq)]

?

[chakkk309]

Hi, if we change #[allow(clippy::derived_hash_with_manual_eq)] to
#[expect(clippy::derived_hash_with_manual_eq)], that will be the error:

  error: you are deriving `Hash` but have implemented `PartialEq` explicitly
      --> datafusion/expr/src/logical_plan/plan.rs:3239:28
       |
  3239 | #[derive(Debug, Clone, Eq, Hash)]
       |                            ^^^^
       |
  note: `PartialEq` implemented here
      --> datafusion/expr/src/logical_plan/plan.rs:3248:1
       |
  3248 | impl PartialEq for Extension {
       | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#derived_hash_with_manual_eq
       = note: `#[deny(clippy::derived_hash_with_manual_eq)]` on by default

  warning: this lint expectation is unfulfilled
      --> datafusion/expr/src/logical_plan/plan.rs:3238:10
       |
  3238 | #[expect(clippy::derived_hash_with_manual_eq)]
       |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |
       = note: `#[warn(unfulfilled_lint_expectations)]` on by default

  warning: `datafusion-expr` (lib) generated 1 warning
  error: could not compile `datafusion-expr` (lib) due to 1 previous error; 1 warning emitted

Seems like expect can't override clippy's default deny-level lint derived_hash_with_manual_eq ：（, so maybe we need allow with a policy override here.

[alamb]

I think this PR is ready to go. @Jefffrey do you see any reason not to merge this?

[Jefffrey]

I think we can merge this, I'm just a bit unclear on some of the implications relating to the cfg_attr for tests; I don't know how they interact for example, and playing around locally with:

- #![cfg_attr(test, allow(clippy::needless_pass_by_value))]
+ #![cfg_attr(test, expect(clippy::needless_pass_by_value))]

I'm having a hard time understanding what exactly is the semantics here 🤔

Also removing some of the existing #![cfg_attr(test, allow(clippy::needless_pass_by_value))] in tests might make them drift with the rest of the test codebase as I assume people in the future running into this lint probably wouldn't add this exception back?

[chakkk309]

I think we can merge this, I'm just a bit unclear on some of the implications relating to the cfg_attr for tests; I don't know how they interact for example, and playing around locally with:

- #![cfg_attr(test, allow(clippy::needless_pass_by_value))]
+ #![cfg_attr(test, expect(clippy::needless_pass_by_value))]

I'm having a hard time understanding what exactly is the semantics here 🤔

Also removing some of the existing #![cfg_attr(test, allow(clippy::needless_pass_by_value))] in tests might make them drift with the rest of the test codebase as I assume people in the future running into this lint probably wouldn't add this exception back?

I see what you mean :) , expect warns when the lint doesn't trigger, while allow stays silent. I removed those test attributes because they showed unfulfilled expectations - the lint wasn't firing there. I'd rather suppress only where it triggers, and let developers see the warning in future tests so they can decide then.

[Jefffrey]

I see what you mean :) , expect warns when the lint doesn't trigger, while allow stays silent. I removed those test attributes because they showed unfulfilled expectations - the lint wasn't firing there. I'd rather suppress only where it triggers, and let developers see the warning in future tests so they can decide then.

I think it was a conscious decision to use allow instead of expect for this test wide configuration as we didn't want this lint applied to test code, both for present code and future code. I feel its more correct to have it allow since we're then essentially turning off the lint, otherwise with expect it's as if we expect the test code to trigger the lint.

[alamb]

I see what you mean :) , expect warns when the lint doesn't trigger, while allow stays silent. I removed those test attributes because they showed unfulfilled expectations - the lint wasn't firing there. I'd rather suppress only where it triggers, and let developers see the warning in future tests so they can decide then.

I think it was a conscious decision to use allow instead of expect for this test wide configuration as we didn't want this lint applied to test code, both for present code and future code. I feel its more correct to have it allow since we're then essentially turning off the lint, otherwise with expect it's as if we expect the test code to trigger the lint.

So how about we change it back to allow and add a comment explaining the rationale?

[chakkk309]

I see what you mean :) , expect warns when the lint doesn't trigger, while allow stays silent. I removed those test attributes because they showed unfulfilled expectations - the lint wasn't firing there. I'd rather suppress only where it triggers, and let developers see the warning in future tests so they can decide then.

I think it was a conscious decision to use allow instead of expect for this test wide configuration as we didn't want this lint applied to test code, both for present code and future code. I feel its more correct to have it allow since we're then essentially turning off the lint, otherwise with expect it's as if we expect the test code to trigger the lint.

So how about we change it back to allow and add a comment explaining the rationale?

Thanks for the feedback!
I've changed it back to allow and added a comment explaining that we use allow instead of expect for test configuration to explicitly disable the lint for all test code rather than expecting violations.

[Jefffrey]

Took the liberty of pushing some fixes to hopefully get this PR over the line: 27835f5

[Jefffrey]

Thanks @chakkk309 & @alamb