Conversation
WalkthroughThe changes primarily involve updates to configuration files and error message handling in Rust projects. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Compiler
participant Lint
User->>Compiler: Write Rust code with attributes
Compiler->>Lint: Check for lints and errors
Lint-->>Compiler: Return warnings/errors
Compiler-->>User: Display error messages
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Veetaha
changed the title
#[builder(crate)] attribute#[builder(crate)] attribute and #[builder(expose_positional_fn)] replacement with #[builder(start_fn)]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
bon/tests/integration/ui/compile_fail/attr_bon.stderr (1)
13-19: Consider enhancing the error message with more context.While the error message is clear, it could be more educational by explicitly stating when the
#[bon]attribute is necessary.Consider updating the error message to:
-error: there are no #[builder] functions in the impl block, so there is no need for a #[bon] attribute here +error: there are no #[builder] functions in the impl block, so there is no need for a #[bon] attribute here. The #[bon] attribute is only required when the impl block contains functions marked with #[builder]bon-macros/Cargo.toml (1)
61-61: LGTM! Consider documenting the dependency's purpose.The addition of
rustversion = "1.0"is appropriate and compatible with the crate's MSRV of 1.59.0. However, consider adding a comment explaining its purpose, similar to how other dependencies are documented in this file.-rustversion = "1.0" +# Used for conditional compilation based on Rust version features +rustversion = "1.0"bon/tests/integration/ui/compile_fail/attr_builder.stderr (1)
13-13: Error message improvements look good, consider adding documentation links.The revised error message is more helpful and consistent, providing clear guidance on the correct syntax.
Consider enhancing it further by adding a link to the documentation where users can learn more about the
builderattribute parameters, for example:-error: this empty `builder` attribute is unexpected; remove it or pass parameters in parentheses: `builder(...)` +error: this empty `builder` attribute is unexpected; remove it or pass parameters in parentheses: `builder(...)`. See https://docs.rs/bon/latest/bon/attr.builder.html for available parametersbon/tests/integration/ui/compile_fail/wrong_delimiters.stderr (1)
97-102: Consider enhancing the error message with more context.While the error message is clear and consistent, consider adding a brief explanation of why doc attributes specifically require curly braces. This would help developers understand the syntax requirements better.
Example enhancement:
-error: wrong delimiter, expected curly braces e.g. `doc{...}`, but got parentheses: `doc(...)` +error: wrong delimiter, expected curly braces for doc attributes e.g. `doc{...}`, but got parentheses: `doc(...)`. Doc attributes use curly braces to maintain consistency with rustdoc syntax.Cargo.toml (1)
150-155: LGTM! Consider adding a documentation link.The addition of the
tail_expr_drop_orderlint configuration is well-documented and properly configured. The comment clearly explains the rationale for allowing this lint globally.Consider adding a link to the Rust documentation about this lint for future reference. For example:
# This lint is used only to warn about the changes in drop order during the # transition from Rust 2021 to Rust 2024. There are too many instances of this # lint in our code base and some of them may be false positives. So we just allow -# this lint globally. +# this lint globally. +# See: https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#tail-expr-drop-order tail_expr_drop_order = { level = "allow", priority = 2 }website/.vitepress/config.mts (1)
196-199: Consider more descriptive anchor names.While the distinction between top-level and member-level
finish_fnis correct, the anchor suffix "-1" is not very descriptive. Consider using more meaningful anchor names.Apply this diff to improve clarity:
{ text: "finish_fn", - link: "/reference/builder#finish-fn-1", + link: "/reference/builder#member-finish-fn", },website/reference/builder.md (2)
598-648: LGTM! Well-structured documentation forstart_fn.The documentation is comprehensive and includes clear examples for both simple and verbose forms of the attribute.
Consider adding a comma in the following line for better readability:
-The `vis` must be specified as a string literal e.g. `"pub(crate)"`, `"pub"` or `""` (empty string means private visibility). +The `vis` must be specified as a string literal e.g. `"pub(crate)"`, `"pub"`, or `""` (empty string means private visibility).🧰 Tools
🪛 LanguageTool
[uncategorized] ~611-~611: Possible missing comma found.
Context: ...Thevismust be specified as a string literal e.g."pub(crate)","pub"or""(e...(AI_HYDRA_LEO_MISSING_COMMA)
862-970: LGTM! Comprehensive documentation with suggested style improvements.The documentation thoroughly explains the
finish_fnattribute with clear examples and use cases.Consider these style improvements:
-Importantly, `Into` conversions for such members work slightly differently from the regular (named) members in regard to the `Option` type. +Importantly, `Into` conversions for such members work slightly differently from the regular (named) members regarding the `Option` type.🧰 Tools
🪛 LanguageTool
[style] ~963-~963: ‘In regard to’ might be wordy. Consider using a more concise alternative.
Context: ...rently from the regular (named) members in regard to theOptiontype. TheOptiontype gi...(REGARD_TO)
🪛 Markdownlint
868-868: null
Link fragments should be valid(MD051, link-fragments)
874-874: null
Link fragments should be valid(MD051, link-fragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (58)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lockbenchmarks/runtime/src/args_10.rsis excluded by!**/*.rsbenchmarks/runtime/src/args_10_alloc.rsis excluded by!**/*.rsbenchmarks/runtime/src/args_10_structs.rsis excluded by!**/*.rsbenchmarks/runtime/src/args_20.rsis excluded by!**/*.rsbenchmarks/runtime/src/args_3.rsis excluded by!**/*.rsbenchmarks/runtime/src/args_5.rsis excluded by!**/*.rsbon-macros/src/bon.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/builder_derives.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/builder_params/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/finish_fn.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/input_fn.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/input_struct.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/config/blanket.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/config/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/config/setter_closure.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/config/setters.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/into_conversion.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/named.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/models.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/setters/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/state_mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/top_level_config/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/top_level_config/on.rsis excluded by!**/*.rsbon-macros/src/builder/item_fn.rsis excluded by!**/*.rsbon-macros/src/builder/item_impl.rsis excluded by!**/*.rsbon-macros/src/builder/mod.rsis excluded by!**/*.rsbon-macros/src/error/mod.rsis excluded by!**/*.rsbon-macros/src/error/panic_context.rsis excluded by!**/*.rsbon-macros/src/normalization/cfg/mod.rsis excluded by!**/*.rsbon-macros/src/parsing/item_sig.rsis excluded by!**/*.rsbon-macros/src/parsing/mod.rsis excluded by!**/*.rsbon-macros/src/parsing/simple_closure.rsis excluded by!**/*.rsbon-macros/src/parsing/spanned_key.rsis excluded by!**/*.rsbon-macros/src/util/attrs.rsis excluded by!**/*.rsbon-macros/src/util/expr.rsis excluded by!**/*.rsbon-macros/src/util/ide.rsis excluded by!**/*.rsbon-macros/src/util/mod.rsis excluded by!**/*.rsbon-macros/src/util/path.rsis excluded by!**/*.rsbon/src/__private/cfg_eval.rsis excluded by!**/*.rsbon/src/__private/derives.rsis excluded by!**/*.rsbon/src/__private/ide.rsis excluded by!**/*.rsbon/src/__private/mod.rsis excluded by!**/*.rsbon/src/builder_state.rsis excluded by!**/*.rsbon/src/collections.rsis excluded by!**/*.rsbon/src/lib.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_bon.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_crate.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_expose_positional_fn.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_top_level_start_fn.rsis excluded by!**/*.rsbon/tests/integration/builder/mod.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_bon.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_crate.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_start_finish_fn.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_top_level_start_finish_fn.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/wrong_delimiters.rsis excluded by!**/*.rs
📒 Files selected for processing (12)
- Cargo.toml (1 hunks)
- bon-macros/Cargo.toml (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_bon.stderr (2 hunks)
- bon/tests/integration/ui/compile_fail/attr_builder.stderr (3 hunks)
- bon/tests/integration/ui/compile_fail/attr_crate.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_derive.stderr (22 hunks)
- bon/tests/integration/ui/compile_fail/attr_on.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_start_finish_fn.stderr (0 hunks)
- bon/tests/integration/ui/compile_fail/attr_top_level_start_finish_fn.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/wrong_delimiters.stderr (4 hunks)
- website/.vitepress/config.mts (3 hunks)
- website/reference/builder.md (10 hunks)
💤 Files with no reviewable changes (1)
- bon/tests/integration/ui/compile_fail/attr_start_finish_fn.stderr
🧰 Additional context used
🪛 LanguageTool
website/reference/builder.md
[uncategorized] ~611-~611: Possible missing comma found.
Context: ...Thevismust be specified as a string literal e.g."pub(crate)","pub"or""(e...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~963-~963: ‘In regard to’ might be wordy. Consider using a more concise alternative.
Context: ...rently from the regular (named) members in regard to theOptiontype. TheOptiontype gi...(REGARD_TO)
🪛 Markdownlint
website/reference/builder.md
868-868: null
Link fragments should be valid(MD051, link-fragments)
874-874: null
Link fragments should be valid(MD051, link-fragments)
🔇 Additional comments (19)
bon/tests/integration/ui/compile_fail/attr_bon.stderr (1)
Line range hint
1-5: LGTM! Clear and precise error message.The error message follows Rust's standard format and clearly identifies the problem with the unknown
attrsfield.bon/tests/integration/ui/compile_fail/attr_on.stderr (1)
Line range hint
1-24: Well-structured and informative error messages!The error messages effectively communicate issues with the
#[builder]attribute usage by:
- Clearly explaining redundant attributes (
into,overwritable) when they're already implied byon(...)- Providing precise locations and code snippets
- Offering actionable guidance for fixing issues
- Including a GitHub link for reporting unsupported use cases
The messages help developers quickly identify and fix problems with attribute syntax and semantics.
bon/tests/integration/ui/compile_fail/attr_builder.stderr (1)
Line range hint
1-69: Verify test coverage for all error scenarios.The error messages are well-structured and consistent. Let's ensure we have test coverage for all error scenarios.
✅ Verification successful
All error scenarios have corresponding test cases in the source file
Based on the source file content and the test cases found, I can verify that:
- Empty
#[builder]attribute cases are tested at struct, function, and member levels- Empty parentheses cases
#[builder()]are tested in all contexts- Struct-related error cases are covered with
LegacyBuilderProcMacroAttrOnStruct- Non-function items are tested with
EnumsAreUnsupported- Multiple
#[must_use]scenario is tested withdouble_must_use()- Destructuring pattern cases are tested with
destructuring1anddestructuring2Each error message in the stderr file corresponds to a specific test case in the source file, providing complete coverage for all error scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all error messages have corresponding test cases # Test: Check if each error message in the stderr file has a corresponding test case echo "Checking for test cases in the source file..." rg --no-filename "error:" bon/tests/integration/ui/compile_fail/attr_builder.stderr | while read -r error_msg; do # Extract the key part of the error message key_msg=$(echo "$error_msg" | sed 's/error: //') # Look for corresponding test case in the .rs file echo "Searching for test case for: $key_msg" rg -l "$key_msg" "bon/tests/integration/ui/compile_fail/attr_builder.rs" doneLength of output: 8642
Script:
#!/bin/bash # Let's try a different approach to verify the test coverage # First, let's get the source file content echo "=== Source file content ===" cat bon/tests/integration/ui/compile_fail/attr_builder.rs # Then, let's check for specific test cases for each error scenario echo -e "\n=== Checking specific test cases ===" # Check for empty builder attribute cases echo -e "\n1. Empty builder attributes:" rg "#\[builder\]" bon/tests/integration/ui/compile_fail/attr_builder.rs -A 1 # Check for empty parentheses cases echo -e "\n2. Empty parentheses cases:" rg "#\[builder\(\)\]" bon/tests/integration/ui/compile_fail/attr_builder.rs -A 1 # Check for struct-related cases echo -e "\n3. Struct-related cases:" rg "struct.*\{" bon/tests/integration/ui/compile_fail/attr_builder.rs -A 1 # Check for non-function items with builder attribute echo -e "\n4. Non-function items with builder:" rg "#\[builder.*\]" bon/tests/integration/ui/compile_fail/attr_builder.rs -B 1Length of output: 4047
bon/tests/integration/ui/compile_fail/attr_crate.stderr (3)
1-6: Well-structured error messages with clear guidance.The error messages effectively communicate the path resolution rules for the
crateparameter:
- For paths within the current crate: Use
crate::path::to::bon- For external crate reexports: Use
::crate_name::reexport::path::bonThe messages are consistent across different error cases and provide clear examples of the correct syntax.
Also applies to: 13-18, 19-24, 25-30, 31-36, 43-48, 49-54, 55-59
37-42: Clear distinction for impl block attribute placement.The error message effectively communicates that the
crateparameter should be specified in the#[bon]attribute when using impl block syntax, not in the method's#[builder]attribute. This helps prevent confusion about attribute placement and maintains a clear separation of concerns.
1-59: Comprehensive path resolution rules align with PR objectives.The error messages support the PR's objective of implementing
#[builder(crate = ...)]override by:
- Enforcing absolute paths to prevent ambiguity
- Supporting both internal (
crate::) and external (::-prefixed) paths- Providing clear guidance for impl block usage
Let's verify that these error messages are consistent with the actual test file:
✅ Verification successful
Error messages correctly enforce path resolution rules
The verification confirms that the error messages in the stderr file perfectly match the test cases in the source file. The implementation correctly enforces:
- Rejection of relative paths (
self::,super::) with clear guidance to usecrate::for internal paths- Rejection of bare identifiers (like
bon) with instructions to use either::bonfor external crates orcrate::for internal paths- Proper handling of
crateparameter placement in impl blocks, preventing redundant specifications- Consistent error messaging across struct, function, and impl block contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the test file contains all the cases mentioned in the error messages # Test: Find the corresponding test file and show its contents fd -t f "attr_crate.rs" tests/integration/ui/compile_fail/ --exec cat {}Length of output: 196
Script:
#!/bin/bash # Let's try searching from the repository root without specifying the directory fd -t f "attr_crate.rs" --exec cat {}Length of output: 3358
Script:
#!/bin/bash # Let's find the corresponding stderr file to verify error messages fd -t f "attr_crate.stderr" --exec cat {}Length of output: 3654
bon/tests/integration/ui/compile_fail/wrong_delimiters.stderr (3)
13-18: LGTM! Clear and consistent error message format.The error message clearly indicates:
- What went wrong (wrong delimiter)
- What was expected (parentheses)
- What was provided (curly braces)
- Example of correct usage
43-48: LGTM! Consistent error handling for square brackets.The error message maintains the same clear format while correctly handling the square brackets case.
61-83: LGTM! Clear distinction between attribute and function call requirements.The error messages properly distinguish that
docattributes require curly braces, maintaining consistency with the overall error message format.bon/tests/integration/ui/compile_fail/attr_top_level_start_finish_fn.stderr (3)
1-30: LGTM: Clear and consistent error messages for parentheses-related issues.The error messages for empty and missing parentheses are clear, consistent, and provide good guidance. They effectively communicate:
- When parameters are expected in parentheses
- When an empty attribute is unexpected
- The correct syntax to use (
start_fn(...))
31-36: Comprehensive error message for visibility parameter requirements.The error message excellently explains:
- The requirement for
start_fn(name)when usingstart_fn(vis)- The default behavior without explicit naming
- The implications for the positional function's visibility and accessibility
43-83: LGTM: Consistent error reporting across different contexts.The error messages are consistently applied across different contexts (top-level and nested attributes), maintaining the same level of detail and clarity. This helps users understand that these rules apply uniformly regardless of where the attribute is used.
website/.vitepress/config.mts (1)
159-160: LGTM! Clear and consistent terminology.The renaming from "Item attributes" to "Top-level attributes" improves clarity and better reflects the scope of these attributes.
bon/tests/integration/ui/compile_fail/attr_derive.stderr (5)
Line range hint
1-15: LGTM! Clear and helpful error messages for attribute syntax.The error messages effectively guide users on correct attribute syntax by:
- Indicating when parameters are expected in parentheses
- Specifying missing required fields
- Clarifying expected delimiter usage with examples
Line range hint
16-211: LGTM! Comprehensive error messages for Clone trait requirements.The error messages effectively communicate:
- The missing trait implementation
- The source of the requirement (
clone_memberfunction)- A clear solution with
#[derive(Clone)]
Line range hint
212-446: LGTM! Clear error messages for Debug trait requirements.The error messages effectively communicate:
- The missing trait implementation
- The source of the requirement (
as_dyn_debugfunction)- A clear solution with
#[derive(Debug)]
Line range hint
1-446: LGTM! Well-structured error messages file.The file effectively organizes compiler errors by:
- Grouping related errors together (attribute syntax, Clone trait, Debug trait)
- Maintaining consistent error message formatting
- Providing clear line number references and helpful suggestions
38-38: Verify the path change fromprivateto__private.The error messages reference
src/__private/derives.rsinstead of the previoussrc/private/derives.rs. This change might be intentional to better indicate internal modules, but we should verify this change is consistent with the PR objectives.Also applies to: 177-177, 194-194, 211-211
✅ Verification successful
Path change to
__privateis consistent and properly implementedThe verification confirms that:
- The file exists at the new path
bon/src/__private/derives.rs- No instances of the old path
private/deriveswere found- All references in test files consistently use the new
__privatepath- The change appears to be a deliberate module restructuring to better indicate internal components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the path change from private to __private # Test 1: Check if the old path still exists echo "Checking for old path..." fd --type f "derives.rs" -p "src/private" # Test 2: Verify the new path exists echo "Checking for new path..." fd --type f "derives.rs" -p "src/__private" # Test 3: Look for any remaining references to the old path echo "Checking for remaining references to old path..." rg -l "src/private/derives"Length of output: 534
Script:
#!/bin/bash # Let's try a different approach to verify the path change # Check for any files named derives.rs echo "Looking for derives.rs files..." fd "derives.rs" # Search for any references to private/derives or __private/derives echo "Searching for path references..." rg -l "private/derives|__private/derives" # Look specifically in test files for path references echo "Searching in test files..." fd -e stderr -e stdout | xargs rg "private/derives|__private/derives"Length of output: 2417
website/reference/builder.md (1)
9-10: LGTM! Improved terminology clarity.The change from "Item attributes" to "Top-level attributes" better describes the scope and application of these attributes, making the documentation more intuitive.
2 participants
This implement implements the following:
#[builder(crate = ...)]override supportexpose_positional_fn. Use#[builder(start_fn)]instead.TODO before 3.0
#[builder(on(..., transparent))]support#[builder(with)]:SomeFromIterator::from_iteror<_>::from_iterIsSetandIsUnsettratis (cleanup lib.rs)IsSettraitdocs { ... }overrides attributes added in Add docs override attributes for various generated items #139vis(...)configs and how they influence each other (e.g.builder_typevisibility influences thebuilder_modvisibility)builder_type(vis)- changes the default visibility of builder struct and all of its methods (setters,finish_fn)start_fn/finish_fn(vis)- changes only the visibility of thestart_fn/finish_fnbuilder_mod(vis)- changes only the visibility of thebuilder_mod. Doesn't influence the visibility of items defined inside it (since they are used in the builder's type signature)ReferencesectionSummary by CodeRabbit
New Features
tail_expr_drop_orderto improve code quality checks.rustversionfor compile-time Rust version checks.#[bon]and#[builder]attributes, providing clearer guidance on usage.Documentation
Bug Fixes