Skip to content

refactor(rules): migrate all Rule impls to define_rule! macro #1558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gvozdvmozgu
Copy link
Collaborator

No description provided.

@gvozdvmozgu
Copy link
Collaborator Author

@benfdking it seems much simpler, and in the future it will be easier if we want to remove the Rule trait.

Copy link

Benchmark for 904116d

Click to view benchmark
Test Base PR %
DepthMap::from_parent 39.9±0.35µs 40.4±0.40µs +1.25%
fix_complex_query 9.3±0.18ms 9.8±0.67ms +5.38%
fix_superlong 102.5±5.61ms 108.2±7.92ms +5.56%
parse_complex_query 2.9±0.03µs 2.8±0.05µs -3.45%
parse_expression_recursion 5.1±0.11µs 5.1±0.04µs 0.00%
parse_simple_query 802.7±9.88ns 799.6±9.03ns -0.39%

@benfdking
Copy link
Collaborator

Hey 👋

This to me feels somewhat simpler and more complex in the same breath: Cleaner as the rule code itself looker simpler, less approachable because of that macro.

I honestly don't know which is better, so happy to go with your recommendation.

As I see it at the moment, there's a neat win in having rule descriptions also apply in Rust docs. Curious to understand though why you think we can drop the trait/why we would want that.

@gvozdvmozgu
Copy link
Collaborator Author

By removing the Rule trait, we avoid cloning the rule when generating linter warnings (i.e., cloning the entire struct, not just a pointer, which can be costly if the rule carries configuration) and can finally eliminate the dyn_clone dependency.

@gvozdvmozgu
Copy link
Collaborator Author

However, this will probably also require modifying the configurator, as suggested in an issue, to consolidate everything into a single large Config struct.

@benfdking
Copy link
Collaborator

benfdking commented Apr 29, 2025

By removing the Rule trait, we avoid cloning the rule when generating linter warnings (i.e., cloning the entire struct, not just a pointer, which can be costly if the rule carries configuration) and can finally eliminate the dyn_clone dependency.

Happy to give it a go for the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants