Skip to content

Conversation

@kumarUjjawal
Copy link
Contributor

@kumarUjjawal kumarUjjawal commented Dec 13, 2025

Which issue does this PR close?

Rationale for this change

We want to use #[expect] in place of #[allow] on the workspace level but to get there we wanted to do this in few crates at a time before enabling it in workspace.

What changes are included in this PR?

Attributes changes for the following crates:

  • datafusion-macros
  • datafusion-optimizer

Are these changes tested?

Yes

Are there any user-facing changes?

@github-actions github-actions bot added optimizer Optimizer rules core Core DataFusion crate ffi Changes to the ffi crate labels Dec 13, 2025
child_filter.input,
)?);
#[allow(clippy::used_underscore_binding)]
#[expect(clippy::used_underscore_binding)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix the code here to not need this expect?


// recursively reapply the rule on the new plan
#[allow(clippy::used_underscore_binding)]
#[expect(clippy::used_underscore_binding)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

#[derive(Debug, StableAbi)]
#[allow(non_camel_case_types)]
#[expect(clippy::large_enum_variant)]
#[allow(clippy::large_enum_variant)]
Copy link
Contributor

Choose a reason for hiding this comment

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

For FFI I think we're missing the deny at the lib.rs level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I add #![deny(clippy::allow_attributes)] in ffi, I get several unfulfilled expectations, so we’d need to also enable the relevant lints (#![warn(non_camel_case_types, clippy::type_complexity, clippy::disallowed_methods, clippy::large_enum_variant)]) so the #[expect]s actually fire. Is there any alternative you have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

So from what I've seen, the main problem is that expect doesn't play well with non_camel_case_types? I haven't had time to look into it properly, but we'll definitely want to figure something out for this instead of omitting the #![deny(clippy::allow_attributes)] in ffi entirely. Maybe for this PR we can exclude ffi until we figure out a proper solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will revert the changes for the FFI from this PR and see if I can figure something out for it and open a seperate PR.

@Jefffrey
Copy link
Contributor

fyi @chakkk309 since I think you were planning to work on the FFI part too?

@chakkk309
Copy link
Contributor

fyi @chakkk309 since I think you were planning to work on the FFI part too?

Thanks for the heads up! I'm working on the FFI part, but to avoid more duplicate work, I won't submit the pr :)

@kumarUjjawal
Copy link
Contributor Author

fyi @chakkk309 since I think you were planning to work on the FFI part too?

Thanks for the heads up! I'm working on the FFI part, but to avoid more duplicate work, I won't submit the pr :)

If you have made progress on the FFI crate I can revert my changes to FFI and you can open the PR for that.

@github-actions github-actions bot removed the ffi Changes to the ffi crate label Dec 16, 2025
@kumarUjjawal kumarUjjawal changed the title chore: enforce clippy::allow_attributes for ffi, optimizer and macros chore: enforce clippy::allow_attributes for optimizer and macros Dec 16, 2025
@Jefffrey Jefffrey added this pull request to the merge queue Dec 16, 2025
Merged via the queue into apache:main with commit 02c647a Dec 16, 2025
33 checks passed
@Jefffrey
Copy link
Contributor

Thanks @kumarUjjawal

@chakkk309
Copy link
Contributor

chakkk309 commented Dec 16, 2025

fyi @chakkk309 since I think you were planning to work on the FFI part too?

Thanks for the heads up! I'm working on the FFI part, but to avoid more duplicate work, I won't submit the pr :)

If you have made progress on the FFI crate I can revert my changes to FFI and you can open the PR for that.

No worries, please feel free to take the FFI part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants