Flexible builder extension and type signature API #145
Conversation
Veetaha
force-pushed
the
feat/generic-builder-type
branch
from
99ff6a9
to
60bf268
Compare
|
Quick feedback on a simple example : impl<State: endpoint_builder::State> EndpointBuilder<State> {
pub fn smart_crop(self) -> EndpointBuilder<SetSmart<State>>
where
State::Smart: bon::IsUnset,
{
self.smart(true)
}
}The API is very straightforward and convenient to work with, nice work 🎉 |
|
Thank you for the feedback ❤️, this should be landed in the next couple of weeks |
![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 (64)
bon/tests/integration/ui/compile_fail/attr_into.stderr (1)
1-5: LGTM! Error message is correctly formatted and informative.The error message accurately points out the misuse of the
#[builder(into = false)]attribute on au32parameter. This is likely an intentional test case in thecompile_faildirectory to ensure that incorrect usage of the attribute is caught during compilation.Consider enhancing the error message to provide more context about why
boolis unexpected here. For example:- error: Unexpected type `bool` + error: Unexpected type `bool` for `into` attribute. `into = false` is not a valid option.This would make it clearer to users that
into = falseis not a valid use of the attribute, rather than implying that a boolean value might be expected in some cases.benchmarks/compilation/codegen/Cargo.toml (1)
13-16: Dependencies are appropriate, but consider using more specific version constraints.The chosen dependencies (anyhow, proc-macro2, and quote) are well-suited for code generation tasks. However, using "1.0" as the version constraint might lead to unexpected breaking changes in the future.
Consider using more specific version constraints to ensure reproducible builds:
[dependencies] -anyhow = "1.0" -proc-macro2 = "1.0" -quote = "1.0" +anyhow = "1.0.70" +proc-macro2 = "1.0.56" +quote = "1.0.26"You can use
cargo updateto get the latest compatible versions and then specify them explicitly in the Cargo.toml file.bon/tests/integration/ui/compile_fail/attr_bon.stderr (2)
1-6: LGTM with a minor suggestion for improvement.The error message is clear and informative. It correctly indicates that the
#[bon]attribute doesn't currently accept parameters and provides the exact location of the issue. The hint about future functionality is a nice touch for setting user expectations.Consider adding a suggestion for the correct usage in the current version. For example:
error: `#[bon]` attribute does not accept any parameters yet, but it will in future releases --> tests/integration/ui/compile_fail/attr_bon.rs:5:7 | 5 | #[bon(attrs)] | ^^^^^ + | + = help: use `#[bon]` without parameters for nowThis addition would provide immediate guidance to users on how to correct their code.
7-11: LGTM with a suggestion for enhanced clarity.The error message is concise and accurately points out the issue with using
#[builder]on the receiver. It provides the exact location and context of the problem.To enhance clarity, consider expanding the error message slightly to explain why this isn't supported or provide a hint on the correct usage. For example:
error: #[builder] attributes on the receiver are not supported --> tests/integration/ui/compile_fail/attr_bon.rs:16:12 | 16 | fn sut(#[builder] &self) {} | ^ + | + = note: #[builder] can only be applied to struct fields or the entire function, not to individual parametersThis addition would provide more context and guide users towards the correct usage of the
#[builder]attribute.scripts/install/hyperfine.sh (4)
1-5: Good setup, consider adding a brief script description.The script setup looks good. It uses appropriate error handling and sources a shared library. However, it would be helpful to add a brief comment describing the purpose of this script.
Consider adding a comment like this at the beginning of the script:
#!/usr/bin/env bash # This script installs the Hyperfine benchmarking tool set -euo pipefail
12-15: Download and decompress logic looks good, consider adding a comment.The download and decompress logic is well-structured and uses appropriate options.
To improve clarity, consider adding a brief comment explaining what this section does:
# Download and extract the Hyperfine binary download_and_decompress \ "$base_url/$file_stem.tar.gz" \ --strip-components 1 \ "$file_stem/hyperfine"
17-17: Final installation step looks good, consider adding error handling.The use of
move_exe_to_pathis appropriate for finalizing the installation.To improve robustness, consider adding error handling:
if ! move_exe_to_path hyperfine; then echo "Failed to move Hyperfine to PATH" >&2 exit 1 fi echo "Hyperfine installed successfully"This change will provide better feedback if the installation fails and confirm success when it completes.
1-17: Overall, the script is well-structured but could benefit from minor improvements.The script effectively installs the Hyperfine benchmarking tool with good practices in place. To further enhance it:
- Add a brief description at the beginning of the script.
- Verify the
arch_rustvariable's definition.- Add comments to explain key sections.
- Improve error handling and success reporting.
Would you like assistance in implementing these improvements or creating a more comprehensive error handling strategy for the script?
🧰 Tools
🪛 Shellcheck
[warning] 10-10: arch_rust is referenced but not assigned.
(SC2154)
benchmarks/compilation/Cargo.toml (1)
14-18: Dependencies are well-structured. Consider version constraints for local crates.The dependencies are appropriately defined for a benchmarking crate. Good job on making alternative builders optional.
Consider adding a version constraint for the local
boncrate:-bon = { path = "../../bon", optional = true } +bon = { path = "../../bon", version = "=0.1.0", optional = true }This ensures that the benchmarks are always run against the correct version of
bon, even if the workspace contains multiple versions.benchmarks/compilation/run.sh (3)
5-15: LGTM: Well-structured array definitions.The
macrosandsuitesarrays are correctly defined and their contents align with the benchmarking objectives. The naming is clear and descriptive.Consider adding comments above each array to briefly explain their purpose and the significance of their entries. This would enhance maintainability, especially as the script grows or is modified by different contributors.
17-24: LGTM: Well-configured hyperfine command for comprehensive benchmarking.The
hyperfinecommand is well-structured, using appropriate options for setup, preparation, and result export. The use of parameter lists for macros and suites allows for easy expansion of test cases.Consider adding a post-processing step to analyze the benchmark results. For example, you could use a tool like
jqto parse the JSON output (which can be enabled with--export-json results.json) and generate a summary or comparison of the different configurations. This could help quickly identify performance differences between the various macros and struct configurations.Example addition:
hyperfine ... --export-json results.json jq -r '.results | sort_by(.mean) | .[] | "\(.command): \(.mean) ±\(.stddev) seconds"' results.json > summary.txt
25-25: LGTM: Correct benchmark command with proper feature specification.The final command correctly builds the
compilation-benchmarkspackage with the specified features using the{suite}and{macro}placeholders.Consider adding error handling for cases where the build might fail. For example, you could wrap the command in a function that checks the exit status and logs any errors. This would provide more context if a particular configuration fails to build.
Example:
build_and_check() { if ! cargo build -p compilation-benchmarks --features="$1,$2"; then echo "Error: Build failed for suite $1 and macro $2" >&2 return 1 fi } # Then use it in the hyperfine command hyperfine ... 'build_and_check {suite} {macro}'bon/tests/integration/ui/compile_fail/derive_builder.stderr (1)
13-19: LGTM with a minor suggestion: Informative error message with a slight inconsistencyThe error message provides clear and specific information about the
Builderderive macro's limitations. The additional note about using-Z macro-backtracefor more info in Nightly builds is helpful for advanced debugging.However, there's a minor inconsistency in the error message:
The error mentions
bon::Builder, but the code showsBuilder. Consider updating the error message to match the actual code or ensure that the code is using the fully qualified pathbon::Builderif that's the intended usage.benchmarks/runtime/run.sh (5)
1-7: Good setup, consider improving argument handling.The script setup is well-structured with appropriate error handling and environment configuration. The use of
set -euxo pipefailis a good practice for shell scripts.Consider using
${1:-args_10_structs}instead of${1:-args_10_structs}for consistency with modern shell scripting practices.
9-11: Build step looks good, consider parameterizing the package name.The clean and build steps are appropriate for ensuring consistent benchmark results.
Consider parameterizing the package name (
runtime-benchmarks) to make the script more reusable:package_name="runtime-benchmarks" cargo build --features "$bench" --release -p "$package_name"
13-19: Assembly generation and diff view are useful, consider improving error handling.The generation of assembly listings and the optional diff view in VS Code are valuable for performance analysis.
Instead of using
|| true, consider capturing and logging any errors:cargo asm --features "$bench" --no-color "runtime_benchmarks::$bench::builder_bench" > builder.dbg.s 2>builder_asm_error.log || echo "Failed to generate builder assembly. Check builder_asm_error.log for details." cargo asm --features "$bench" --no-color "runtime_benchmarks::$bench::regular_bench" > regular.dbg.s 2>regular_asm_error.log || echo "Failed to generate regular assembly. Check regular_asm_error.log for details."
21-22: Benchmarking commands look good, consider capturing results.The use of both
iaiandcriterionbenchmarks provides a comprehensive performance analysis.Consider capturing the benchmark results in files for easier analysis and comparison:
cargo bench --features "$bench" -p runtime-benchmarks --profile release --bench iai > iai_results.txt cargo bench --features "$bench" -p runtime-benchmarks --profile release --bench criterion > criterion_results.txt
1-22: Well-structured benchmark script, consider adding documentation.Overall, this is a well-crafted script that covers all necessary steps for running benchmarks in a Rust project. It follows good shell scripting practices and provides flexibility.
Consider adding a brief comment at the beginning of the script explaining its purpose and usage. For example:
#!/usr/bin/env bash # This script runs benchmarks for the runtime-benchmarks package. # Usage: ./run.sh [benchmark_name] # If no benchmark name is provided, it defaults to 'args_10_structs'. set -euxo pipefail ...This documentation will help other developers understand and use the script more easily.
benchmarks/compilation/results.md (2)
1-12: Insightful benchmark results with notable performance differences.The benchmark results provide valuable insights:
derive_builderconsistently outperforms other methods in both struct configurations.- There's a significant performance gap between
derive_builderand other methods, especially in thestructs_10_fields_50configuration.bon,bon-overwritable, andtyped-buildershow similar performance characteristics.Consider the following actions:
- Analyze the reasons behind
derive_builder's superior performance.- Evaluate if optimizations can be applied to
bonandbon-overwritablebased onderive_builder's approach.- Discuss the trade-offs between performance and features offered by each method.
Would you like assistance in creating a more detailed analysis of these results or in formulating optimization strategies?
🧰 Tools
🪛 Markdownlint
7-7: null
Spaces inside code span elements(MD038, no-space-in-code)
12-12: null
Spaces inside code span elements(MD038, no-space-in-code)
7-7: Consider removing empty code spans for cleaner markdown.The static analysis tool flagged empty code spans (``) at the end of lines 7 and 12. While these don't affect the rendered output, they're unnecessary and can be removed for cleaner markdown.
Consider applying this change:
-| `structs_100_fields_10 ` | 0.104 ± 0.012 | 0.088 | 0.124 | 1.02 ± 0.18 | +| `structs_100_fields_10` | 0.104 ± 0.012 | 0.088 | 0.124 | 1.02 ± 0.18 | -| `structs_10_fields_50 ` | 0.102 ± 0.013 | 0.084 | 0.130 | 1.00 | +| `structs_10_fields_50` | 0.102 ± 0.013 | 0.084 | 0.130 | 1.00 |This minor change will resolve the Markdownlint warnings without affecting the table's appearance or functionality.
Also applies to: 12-12
🧰 Tools
🪛 Markdownlint
7-7: null
Spaces inside code span elements(MD038, no-space-in-code)
website/guide/patterns/fallible-builders.md (3)
5-5: Approved: Correct syntax update with a suggestion for clarity.The change from
#[builder]to#[derive(Builder)]accurately reflects the proper syntax for deriving builders in Rust. This update improves the documentation's accuracy.Consider adding a brief explanation about why
#[derive(Builder)]can't be used in this context. For example:- If you need to build a struct and do some validation, you won't be able to use the `#[derive(Builder)]` annotation on the struct for that. You'll *have to* define your logic in the associated constructor method (e.g. `new`). + If you need to build a struct and do some validation, you won't be able to use the `#[derive(Builder)]` annotation on the struct for that, as it doesn't support custom validation logic. Instead, you'll *have to* define your logic in the associated constructor method (e.g. `new`).
41-41: Approved: Correct syntax update with a suggestion for consistency.The change from
#[builder]to#[derive(Builder)]correctly updates the syntax for deriving builders, maintaining consistency with the earlier change.For consistency with the code example earlier in the document, consider updating the line to explicitly mention the
boncrate:- If you have a use case for some convenience attributes to do automatic validations using the `#[derive(Builder)]` macro with the struct syntax, then add a 👍 reaction to [this Github issue](https://github.com/elastio/bon/issues/34). + If you have a use case for some convenience attributes to do automatic validations using the `#[derive(Builder)]` macro from the `bon` crate with the struct syntax, then add a 👍 reaction to [this Github issue](https://github.com/elastio/bon/issues/34).
Line range hint
1-41: Overall: Documentation updates are accurate and valuable.The changes in this file correctly update the syntax for deriving builders from
#[builder]to#[derive(Builder)], improving the accuracy of the documentation. The document provides clear explanations and examples for fallible builders in theboncrate.Consider adding a brief section or note about the performance implications of using fallible builders, if any, to give users a complete picture of when to use this pattern.
scripts/test-msrv.sh (1)
Line range hint
1-57: Review summary: Optimization of MSRV testing processThe changes to this script focus on removing the
--all-featuresflag from both the Clippy and test commands. While this optimization may significantly reduce CI build times, it also introduces a potential risk of missing issues or test failures that only occur with certain feature combinations.To maintain a balance between CI performance and thorough testing:
- Ensure that your CI pipeline includes jobs that test various feature combinations, even if not in this MSRV script.
- Regularly run comprehensive tests with all features locally or in scheduled CI jobs.
- Consider implementing a matrix strategy in your CI that tests different feature combinations across different Rust versions.
- Document this change clearly, explaining the rationale and any compensating controls put in place to ensure comprehensive testing.
These steps will help maintain the integrity of your testing process while benefiting from the optimized MSRV tests.
bon-macros/Cargo.toml (3)
60-60: Consider using a more specific version for prettypleaseThe addition of the
prettypleasedependency is good for code formatting capabilities. However, to ensure better reproducibility and avoid potential issues with future updates, consider specifying a more precise version constraint.You might want to update the line to:
prettyplease = "0.2.15"This will pin to the latest patch version within the 0.2.x series, balancing stability with bug fixes.
62-66: Good addition of features sectionThe introduction of the
[features]section with a default empty feature and theimplied-boundsfeature is a positive change. It allows for more flexible use of the crate and follows good practices.Consider slightly modifying the comment to make it more specific:
# See the docs on the `implied-bounds` feature in the `bon` crate's `Cargo.toml` implied-bounds = []This change makes it clearer which feature the comment is referring to.
68-69: Good addition of expect-test for developmentThe introduction of
expect-testas a development dependency is a positive change. It suggests the implementation of snapshot testing, which can significantly improve the reliability and maintainability of your macro tests.Consider using a caret version constraint to allow for compatible updates:
expect-test = "^1.4.1"This will allow for minor version updates that include bug fixes and non-breaking changes, while still maintaining compatibility with your current setup.
bon/tests/integration/ui/compile_fail/attr_on.stderr (2)
37-43: LGTM: Clear error message with helpful origin noteThis error message correctly identifies the missing comma in the
#[builder(on(_))]attribute. The additional note about the error originating from the attribute macro is helpful for debugging, especially for users familiar with Rust's macro system.Consider adding a suggestion for the correct syntax, similar to previous error messages. For example:
error: expected `,` --> tests/integration/ui/compile_fail/attr_on.rs:21:1 | 21 | #[builder(on(_))] | ^^^^^^^^^^^^^^^^^ | = note: this error originates in the attribute macro `builder` (in Nightly builds, run with -Z macro-backtrace for more info) = help: use `#[builder(on(_,))]` to specify an empty list of options after the type patternThis addition would make the error message more actionable for users.
45-51: LGTM: Informative error message for ineffective attributeThis error message effectively communicates that the
#[builder(on(type_pattern, ...))]attribute is ineffective when it contains no options to override the default behavior. This helps users understand when an attribute is unnecessary and encourages more intentional use of builder attributes.Consider adding a brief explanation or example of valid options that could be used in this context. For example:
error: this #[builder(on(type_pattern, ...))] contains no options to override the default behavior for the selected setters like `into`, `overwritable`, so it does nothing --> tests/integration/ui/compile_fail/attr_on.rs:24:1 | 24 | #[builder(on(_,))] | ^^^^^^^^^^^^^^^^^^ | = note: this error originates in the attribute macro `builder` (in Nightly builds, run with -Z macro-backtrace for more info) = help: add options like `into` or `overwritable` to modify the behavior, e.g., #[builder(on(_, into, overwritable))]This addition would provide users with a clear example of how to correctly use the attribute.
bon/tests/integration/ui/compile_fail/broken_intra_doc_links.stderr (1)
1-53: Summary: Comprehensive error checking enhances builder pattern implementationThe error messages in this file collectively demonstrate a thorough approach to preventing potential confusion in documentation related to the builder pattern. By catching the misuse of
Selfin various contexts (impl blocks, builder structs, and state modules), these compile-fail tests contribute significantly to the PR's objectives of improving the flexibility and usability of builder types.These error messages will help users of the
boncrate to write clearer, more maintainable code by encouraging explicit type names in documentation. This aligns well with the PR's goal of enhancing type safety and usability in the builder pattern implementation.Consider documenting these error messages in the user guide or API documentation to help users understand why
Selfshould be avoided in certain documentation contexts when using the builder pattern.bon/tests/integration/ui/compile_fail/attr_builder.stderr (1)
55-60: LGTM: Informative error message for struct builder usageThe error message correctly guides users to use
#[derive(bon::Builder)]for structs instead of#[bon::builder]. It also provides helpful information about the support for#[bon::builder]on functions in bon v3.Consider adding a brief explanation of why
#[bon::builder]is not supported for structs to provide more context to users.bon/tests/integration/ui/compile_fail/wrong_delimiters.stderr (5)
1-59: Approve error messages for incorrect function call delimiters.The error messages for incorrect usage of delimiters in function calls (start_fn, builder_type, state_mod, finish_fn) are clear and informative. They correctly identify the wrong delimiter usage and suggest the correct syntax.
Consider adding a note in the crate's documentation about these common syntax errors to help users avoid them.
25-30: Approve error messages for incorrect attribute delimiters.The error messages for incorrect usage of delimiters in the
settersattribute are clear and helpful. They correctly identify the wrong delimiter usage and suggest the correct syntax.Consider adding a specific example of the correct
settersattribute usage in the error message to further assist users, e.g., "expected parentheses e.g.setters(...), but got ...".Also applies to: 55-60
61-84: Approve helpful error messages for unknown fields.The error messages correctly identify the use of an unknown field "docs" and helpfully suggest the correct field name "doc". This is excellent for guiding users to the correct syntax.
Consider adding a note in the crate's documentation about the correct usage of the "doc" field in various builder functions to preemptively address this common mistake.
85-107: Approve error messages for incorrect doc attribute delimiters.The error messages correctly identify the wrong delimiter usage for the doc attribute, specifying that curly braces should be used instead of parentheses.
To maintain consistency with earlier error messages, consider modifying these to include the full context, e.g., "wrong delimiter, expected curly braces e.g.
start_fn(doc{...}), but got parentheses:start_fn(doc(...))".
1-107: Overall approval of comprehensive error reporting.The error messages in this file collectively demonstrate a robust error reporting system for the
boncrate's builder pattern. They cover various scenarios of incorrect syntax, providing clear and helpful guidance to users. These negative test cases are crucial for ensuring the reliability and user-friendliness of the crate.Consider creating a dedicated documentation section or guide that covers these common syntax errors and their corrections. This proactive approach could significantly improve the user experience and reduce the likelihood of users encountering these errors in their code.
.github/workflows/ci.yml (5)
27-27: Approve renaming and directory change for runtime benchmarks.The renaming from
benchmarkstoruntime-benchmarksprovides better clarity. The change in working directory to./benchmarks/runtimealigns with the project restructuring.Consider adding a comment explaining the purpose of these runtime benchmarks for better documentation.
Also applies to: 44-44
46-53: Approve addition of compilation benchmarks job.The new
compilation-benchmarksjob is a valuable addition for measuring and monitoring compile times, which aligns with the PR objectives of addressing performance concerns.Consider adding a step to cache the hyperfine installation to speed up subsequent workflow runs:
- uses: actions/cache@v2 with: path: ~/.cargo/bin/hyperfine key: ${{ runner.os }}-hyperfine
87-92: Approve additions to test-stable job.The inclusion of the
rust-srccomponent ensures consistent error rendering between CI and local environments, which is excellent for debugging. The new test commands for theimplied-boundsfeature align with the PR objectives of enhancing the builder pattern.Consider adding a comment explaining the significance of the
implied-boundsfeature and why it's being tested separately.Also applies to: 100-101
139-144: Approve changes to test-unstable and addition of cargo-miri job.The new clippy flags in
test-unstableshow forward-thinking by accommodating the 2024 edition of Rust. The addition of thecargo-mirijob significantly enhances the test suite by checking for undefined behavior.For the
cargo-mirijob, consider parameterizing the nightly toolchain version to make it easier to update in the future:env: MIRI_NIGHTLY: nightly-2024-10-14 steps: # ... - uses: actions-rust-lang/setup-rust-toolchain@v1 with: toolchain: ${{ env.MIRI_NIGHTLY }} components: miriThis way, you can update the version in one place when needed.
Also applies to: 145-157
Line range hint
1-224: Overall approval: Significant improvements to CI/CD workflow.The changes to this workflow file significantly enhance the CI/CD process, particularly in terms of benchmarking and testing. The additions and modifications align well with the PR objectives of improving the builder pattern and addressing performance concerns. The new benchmarking jobs, enhanced testing, and the addition of Miri checks all contribute to a more robust and performant codebase.
Consider creating a separate workflow file for benchmarks to keep the main CI workflow more focused and easier to maintain. This could also allow for more flexible scheduling of benchmark runs.
website/blog/bon-builder-generator-v2-release.md (6)
Line range hint
30-52: LGTM! Consider adding a comment for clarity.The code example effectively demonstrates the new feature of
skipanddefaultexpressions having access to other members in their scope. It's well-structured and includes assertions to validate the expected behavior.Consider adding a brief comment before the
assert_eq!statements to explain the expected values, enhancing readability for users new to this feature. For example:// Verify that skip expressions correctly reference other members assert_eq!(example.member_1, 3); assert_eq!(example.member_2, 6); assert_eq!(example.member_3, 9);
Line range hint
103-119: LGTM! Consider adding a comment aboutOption<String>.The code example effectively demonstrates the migration path for enabling
Intoconversions inbonv2. The use of the new#[builder(on(String, into))]attribute is clearly shown and explained.For completeness, consider adding a brief comment explaining that the
Intoconversion forOption<String>is automatically handled whenStringconversions are enabled. This could help prevent confusion for users who might wonder about nested types. For example:#[builder(on(String, into))] // Enables `Into` for String and automatically for Option<String> struct Example { arg1: String, arg2: Option<String>, arg3: u32, }
Line range hint
131-143: LGTM! Consider adding a comment about performance implications.The code example effectively demonstrates the alternative approach of using
#[builder(into)]on individual fields to enableIntoconversions. This provides users with flexibility in applying the conversions.To provide more context for users choosing between struct-level and field-level
intoattributes, consider adding a brief comment about potential performance implications. For example:// Note: Using field-level `into` attributes may have different performance // characteristics compared to struct-level attributes in some cases. #[builder] struct Example { #[builder(into)] arg1: String, // ... rest of the struct }This comment could prompt users to consider performance when deciding between the two approaches, especially for larger structs or performance-critical code.
Line range hint
203-216: LGTM! Consider adding a comment about the rationale.The code examples effectively demonstrate the removal of support for
skipin function arguments and show the new, simpler approach using local variables. This change promotes clearer and more idiomatic Rust code.To further clarify the rationale behind this change, consider adding a brief comment explaining the benefits of the new approach. For example:
// New approach: Using local variables instead of `skip` attribute // Benefits: Simpler syntax, better readability, and more idiomatic Rust #[builder] fn example() { let value = 42; println!("Arg: {value}") }This addition would help users understand why the change was made and appreciate the benefits of the new approach.
Line range hint
234-249: LGTM! Consider adding a comment about compatibility with other features.The code examples effectively demonstrate the removal of support for destructuring patterns in function arguments and show the new, simpler approach of destructuring inside the function body. This change promotes clearer code and better consistency with other features.
To provide a more comprehensive explanation, consider adding a brief comment about how this change improves compatibility with other features, such as the newly introduced ability for
skipanddefaultexpressions to access other members. For example:// New approach: Destructuring inside the function body // Benefits: Simpler syntax, better compatibility with other features like // `skip` and `default` expressions accessing other members #[builder] fn example(point: (u32, u32)) { let (x, y) = point; // Now `x` and `y` are accessible to `skip` and `default` expressions }This addition would help users understand the broader implications and benefits of this change in the context of the entire
bonlibrary.
Line range hint
1-265: Excellent blog post! Consider adding a call-to-action for user feedback.The blog post effectively communicates the new features and breaking changes in
bonv2. The content is well-structured, informative, and provides clear examples to guide users through the migration process. The positive tone and encouragement for community engagement are commendable.To further enhance user engagement, consider adding a call-to-action at the end of the post, encouraging users to share their experiences with the new version. For example:
## We'd love to hear from you! Have you tried `bon` v2? We'd love to hear about your experience! Share your thoughts, questions, or any cool projects you've built using `bon` in the comments section below or on our GitHub discussions page. Your feedback is invaluable in helping us improve and evolve `bon` further.This addition could foster more community interaction and provide valuable feedback for future improvements.
bon/tests/integration/ui/compile_fail/attr_with.stderr (3)
7-12: Consider adding an explanation for thewithandintoincompatibility.While the error message clearly states that
withandintoattributes can't be used together, it would be helpful to briefly explain why they are mutually exclusive. This additional context could help users understand the design decisions behind the#[builder]attribute and choose the appropriate option for their use case.
43-93: LGTM: Comprehensive error message for return type annotations with a suggestion.The error message provides detailed explanations for the expected return type annotations, clearly outlining the two valid options. The information about using
Resultfor fallible setters is particularly valuable.To further improve this message:
Consider adding a brief example for each option to illustrate the correct syntax. This could help users quickly understand and implement the proper return type annotation.
168-179: LGTM: Clear error message for type mismatch in closure return with a suggestion for improvement.This error message effectively communicates the type mismatch between the expected
u32and the providedimpl Into<::core::net::IpAddr>in the closure return. The additional context about the source of the expected type is helpful.To further improve this message:
Consider adding a suggestion on how to resolve the mismatch, such as converting the
IpAddrto au32representation or changing the field type to match the closure return type. This would provide more actionable guidance to users encountering this error.website/blog/bon-builder-v2-1-release.md (2)
Line range hint
108-114: LGTM! Consider adding a comment for clarity.The code snippet effectively demonstrates the new
#[must_use]attribute behavior. Thecompile_failattribute is correctly used to indicate that this code should produce a compiler warning.To enhance clarity, consider adding a brief comment above the code snippet explaining that this example is intended to demonstrate a compiler warning. For example:
// This example demonstrates the compiler warning for unused `build()` result
Line range hint
1-108: Excellent blog post! Consider minor enhancements for completeness.The blog post effectively communicates the improvements in
bon2.1, providing clear explanations and relevant code examples for each new feature. The technical details, especially in the "How did we improve compile times" section, offer valuable insights into the optimization process.Consider the following minor enhancements to further improve the post:
- Add a brief introduction summarizing the key improvements at the beginning of the post.
- Include a section on how users can upgrade to the new version and any potential breaking changes they should be aware of.
- Consider adding a "Future Work" or "Roadmap" section to give readers an idea of what's coming next for
bon.These additions would provide a more comprehensive overview of the release and its impact on users.
Also applies to: 115-265
website/changelog.md (6)
13-16: LGTM! Consider updating documentation for attribute syntax changes.The changes to attribute syntax and behavior look good. They enforce stricter rules which can help prevent errors and improve consistency.
Consider updating the user documentation to clearly highlight these syntax changes, especially the rejection of square brackets and curly braces delimiters for certain attributes. This will help users adapt to the new syntax more easily.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: A comma might be missing here.
Context: ...attributes syntax. Only parentheses are accepted e.g.#[builder(finish_fn(...))]or `#...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
19-19: LGTM! Consider providing a migration guide.The removal of the
#[bon::builder]proc-macro attribute on structs is in line with the deprecation notice from version 2.2.0. This change improves consistency by standardizing on#[derive(bon::Builder)].To ease the transition for users, consider providing a detailed migration guide in the documentation. This guide could include examples of how to update existing code to use the new
#[derive(bon::Builder)]syntax.
23-36: LGTM! Excellent additions. Ensure comprehensive documentation.The new features and attributes provide great flexibility and customization options for users. The improvements to rustdoc output will significantly enhance the developer experience.
To help users take full advantage of these new features:
- Consider creating a dedicated section in the documentation for each new attribute, with examples of their usage.
- For complex features like
#[builder(with = closure)], provide detailed examples showing how to use them effectively.- Update any existing tutorials or guides to incorporate these new features where appropriate.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~33-~33: A comma might be missing here.
Context: ...optional. - For members with defaults values show the default value in the docs. -...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~34-~34: A comma might be missing here.
Context: ...ult value in the docs. - For optional members provide a link to a companion setter. T...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~36-~36: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...nd should be reported via a Github issue but this provides an easy temporary workaro...(COMMA_COMPOUND_SENTENCE_2)
45-45: LGTM! Great addition for robustness. Consider adding a test case.The addition of graceful internal panic handling is an excellent improvement. It will enhance the robustness of the library and improve the developer experience, especially in IDEs by providing fallback intellisense.
Consider adding a test case that simulates an internal panic to ensure this fallback mechanism works as expected across different scenarios.
Line range hint
1-219: LGTM! Well-structured and informative changelog.The changelog is well-organized, following the Keep a Changelog format. It provides clear and detailed information about changes in each version, which is extremely valuable for users upgrading between versions.
To further enhance the changelog:
- Consider adding links to relevant documentation or examples for significant new features.
- For breaking changes, you might want to add a brief note on how users can migrate their code, or link to migration guides if available.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: A comma might be missing here.
Context: ...attributes syntax. Only parentheses are accepted e.g.#[builder(finish_fn(...))]or `#...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[duplication] ~18-~18: Possible typo: you repeated a word
Context: ...rive(Clone/Debug(bounds(...))))]. ### Removed - Removed support for#[bon::builder]` proc-macr...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~33-~33: A comma might be missing here.
Context: ...optional. - For members with defaults values show the default value in the docs. -...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~34-~34: A comma might be missing here.
Context: ...ult value in the docs. - For optional members provide a link to a companion setter. T...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~36-~36: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...nd should be reported via a Github issue but this provides an easy temporary workaro...(COMMA_COMPOUND_SENTENCE_2)
15-15: Consider addressing minor grammatical issues.There are a few minor grammatical issues that could be addressed to improve readability:
- Line 15: Consider adding a comma before "e.g." for clarity.
- Line 33: Consider adding a comma after "values" for better sentence structure.
- Line 34: Consider adding a comma after "members" for consistency.
- Line 36: Consider adding a comma before "but" to separate independent clauses.
These are minor suggestions and can be implemented at your discretion, keeping in mind the overall style and consistency of the changelog.
Also applies to: 33-33, 34-34, 36-36
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: A comma might be missing here.
Context: ...attributes syntax. Only parentheses are accepted e.g.#[builder(finish_fn(...))]or `#...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
website/reference/builder.md (6)
12-50: Great addition of comprehensive code examples!The inclusion of examples for struct, free function, and associated method syntaxes provides excellent clarity on how to use item and member attributes in different contexts. The comments within the code blocks effectively highlight the distinction between item and member attributes.
For consistency, consider adding a comment to the struct example similar to the ones in the function examples:
struct Example { + // <-- this is a member attribute #[builder(default)] field: u32 }
Line range hint
52-58: Valuable historical context added!The inclusion of this historical note about the introduction of
#[derive(bon::Builder)]syntax in version 2.2 is extremely helpful. It provides important context for users who might be transitioning from older versions of the library.To further enhance this section, consider adding a link to the migration guide (if one exists) to help users transition from the deprecated
#[bon::builder]syntax to the new#[derive(bon::Builder)]syntax.
Line range hint
60-442: Comprehensive and well-structured item attributes section!The detailed explanations and relevant examples for each item attribute provide excellent guidance for users implementing builder patterns. The inclusion of examples for different contexts (struct, free function, associated method) is particularly helpful.
For the
builder_typeattribute, consider adding a note about naming conventions. For example:#[builder(builder_type = MyCustomBuilder)] // Consider using PascalCase for type namesThis would reinforce Rust's naming conventions and help maintain consistency in users' codebases.
Line range hint
647-1253: Excellent and detailed member attributes section!The explanations and examples for each member attribute are comprehensive and clear. The inclusion of examples for different contexts (struct field, free function argument, associated method argument) provides valuable guidance for various use cases.
For the
intoattribute, consider adding a brief note about performance implications:#[builder(into)] // Note: This may introduce a slight performance overhead due to the trait object creationThis would help users make informed decisions about when to use this attribute, especially in performance-critical code.
Line range hint
833-839: Valuable caveats and compile errors sections!The inclusion of caveats and compile errors for specific attributes is extremely helpful. This information will assist users in avoiding common pitfalls and understanding the limitations of certain attributes.
To improve visibility, consider adding a visual separator or using a different formatting for these sections. For example:
### Caveats > **Note:** The `self` parameter in associated methods is not available to the `default` expression... ### Compile errors > **Warning:** This attribute is incompatible with members of `Option` type...This would make these important sections stand out more clearly in the documentation.
Also applies to: 1068-1074
Line range hint
1-1253: Outstanding revision of the builder macros documentation!This comprehensive update to the
website/reference/builder.mddocument significantly enhances its value as a reference for thebonlibrary's builder macros. The clear structure, detailed explanations, and numerous examples across different contexts (structs, free functions, and associated methods) provide excellent guidance for users implementing builder patterns in Rust.Key improvements include:
- Clear categorization of item and member attributes
- Detailed explanations and examples for each attribute
- Historical context and migration information
- Caveats and compile error sections for potential pitfalls
To further enhance this excellent documentation:
- Consider adding a table of contents at the beginning of the document for easier navigation.
- Include links to related documentation or examples, if available, for more complex use cases.
- Add a brief section on best practices or common patterns when using these builder macros.
Overall, this revision greatly improves the usability and clarity of the
bonlibrary's documentation. Great work!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (123)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lockbenchmarks/compilation/codegen/src/main.rsis excluded by!**/*.rsbenchmarks/compilation/src/lib.rsis excluded by!**/*.rsbenchmarks/compilation/src/structs_100_fields_10.rsis excluded by!**/*.rsbenchmarks/compilation/src/structs_10_fields_50.rsis excluded by!**/*.rsbenchmarks/runtime/benches/criterion.rsis excluded by!**/*.rsbenchmarks/runtime/benches/iai.rsis excluded by!**/*.rsbenchmarks/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!**/*.rsbenchmarks/runtime/src/lib.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.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/builder_params/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/builder_params/on_params.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/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/member/params/blanket.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/params/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/params/setter_closure.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/member/params/setters.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/setter_methods.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/item_fn.rsis excluded by!**/*.rsbon-macros/src/builder/item_func.rsis excluded by!**/*.rsbon-macros/src/builder/item_impl.rsis excluded by!**/*.rsbon-macros/src/builder/item_struct.rsis excluded by!**/*.rsbon-macros/src/builder/mod.rsis excluded by!**/*.rsbon-macros/src/collections/map.rsis excluded by!**/*.rsbon-macros/src/collections/set.rsis excluded by!**/*.rsbon-macros/src/error.rsis excluded by!**/*.rsbon-macros/src/lib.rsis excluded by!**/*.rsbon-macros/src/normalization/cfg/mod.rsis excluded by!**/*.rsbon-macros/src/normalization/cfg/parse.rsis excluded by!**/*.rsbon-macros/src/normalization/generics_namespace.rsis excluded by!**/*.rsbon-macros/src/normalization/impl_traits.rsis excluded by!**/*.rsbon-macros/src/normalization/lifetimes.rsis excluded by!**/*.rsbon-macros/src/normalization/mod.rsis excluded by!**/*.rsbon-macros/src/normalization/syntax_variant.rsis excluded by!**/*.rsbon-macros/src/parsing/docs.rsis excluded by!**/*.rsbon-macros/src/parsing/item_params.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/tests/attr_setters.rsis excluded by!**/*.rsbon-macros/src/tests/mod.rsis excluded by!**/*.rsbon-macros/src/tests/syntax_errors.rsis excluded by!**/*.rsbon-macros/src/util/attrs.rsis excluded by!**/*.rsbon-macros/src/util/generic_param.rsis excluded by!**/*.rsbon-macros/src/util/ide.rsis excluded by!**/*.rsbon-macros/src/util/ident.rsis excluded by!**/*.rsbon-macros/src/util/meta_list.rsis excluded by!**/*.rsbon-macros/src/util/mod.rsis excluded by!**/*.rsbon-macros/src/util/ty/match_types.rsis excluded by!**/*.rsbon-macros/src/util/ty/mod.rsis excluded by!**/*.rsbon-macros/src/util/visibility.rsis excluded by!**/*.rsbon-macros/tests/snapshots/bon_incomplete_if.rsis excluded by!**/*.rsbon-macros/tests/snapshots/setters_docs_and_vis.rsis excluded by!**/*.rsbon/src/builder_state.rsis excluded by!**/*.rsbon/src/collections.rsis excluded by!**/*.rsbon/src/lib.rsis excluded by!**/*.rsbon/src/private/cfg_eval.rsis excluded by!**/*.rsbon/src/private/deprecations.rsis excluded by!**/*.rsbon/src/private/derives.rsis excluded by!**/*.rsbon/src/private/mod.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_default.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_derive.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_expose_positional_fn.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_into.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_overwritable.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_setters.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_transparent.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_with/mod.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_with/multi_arg.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_with/overwritable.rsis excluded by!**/*.rsbon/tests/integration/builder/attr_with/single_arg.rsis excluded by!**/*.rsbon/tests/integration/builder/builder_derives.rsis excluded by!**/*.rsbon/tests/integration/builder/cfgs.rsis excluded by!**/*.rsbon/tests/integration/builder/init_order.rsis excluded by!**/*.rsbon/tests/integration/builder/legacy.rsis excluded by!**/*.rsbon/tests/integration/builder/mod.rsis excluded by!**/*.rsbon/tests/integration/builder/name_conflicts/builder_state.rsis excluded by!**/*.rsbon/tests/integration/builder/name_conflicts/generics.rsis excluded by!**/*.rsbon/tests/integration/builder/name_conflicts/member_and_type_named_the_same.rsis excluded by!**/*.rsbon/tests/integration/builder/name_conflicts/member_names_state.rsis excluded by!**/*.rsbon/tests/integration/builder/name_conflicts/mod.rsis excluded by!**/*.rsbon/tests/integration/builder/positional_members.rsis excluded by!**/*.rsbon/tests/integration/builder/raw_idents.rsis excluded by!**/*.rsbon/tests/integration/main.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_bon.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_builder.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_derive.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_into.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_on.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_setters.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_skip.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_start_finish_fn.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_transparent.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/attr_with.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/broken_intra_doc_links.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/builder_derives.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/collections.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/derive_builder.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/diagnostic_on_unimplemented.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/errors.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/name_conflicts.rsis excluded by!**/*.rsbon/tests/integration/ui/compile_fail/wrong_delimiters.rsis excluded by!**/*.rsbon/tests/integration/ui/mod.rsis excluded by!**/*.rse2e-tests/src/attr_with.rsis excluded by!**/*.rse2e-tests/src/lib.rsis excluded by!**/*.rse2e-tests/src/reexports.rsis excluded by!**/*.rse2e-tests/src/state_mod_pub.rsis excluded by!**/*.rs
📒 Files selected for processing (45)
- .github/workflows/ci.yml (4 hunks)
- Cargo.toml (3 hunks)
- benchmarks/compilation/Cargo.toml (1 hunks)
- benchmarks/compilation/README.md (1 hunks)
- benchmarks/compilation/codegen/Cargo.toml (1 hunks)
- benchmarks/compilation/results.md (1 hunks)
- benchmarks/compilation/run.sh (1 hunks)
- benchmarks/run.sh (0 hunks)
- benchmarks/runtime/Cargo.toml (2 hunks)
- benchmarks/runtime/README.md (2 hunks)
- benchmarks/runtime/run.sh (1 hunks)
- bon-macros/Cargo.toml (1 hunks)
- bon/Cargo.toml (2 hunks)
- bon/tests/integration/ui/compile_fail/attr_bon.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_builder.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_derive.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_into.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_on.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_setters.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_skip.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_start_finish_fn.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_transparent.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/attr_with.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/broken_intra_doc_links.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/builder_derives.stderr (0 hunks)
- bon/tests/integration/ui/compile_fail/collections.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/derive_builder.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/diagnostic_on_unimplemented.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/errors.stderr (0 hunks)
- bon/tests/integration/ui/compile_fail/name_conflicts.stderr (1 hunks)
- bon/tests/integration/ui/compile_fail/positional_members.stderr (8 hunks)
- bon/tests/integration/ui/compile_fail/warnings.stderr (4 hunks)
- bon/tests/integration/ui/compile_fail/wrong_delimiters.stderr (1 hunks)
- rust-toolchain (1 hunks)
- scripts/install/hyperfine.sh (1 hunks)
- scripts/test-msrv.sh (1 hunks)
- website/.vitepress/config.mts (2 hunks)
- website/.vitepress/theme/utils/versioning.ts (1 hunks)
- website/blog/bon-builder-generator-v2-release.md (3 hunks)
- website/blog/bon-builder-v2-1-release.md (1 hunks)
- website/changelog.md (6 hunks)
- website/guide/overview.md (1 hunks)
- website/guide/patterns/fallible-builders.md (2 hunks)
- website/guide/positional-members.md (2 hunks)
- website/reference/builder.md (5 hunks)
💤 Files with no reviewable changes (3)
- benchmarks/run.sh
- bon/tests/integration/ui/compile_fail/builder_derives.stderr
- bon/tests/integration/ui/compile_fail/errors.stderr
✅ Files skipped from review due to trivial changes (3)
- benchmarks/compilation/README.md
- rust-toolchain
- website/guide/overview.md
🧰 Additional context used
🪛 Markdownlint
benchmarks/compilation/results.md
7-7: null
Spaces inside code span elements(MD038, no-space-in-code)
12-12: null
Spaces inside code span elements(MD038, no-space-in-code)
🪛 Shellcheck
scripts/install/hyperfine.sh
[warning] 10-10: arch_rust is referenced but not assigned.
(SC2154)
🪛 LanguageTool
website/changelog.md
[uncategorized] ~15-~15: A comma might be missing here.
Context: ...attributes syntax. Only parentheses are accepted e.g.#[builder(finish_fn(...))]or `#...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[duplication] ~18-~18: Possible typo: you repeated a word
Context: ...rive(Clone/Debug(bounds(...))))]. ### Removed - Removed support for#[bon::builder]` proc-macr...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~33-~33: A comma might be missing here.
Context: ...optional. - For members with defaults values show the default value in the docs. -...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~34-~34: A comma might be missing here.
Context: ...ult value in the docs. - For optional members provide a link to a companion setter. T...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~36-~36: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...nd should be reported via a Github issue but this provides an easy temporary workaro...(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~86-~86: You might be missing the article “an” here.
Context: ...pull/99)) - Fix a bug where a member of opaqueOptiontype (i.e. theOptiontype t...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[style] ~93-~93: ‘in the meantime’ might be wordy. Consider a shorter alternative.
Context: ...ine the MSRV that should be good enough in the meantime while we work on lowering it even more ...(EN_WORDINESS_PREMIUM_IN_THE_MEANTIME)
[grammar] ~195-~195: There seems to be a noun/verb agreement error. Did you mean “isn't” or “wasn't”?
Context: ...captured generics on an impl block that aren't referenced in the method ([#17](https:/...(SINGULAR_NOUN_AGREEMENT_WHO_DONT)
🔇 Additional comments (119)
benchmarks/compilation/codegen/Cargo.toml (1)
1-11: LGTM: Package metadata is well-defined and follows best practices.The package metadata is correctly specified:
- The name "compilation-benchmarks-codegen" clearly indicates its purpose.
- Setting
publish = falseis appropriate for an internal benchmarking tool.- The description is concise and informative.
- Using Rust edition 2021 ensures access to the latest stable language features.
bon/tests/integration/ui/compile_fail/attr_start_finish_fn.stderr (3)
1-5: LGTM: Correct error message forstart_fn()The error message correctly identifies the issue with the
start_fn()usage in the#[builder]attribute. It accurately points out that parameters are expected in parentheses.
7-11: LGTM: Correct error message forfinish_fn()The error message correctly identifies the issue with the
finish_fn()usage in the#[builder]attribute. It accurately points out that parameters are expected in parentheses.
1-11: Verify the consistency of error messagesBoth error messages are consistent in their format and content, which is good. However, it's important to ensure that these error messages align with the overall error reporting strategy of the
boncrate.To verify the consistency of error messages across the crate, run the following script:
✅ Verification successful
Consistency of error messages confirmed
All error messages related to the
#[builder]attribute are consistent in format and content across thecompile_failtests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in error messages related to #[builder] attribute # Test: Search for similar error messages in other compile_fail tests rg -i "expected parameters in parentheses" tests/integration/ui/compile_failLength of output: 152
Script:
#!/bin/bash # Description: Locate the 'compile_fail' directory within tests/integration fd compile_fail tests/integration -t dLength of output: 145
Script:
#!/bin/bash # Description: Locate all 'compile_fail' directories within the repository fd compile_fail . -t dLength of output: 65
Script:
#!/bin/bash # Description: Check for consistency in error messages related to #[builder] attribute within the located compile_fail directory # Search for the specific error message in the identified directory rg -i "expected parameters in parentheses" bon/tests/integration/ui/compile_fail/Length of output: 1093
bon/tests/integration/ui/compile_fail/attr_bon.stderr (1)
1-11: Overall, the error messages are well-crafted and informative.Both error messages effectively communicate the issues with attribute usage in the
boncrate. They provide clear indications of what went wrong and where, which is crucial for a good developer experience.The suggestions for improvement aim to enhance the messages further by providing immediate guidance on correct usage. This approach not only points out errors but also educates users on proper implementation, potentially reducing future mistakes and improving the learning curve for the crate.
Clear, informative error messages like these are a vital part of a library's usability and can significantly impact developer productivity and satisfaction.
scripts/install/hyperfine.sh (1)
7-10: Version and URL setup looks good, verifyarch_rustvariable.The version and URL setup is well-structured, allowing for easy updates. However, there's a potential issue with the
arch_rustvariable.Please verify that the
arch_rustvariable is defined in the sourced library file. If it's not, you'll need to define it before use. Here's a script to check its definition:If the variable is not found, consider adding its definition to the script or the library file.
✅ Verification successful
arch_rustvariable is properly defined.The
arch_rustvariable is correctly defined inscripts/install/lib.sh, ensuring proper URL construction.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if arch_rust is defined in the library file # Test: Search for arch_rust definition in the library file rg -n 'arch_rust=' "$(dirname "${BASH_SOURCE[0]}")/lib.sh"Length of output: 126
Script:
#!/bin/bash # Description: Search for arch_rust definition across the entire codebase rg -n 'arch_rust=' .Length of output: 95
🧰 Tools
🪛 Shellcheck
[warning] 10-10: arch_rust is referenced but not assigned.
(SC2154)
benchmarks/runtime/Cargo.toml (3)
2-2: Approve package name changeThe package name change from "benchmarks" to "runtime-benchmarks" is more specific and descriptive. This change improves clarity and helps distinguish this package from other potential benchmark types in the project.
7-7: Approve description updateThe updated description is more concise and accurately describes the crate's purpose. Removing "manual" from "manual benchmarking" is appropriate as it doesn't unnecessarily limit the scope of the benchmarking capabilities.
Line range hint
1-34: Summary of changes and their impactThe changes made to this
Cargo.tomlfile are consistent with the PR objectives and the restructuring of the workspace. The package has been appropriately renamed, the description updated, and the dependency path adjusted. These modifications improve clarity and align the package with its specific purpose of runtime benchmarking.The changes appear to be well-considered and do not introduce any apparent issues. They contribute to a more organized and clearly defined project structure.
benchmarks/compilation/Cargo.toml (3)
1-12: LGTM: Package metadata is well-defined.The package metadata is appropriately set up for a benchmarking crate. The name, description, and other fields are clear and informative. Good job on using the latest Rust edition (2021) and following semantic versioning.
20-21: LGTM: Workspace lints enabled.Good practice to use workspace-level lint configuration. This ensures consistency across all crates in the workspace.
23-28: Features are well-structured. Clarification needed on empty feature flags.The features section is well-organized, allowing for flexible benchmarking configurations.
Could you provide more information about the purpose of the empty feature flags
structs_100_fields_10andstructs_10_fields_50? Are these placeholders for future benchmarking scenarios?To help understand the usage of these features, let's check for any references in the codebase:
✅ Verification successful
Verified: Empty feature flags control conditional compilation.
The empty feature flags
structs_100_fields_10andstructs_10_fields_50are used to enable conditional compilation of their respective modules inbenchmarks/compilation/src/lib.rs. Their current configuration effectively facilitates flexible benchmarking scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the empty feature flags rg "structs_100_fields_10|structs_10_fields_50" --type rustLength of output: 341
benchmarks/compilation/run.sh (2)
1-3: LGTM: Proper script setup with good error handling.The script starts with the correct shebang and sets appropriate shell options for error handling and debugging. This is a good practice that will help catch and diagnose issues during script execution.
1-25: Overall: Excellent benchmarking script that aligns well with PR objectives.This script is well-structured and effectively implements a comprehensive benchmarking process for different macro and struct configurations. It addresses the performance concerns mentioned in the PR summary by allowing for systematic comparison of different approaches to the builder pattern.
Key strengths:
- Good error handling and debugging setup
- Clear and extensible definition of test cases
- Effective use of
hyperfinefor benchmarking- Consistent build and clean processes for each test run
The minor suggestions provided (adding comments, post-processing results, and additional error handling) would further enhance the script's maintainability and usefulness. Great job on creating a valuable tool for assessing the performance impact of the new builder pattern implementations!
bon/tests/integration/ui/compile_fail/derive_builder.stderr (3)
1-6: LGTM: Clear and informative error messageThe error message accurately describes the issue with using a tuple struct instead of a struct with named fields. It provides the correct line number and file name, which is helpful for users to locate and fix the problem.
7-12: LGTM: Consistent error reportingThe second error message maintains consistency with the first one, correctly identifying the same issue with using a tuple struct. The updated line number accurately reflects the new location of the error in the source file.
1-19: Overall: Well-structured and informative error messagesThe error messages in this file effectively communicate the issues with using the
Builderderive macro on unsupported struct types. They provide consistent and clear information, including file names and line numbers, which is crucial for debugging. The additional information about macro-backtrace in the third error message is particularly helpful for advanced users.The minor inconsistency noted earlier doesn't significantly impact the overall quality of the error reporting. These error messages will greatly assist users in understanding and resolving issues related to the
Builderderive macro usage.bon/tests/integration/ui/compile_fail/collections.stderr (5)
1-6: LGTM: Correct error message for duplicate map key.The error message accurately identifies the duplicate key "Hello" in the map at line 5 of the test file. This is the expected behavior for a compile-fail test checking for duplicate keys in collections.
7-12: LGTM: Correct error message for second duplicate map key.The error message correctly identifies the second occurrence of the duplicate key "Hello" in the map at line 6 of the test file. This is the expected behavior for a compile-fail test checking for duplicate keys in collections.
13-18: LGTM: Correct error message for duplicate set value.The error message accurately identifies the duplicate value "mintals" in the BTreeSet at line 9 of the test file. This is the expected behavior for a compile-fail test checking for duplicate values in set collections.
19-23: LGTM: Correct error message for second duplicate set value.The error message correctly identifies the second occurrence of the duplicate value "mintals" in the BTreeSet at line 9 of the test file. This is the expected behavior for a compile-fail test checking for duplicate values in set collections.
1-23: Summary: All error messages are correct and as expected.This file contains the expected compiler error messages for a compile-fail test checking for duplicate keys in maps and duplicate values in sets. All four error messages accurately identify the issues at the correct locations in the test file. These messages demonstrate that the
boncrate is correctly implementing compile-time checks for these collection-related errors.website/.vitepress/theme/utils/versioning.ts (2)
Line range hint
3-41: LGTM! Changes integrate well with existing code.The removal of the
useRouterimport doesn't affect the rest of the file. TheparseRouteAsVersionedfunction and other utilities remain intact and continue to use thewithBaseimport correctly. The overall structure and functionality of the file are preserved.
1-1: LGTM! Verify unused import removal.The removal of the
useRouterimport is appropriate if it's no longer used in this file. This change helps keep the imports clean and relevant.Let's verify that
useRouteris indeed unused in this file:benchmarks/compilation/results.md (1)
1-12: Well-structured and formatted benchmark results table.The table presents the benchmark results in a clear and organized manner. The structure, alignment, and formatting make it easy to read and compare performance metrics across different configurations.
🧰 Tools
🪛 Markdownlint
7-7: null
Spaces inside code span elements(MD038, no-space-in-code)
12-12: null
Spaces inside code span elements(MD038, no-space-in-code)
benchmarks/runtime/README.md (2)
3-3: LGTM: Improved clarity in benchmark description.The addition of "runtime benchmarks" provides more specific information about the nature of the benchmarks, which enhances the documentation's clarity.
21-21: LGTM: Enhanced clarity in execution instructions.The addition of "from this directory" provides clearer instructions on where to execute the benchmark command, which improves the overall usability of the documentation.
scripts/test-msrv.sh (2)
42-42: Verify the impact of removing--all-featuresfrom ClippyThe
--all-featuresflag has been removed from thecargo clippycommand. While this may speed up the linting process, it could potentially miss issues that only appear with certain feature combinations.To ensure this change doesn't compromise the thoroughness of your linting process:
- Verify that your CI pipeline includes separate jobs that test different feature combinations.
- Consider the trade-off between faster CI runs and comprehensive linting.
Run the following script to check if there are other Clippy runs with different feature combinations:
If the results show Clippy runs with various feature combinations, this change is likely safe. Otherwise, consider adding such runs or reverting this change.
✅ Verification successful
Clippy remains configured to run with all features in CI
The
--all-featuresflag is still used in.github/workflows/ci.yml, ensuring that Clippy checks all feature combinations. Removing--all-featuresfromscripts/test-msrv.shwill not compromise the linting process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Clippy runs with different feature combinations in CI config # Test: Search for Clippy commands in CI config files rg -i 'cargo\s+clippy' .github/workflows/*.ymlLength of output: 205
Line range hint
44-57: Verify the impact of removing--all-featuresfrom test argumentsThe
--all-featuresflag has been removed from thetest_argsarray. This change aligns with the modification to the Clippy command, but it's important to ensure that it doesn't compromise the thoroughness of your testing process.To ensure this change doesn't lead to missed test failures:
- Verify that your CI pipeline includes separate jobs that test different feature combinations.
- Consider the trade-off between faster CI runs and comprehensive testing.
Run the following script to check if there are other test runs with different feature combinations:
If the results show test runs with various feature combinations, this change is likely safe. Otherwise, consider adding such runs or reverting this change.
Additionally, ensure that your test suite includes tests that cover different feature combinations, even if they're not all run in this MSRV test script.
✅ Verification successful
Removal of
--all-featuresintest-msrv.shVerifiedThe CI pipeline already includes comprehensive test runs with various feature combinations. Removing
--all-featuresfrom thetest_argsintest-msrv.shdoes not compromise the thoroughness of your testing process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test runs with different feature combinations in CI config # Test: Search for cargo test commands in CI config files rg -i 'cargo\s+test' .github/workflows/*.ymlLength of output: 727
bon/tests/integration/ui/compile_fail/name_conflicts.stderr (4)
1-9: LGTM: Appropriate error for compile_fail testThis error message correctly identifies the use of an undeclared lifetime
'fn1in thebodyfunction. It's an expected outcome for a compile_fail test, demonstrating that the#[builder]macro doesn't introduce the'fn1lifetime as intended. This test effectively verifies the macro's behavior regarding lifetime declarations.
10-19: LGTM: Valuable test case for builder attributesThis error message correctly identifies the use of an undeclared lifetime
'fn1in theattr_withfunction's builder attribute. It's an appropriate outcome for a compile_fail test, showcasing how the#[builder]macro handles (or fails to handle) lifetimes in custom attributes. This test case is particularly valuable as it verifies the macro's behavior in more complex scenarios involving builder attributes and lifetimes.
20-30: LGTM: Comprehensive testing of lifetime handlingThis error message correctly identifies the use of an undeclared lifetime
'fn1in a variable declaration. It's an expected and valuable outcome for a compile_fail test, demonstrating that the#[builder]macro consistently fails to introduce the'fn1lifetime across different contexts. This test case, along with the previous two, ensures comprehensive coverage of the macro's behavior regarding lifetime declarations and conflicts.
1-30: Excellent coverage of lifetime handling edge casesThe compile_fail tests represented by these error messages provide excellent coverage of lifetime handling edge cases for the
#[builder]macro. They effectively test:
- Basic lifetime declaration in functions
- Lifetime usage in builder attributes
- Lifetime usage in variable declarations
These tests ensure that the
#[builder]macro correctly fails to introduce lifetimes in various contexts, which is crucial for maintaining type safety and preventing unexpected behavior in user code. The comprehensive nature of these tests contributes significantly to the robustness of thebonlibrary's builder implementation.bon-macros/Cargo.toml (1)
51-53: Excellent documentation for dependency version constraintThe added comment provides crucial context for the specific version requirement of
proc-macro2. It's commendable that you've included a link to the relevant GitHub issue, which will be helpful for future maintenance and understanding of this constraint.bon/tests/integration/ui/compile_fail/attr_on.stderr (6)
1-5: LGTM: Clear and informative error messageThis error message effectively communicates the redundancy of the
#[builder(into)]attribute when a#[builder(on(...))]attribute is already present at the function level. It helps users avoid unnecessary attribute usage and understand the implied behavior.
7-11: LGTM: Consistent error message for redundant attributesThis error message maintains consistency with the previous one, effectively communicating the redundancy of the
#[builder(overwritable)]attribute when a#[builder(on(...))]attribute is already present at the function level. This consistency helps users understand the behavior of multiple builder attributes.
19-23: LGTM: Clear error message for nested attributesThis error message effectively communicates that nested attributes are not allowed in the type pattern of
#[builder(on(type_pattern, ...))]. It clearly points out the problematic#[attr]nested attribute, helping users identify and correct the issue in their code.
25-29: LGTM: Helpful error message with correction suggestionThis error message clearly identifies the issue with an empty
#[on]attribute and provides helpful suggestions to correct it. The message offers two valid options: removing the attribute or passing parameters in parentheses (#[on(...)]). This guidance helps users quickly resolve the issue and understand the correct attribute syntax.
31-35: LGTM: Concise error message for missing parametersThis error message succinctly points out that parameters are expected in parentheses for the
#[builder(on())]attribute. The message is clear and directly identifies the issue, helping users understand and correct the attribute syntax.
13-17: LGTM: Informative error message with actionable feedbackThis error message clearly communicates that the syntax
&dyn std::fmt::Debugis not supported in type patterns for the builder attribute. It also provides actionable feedback by suggesting users open an issue for this use case.Let's verify if the GitHub link in the error message is correct and accessible:
✅ Verification successful
LGTM: Informative error message with accessible issue link
The error message effectively communicates that the syntax
&dyn std::fmt::Debugis not supported in type patterns for the builder attribute. Additionally, the provided GitHub issues link (https://github.com/elastio/bon/issues) is correct and accessible, offering users a clear path to request support for this feature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the GitHub issues link is correct and accessible GITHUB_LINK="https://github.com/elastio/bon/issues" HTTP_STATUS=$(curl -s -o /dev/null -w "%{http_code}" $GITHUB_LINK) if [ $HTTP_STATUS -eq 200 ]; then echo "GitHub link is correct and accessible." else echo "Error: GitHub link returned HTTP status $HTTP_STATUS" fiLength of output: 258
bon/tests/integration/ui/compile_fail/attr_skip.stderr (9)
1-6: LGTM: Correct error for incompatible attributesThis error message accurately reflects the incompatibility between
skipandintoattributes. It provides clear guidance to the user about the incorrect usage.
7-12: LGTM: Proper error for conflicting attributesThe error message correctly identifies the conflict between
skipandnameattributes. This helps maintain the consistency and clarity of the builder pattern implementation.
13-18: Excellent error message with helpful suggestionThis error message goes beyond just identifying the problem. It provides a clear explanation of why
skipanddefaultcan't be used together, and offers a helpful alternative syntax. This kind of user-friendly error message significantly improves the developer experience.
19-24: LGTM: Clear error for unsupported attribute usageThis error message accurately identifies the misuse of the
skipattribute on function arguments. The suggestion to use a local variable provides a helpful alternative, guiding developers towards the correct usage.
25-30: LGTM: Consistent error messageThis error message maintains consistency with the previous one, reinforcing the rule against using
skipon function arguments. The repetition of the same clear guidance helps ensure that developers understand this restriction regardless of the specific context.
31-36: LGTM: Consistently applied error messageThis error message maintains the same clear guidance across different cases of misusing the
skipattribute on function arguments. This consistency helps reinforce the rule and ensures that developers receive the same helpful advice regardless of the specific context.
37-45: LGTM: Error correctly reflectsskipattribute behaviorThis error message accurately demonstrates the effect of the
skipattribute on the builder pattern. By showing that the methodxis not generated for the builder, it helps developers understand the implications of usingskip. This is crucial for preventing unexpected behavior in the final builder implementation.
46-53: LGTM: Consistent error message for skipped fieldsThis error message maintains consistency with the previous one, demonstrating that the
skipattribute behaves uniformly across different fields. By showing that neitherxnorymethods are generated, it reinforces the developer's understanding of howskipaffects the builder pattern implementation.
1-53: Excellent set of error messages forskipattribute usageThe error messages in this file collectively provide comprehensive and consistent guidance on the proper use of the
skipattribute in theboncrate's builder pattern implementation. They effectively cover various misuse cases, including:
- Incompatibility with other attributes (
into,name,default)- Unsupported usage on function arguments
- The effect of
skipon setter method generationThe messages are clear, concise, and often provide helpful suggestions for correct usage. This level of detail and consistency in error reporting significantly enhances the developer experience when working with the
boncrate.bon/tests/integration/ui/compile_fail/attr_setters.stderr (10)
1-6: LGTM: Clear and informative error messageThis error message effectively communicates that the
nameconfiguration is unused due to overrides in the setter functions. It provides the exact location of the issue, which is helpful for debugging.
7-12: LGTM: Consistent error reportingThis error message maintains consistency with the previous one, correctly identifying another instance of an unused
nameconfiguration. The different line number (16) indicates that this issue occurs multiple times in the source file.
13-18: LGTM: Consistent and clear error for visibility configurationThis error message follows the established pattern, clearly indicating that the
vis(visibility) configuration is unused due to overrides in the setter functions. The specific line number (25) is provided, maintaining the helpful debugging information.
19-24: LGTM: Consistent error reporting for documentation configurationThis error message maintains the established pattern, clearly indicating that the
doc(documentation) configuration is unused due to overrides in the setter functions. The specific line number (32) is provided, continuing the trend of helpful debugging information.
25-30: LGTM: Informative error message for setter function usageThis error message clearly explains the conditions under which the
some_fnsetter function can be used. It provides valuable information about the required attributes or types, including the exception case with#[builder(transparent)]. The specific line number (47) aids in locating the issue in the source code.
31-36: LGTM: Consistent error reporting for option_fn setterThis error message maintains consistency with the previous one, applying the same usage rules to the
option_fnsetter. It clearly explains the conditions under whichoption_fncan be used, including the exception case. The specific line number (53) continues to provide helpful debugging information.
37-42: LGTM: Consistent error for some_fn in transparent builder contextThis error message maintains consistency with previous messages about
some_fn, correctly identifying its misuse in a transparent builder context. It reiterates the conditions for proper usage, ensuring users understand the restrictions even when using#[builder(transparent)]. The specific line number (59) continues to aid in debugging.
43-48: LGTM: Consistent error for option_fn in transparent builder contextThis error message maintains consistency with previous messages about
option_fn, correctly identifying its misuse in a transparent builder context. It reiterates the conditions for proper usage, ensuring users understand the restrictions even when using#[builder(transparent)]. The specific line number (65) continues to provide valuable debugging information.
49-53: LGTM: Clear syntax error messageThis error message concisely points out the syntax issue with the
settersattribute. It clearly states that parameters are expected within the parentheses, helping users quickly identify and correct the problem. The specific line number (71) aids in locating the issue in the source code.
1-53: Excellent error messages for builder pattern implementationThe error messages in this file are consistently well-crafted, providing clear and informative feedback to users. They effectively cover various scenarios related to the builder pattern implementation, including:
- Unused configurations for
name,vis, anddoc- Proper usage of
some_fnandoption_fnsetter functions- Considerations for transparent builders
- Syntax requirements for the
settersattributeEach error message includes the specific line number where the issue occurs, greatly aiding in debugging. The consistency in error reporting across different scenarios helps users quickly understand and address issues in their code.
These error messages contribute significantly to a positive developer experience when working with the builder pattern in this library.
bon/tests/integration/ui/compile_fail/broken_intra_doc_links.stderr (3)
1-35: Approval: Error messages correctly identify potential documentation issuesThese error messages are correctly identifying the use of
Selfin documentation comments, which could lead to confusion when the documentation is moved to the builder struct's implementation block. This aligns with the PR objectives of improving the builder pattern and enhancing type safety.The consistency of these errors across multiple instances (lines 1-5, 7-11, 13-17, 19-23, 25-29, 31-35) demonstrates a thorough check for this potential issue throughout the code.
37-41: Note: Slight variation in error message for builder structThis error message (lines 37-41) specifically mentions the builder struct rather than the impl block. It's a subtle but important distinction that shows the compiler is aware of different contexts where
Selfcould be misused in documentation.This level of detail in the error messages contributes to the overall improvement of the builder pattern usage and documentation clarity.
49-53: Highlight: Error message addresses builder's state moduleThis error message (lines 49-53) is particularly noteworthy as it addresses the use of
Selfin the context of the builder's state module. This aligns closely with the PR objectives of improving flexibility in builder types and managing visibility of setter methods.The presence of this error demonstrates that the compiler checks extend to the internal representation of builder states, which is a key aspect of the enhancements mentioned in the PR summary.
bon/tests/integration/ui/compile_fail/warnings.stderr (5)
55-55: Approved: Error message update aligns with PR objectivesThe change from
ExampleBuilder::<(__X, __Y)>toExampleBuilder::<S>in the error message reflects a more flexible and generic approach to type parameters in the builder pattern. This aligns well with the PR's objective of improving the flexibility and usability of builder types.
67-67: Approved: Consistent error message updateThe change from
ExampleMustUseBuildertoExampleMustUseBuilder::<S>in the error message is consistent with the previous update. This maintains a uniform approach to type parameters across different builder types in the library, enhancing overall consistency.
78-78: Approved: Consistent error message update patternThe change from
MustUseBuildertoMustUseBuilder::<S>in the error message follows the same pattern as the previous updates. This consistent approach to updating type parameters across different builder types enhances the overall coherence of the library's error messages.
100-100: Approved: Consistent error message updates enhance library coherenceThe change from
MustUseUnderCfgBuildertoMustUseUnderCfgBuilder::<S>in the error message completes the consistent update pattern across all builder types in the library. These uniform changes to type parameters enhance the overall coherence and flexibility of the library's error reporting. The preservation of the additional note "must use message" maintains important context for users.
Line range hint
55-100: Summary: Consistent type parameter updates improve flexibility and coherenceThe changes to error messages in this file demonstrate a systematic update of type parameters from
(__X, __Y)toSacross various builder types. This consistent approach aligns well with the PR's objectives of improving the flexibility and usability of builder types. The updates enhance the overall coherence of the library's error reporting while maintaining clear and informative messages for users. These changes contribute positively to the library's design and user experience.bon/tests/integration/ui/compile_fail/diagnostic_on_unimplemented.stderr (4)
1-21: Excellent error message for unset required field.The error message for the unset
yfield is clear, informative, and helpful. It correctly identifies the issue, provides the exact location of the error, and offers guidance on the required trait implementation. This level of detail will greatly assist developers in understanding and resolving builder pattern issues.
23-43: Consistent error reporting for multiple unset fields.The error message for the unset
renamedfield maintains the same high quality and level of detail as the previous error. This consistency in error reporting across different fields is excellent, as it provides a uniform experience for developers when troubleshooting builder pattern issues.
45-61: Informative error for attempting to set an already set field.This error message effectively captures an important aspect of the builder pattern - preventing duplicate field assignments. It clearly explains that the
yfield was already set and cannot be set again. The level of detail provided, including information about theIsUnsettrait, is excellent and will help developers understand the internal workings of the builder pattern implementation.
1-61: Comprehensive and developer-friendly error messages for the Builder pattern.The error messages in this file demonstrate a robust and well-implemented Builder pattern in the
bonlibrary. They cover crucial scenarios such as unset required fields and attempts to set already-set fields. The consistency, clarity, and level of detail in these error messages are commendable. They not only point out what went wrong but also provide valuable information about the underlying traits and implementations.These high-quality error messages will significantly enhance the developer experience when using the
bonlibrary, making it easier to understand and correctly implement the Builder pattern. They align well with the PR objectives of improving the flexibility and usability of builder types.bon/tests/integration/ui/compile_fail/attr_builder.stderr (7)
1-6: LGTM: Clear error message for redundant empty attributeThe error message correctly identifies the redundant use of an empty
#[builder]attribute and provides a clear suggestion to remove it. This helps users understand and correct their code easily.
7-12: LGTM: Consistent error messages for missing parametersThe error messages consistently identify instances where parameters are expected in parentheses for the
#[builder]attribute. This helps users understand the correct syntax for using the attribute.Also applies to: 19-24, 31-36, 43-48
13-18: LGTM: Helpful error messages for unexpected empty attributesThe error messages clearly identify unexpected empty
#[builder]attributes and provide actionable suggestions to either remove them or pass parameters in parentheses. This guidance helps users correct their code effectively.Also applies to: 25-30, 37-42
61-68: LGTM: Clear error message for unsupported itemsThe error message accurately states that only
fnitems are supported by the#[bon::builder]attribute. This helps users understand the limitations of the attribute's usage.
69-74: LGTM: Precise error message for multiple #[must_use] attributesThe error message accurately informs users that bon only works with exactly one or zero
#[must_use]attributes. This clear guidance helps prevent misuse and potential confusion.
75-85: LGTM: Helpful error messages for unsupported destructuring patternsThe error messages clearly indicate that destructuring patterns in function arguments are not supported by the
#[builder]attribute. They provide a helpful suggestion to use a simpleidentifier: typesyntax instead. This guidance assists users in correcting their code to work with the#[builder]attribute.
1-85: Overall excellent quality of error messagesThe error messages in this file demonstrate high quality and consistency in communicating various misuses of the
#[builder]attribute. They provide clear, actionable guidance to users, helping them understand and correct issues related to:
- Empty and redundant attributes
- Missing parameters in parentheses
- Incorrect usage on structs
- Unsupported items
- Multiple
#[must_use]attributes- Unsupported destructuring patterns
These well-crafted error messages significantly enhance the user experience when working with the
bonlibrary's builder functionality.bon/Cargo.toml (3)
5-5: Improved package descriptionThe updated description provides a more comprehensive and accurate representation of the package's capabilities. It effectively highlights the key features such as compile-time checking, builder generation, and named function arguments.
Line range hint
1-104: Overall improvements to package metadata and featuresThe changes to
Cargo.tomleffectively support the PR objectives of enhancing the builder pattern in thebonlibrary. The updated description and the new 'implied-bounds' feature contribute to improved flexibility and usability of builder types. These modifications align well with the goals outlined in issue #21, providing a more generic and type-safe approach to handling incomplete builders.
65-104: Well-documented new 'implied-bounds' featureThe addition of the 'implied-bounds' feature is a valuable enhancement to the package. The extensive comments provide clear context and a practical example, effectively explaining the feature's purpose and benefits. This addition aligns well with the PR objectives of improving the flexibility and usability of builder types.
A few points to note:
- The feature correctly leverages Rust 1.79.0's stabilization of bounds in associated type positions.
- The example effectively demonstrates how this feature can improve type safety and usability.
- The comments are thorough and provide clear guidance for users.
To ensure the feature is properly integrated, let's verify its implementation:
✅ Verification successful
'implied-bounds' Feature Verified Successfully
All verification checks confirm that the 'implied-bounds' feature is correctly defined in
Cargo.toml, properly utilized within the Rust codebase, and that theIsCompletetrait is implemented as expected. The feature integration aligns with the project's objectives and enhances type safety and usability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the 'implied-bounds' feature implementation # Test 1: Check if the feature is properly defined in Cargo.toml grep -q 'implied-bounds = \["bon-macros/implied-bounds"\]' bon/Cargo.toml if [ $? -eq 0 ]; then echo "Feature 'implied-bounds' is correctly defined in Cargo.toml" else echo "Error: Feature 'implied-bounds' is not correctly defined in Cargo.toml" fi # Test 2: Check if the feature is used in the code rg -i 'implied-bounds' --type rust if [ $? -eq 0 ]; then echo "Feature 'implied-bounds' is used in the Rust code" else echo "Warning: Feature 'implied-bounds' is not found in the Rust code. Ensure it's implemented." fi # Test 3: Check for the presence of IsComplete trait in the code rg 'trait\s+IsComplete' --type rust if [ $? -eq 0 ]; then echo "IsComplete trait found in the code" else echo "Warning: IsComplete trait not found. Verify the implementation." fiLength of output: 782
bon/tests/integration/ui/compile_fail/positional_members.stderr (8)
Line range hint
1-8: LGTM: Error message spelling correctedThe spelling correction from "oredering" to "ordering" improves the clarity of the error message. This change enhances the readability and professionalism of the compiler output.
Line range hint
10-17: LGTM: Consistent error message correctionThe spelling correction is consistently applied to this error message, maintaining uniformity across the compiler output.
Line range hint
19-24: LGTM: Consistent error message correctionThe spelling correction is consistently applied to this error message, maintaining uniformity across the compiler output.
28-31: LGTM: Consistent error message correction for finish_fnThe spelling correction is consistently applied to this error message, which correctly refers to the
#[builder(finish_fn)]attribute. This maintains uniformity and accuracy across the compiler output.
Line range hint
37-47: LGTM: Consistent error messages for start_fn and finish_fnThe spelling corrections are consistently applied to both error messages, correctly referring to
#[builder(start_fn)]and#[builder(finish_fn)]attributes respectively. This maintains uniformity and accuracy across the compiler output.
Line range hint
55-65: LGTM: Consistent error messages for start_fnThe spelling corrections are consistently applied to both error messages, correctly referring to
#[builder(start_fn)]attributes. This maintains uniformity and accuracy across the compiler output.
Line range hint
64-91: LGTM: Clear type mismatch error for TypePatternMatching::builderThis new error message clearly explains the type mismatch in the
TypePatternMatching::buildermethod call. It provides useful information about the expected and found types, as well as the location of the error and the associated function definition for context.
Line range hint
92-95: LGTM: Clear type mismatch error for TypePatternMatching::buildThis new error message clearly explains the type mismatch in the
TypePatternMatching::buildmethod call. It provides useful information about the expected and found types, as well as the location of the error and the method definition for context.website/guide/positional-members.md (4)
67-70: LGTM: Enhanced builder pattern with finishing function.The addition of
finish_fn = claimto the#[builder]attribute is well-implemented and consistent with the explanation provided in the documentation. This enhancement allows for a more intuitive and flexible builder pattern by introducing a named finishing function.
106-109: LGTM: Consistent implementation of finishing function.The addition of
finish_fn = claimin this example is consistent with the previous demonstration. It effectively reinforces the concept of using a finishing function in the builder pattern across different contexts.
Line range hint
114-122: LGTM: Enhanced flexibility withintoattribute.The addition of the
intoattribute to thefinish_fnfields and thelabelfield is well-implemented. This change:
- Allows for more flexible input types in the finishing function.
- Reduces the need for explicit
to_owned()calls, as explained in the documentation.- Demonstrates an advanced usage of the builder pattern, combining positional parameters with type conversion.
These enhancements improve the usability of the API while maintaining type safety.
Line range hint
67-122: Excellent enhancements to builder pattern documentation and functionality.The changes in this file significantly improve the documentation and functionality of the builder pattern in the
bonlibrary. Key improvements include:
- Clear explanation and demonstration of positional parameters for both starting and finishing functions.
- Introduction of the
finish_fnattribute, enhancing the flexibility of the builder pattern.- Effective use of the
intoattribute to reduce boilerplate and improve API ergonomics.These changes align well with the PR objectives of improving the flexibility and usability of builder types. The documentation is clear, consistent, and provides excellent examples that will help users understand and implement these advanced features effectively.
Cargo.toml (4)
141-142: Improved clarity in lint configuration comments.The updated comments provide a clearer explanation of the lint priority system, which enhances the maintainability of the configuration file. This change is beneficial for developers working on the project.
Line range hint
1-187: Overall, the changes to Cargo.toml are well-aligned with the PR objectives.The modifications to the workspace structure, lint configurations, and enabling of unstable lints all contribute to the goals of improving flexibility, type safety, and performance. The changes look good, but please ensure to run the suggested verification scripts to catch any potential issues early.
186-187: Enabling unstable lints for improved safety.Uncommenting the
lossy_provenance_castsandfuzzy_provenance_castslints is a good step towards improving type safety, which aligns with the PR objectives. However, as these are unstable lints, we should be cautious.To ensure these lints don't cause unexpected issues, please run the following verification script:
If warnings are found, consider addressing them or documenting the reasons for ignoring them.
✅ Verification successful
No warnings detected with the newly enabled lints.
The enabled
lossy_provenance_castsandfuzzy_provenance_castslints do not trigger any warnings. Verification successful.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the newly enabled lints cause any warnings in the codebase # Run cargo check with the new lints enabled cargo check --all-targets 2>&1 | grep -E "warning:.*provenance_casts" > lint_warnings.txt if [ -s lint_warnings.txt ]; then echo "Warnings found for the newly enabled lints:" cat lint_warnings.txt else echo "No warnings found for the newly enabled lints." fi rm lint_warnings.txtLength of output: 223
2-10: Workspace structure update looks good.The reorganization of the
membersfield and the addition of theresolver = "2"are positive changes. They align with the PR objectives by potentially improving the project's structure and dependency resolution.To ensure the new structure is correctly set up, please run the following verification script:
✅ Verification successful
Benchmark directories and Cargo.toml files verified successfully.
All specified benchmark directories and their respective
Cargo.tomlfiles exist as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of new benchmark directories and Cargo.toml files # Check for the existence of new benchmark directories for dir in benchmarks/compilation benchmarks/compilation/codegen benchmarks/runtime; do if [ ! -d "$dir" ]; then echo "Error: Directory $dir does not exist" exit 1 fi done # Check for Cargo.toml files in each new benchmark directory for dir in benchmarks/compilation benchmarks/compilation/codegen benchmarks/runtime; do if [ ! -f "$dir/Cargo.toml" ]; then echo "Warning: Cargo.toml not found in $dir" fi done echo "Verification complete"Length of output: 849
bon/tests/integration/ui/compile_fail/attr_with.stderr (5)
1-6: LGTM: Clear and helpful error message for incorrectwithparameter usage.The error message effectively communicates that a closure is expected for the
withparameter and provides an example of the correct syntax. This guidance will help users understand and correct their usage of the#[builder]attribute.
13-42: LGTM: Consistent and clear error messages for disallowed keywords.The error messages for
for<...>,const,static,async, andmovekeywords are consistent and clearly communicate that these keywords are not allowed in the context of thewithattribute closure. This helps maintain the intended usage of the#[builder]attribute.
94-146: LGTM: Consistent error messages for incorrectResulttype usage.These error messages effectively catch various misuses of the
Resulttype in the context of thewithattribute. By referring back to the comprehensive explanation provided earlier, they maintain consistency and avoid redundancy in the error output.
147-167: LGTM: Clear error message for type mismatch inOkvariant.This error message effectively communicates the type mismatch between the expected
u32and the provided&strin theOkvariant. The additional notes about the source of the mismatch and theOkvariant definition provide valuable context to help users understand and resolve the issue.
1-179: Summary: Comprehensive and informative error messages for#[builder]attribute usage.Overall, the error messages in this file provide clear, consistent, and helpful guidance for users of the
#[builder]attribute. They effectively cover various misuse scenarios and often include detailed explanations or suggestions for correct usage. The messages align well with the objectives of improving the flexibility and usability of builder types while maintaining type safety.Some minor improvements could be made to provide more context or examples in specific cases, which would further enhance the user experience when working with the
bonlibrary's builder pattern implementation.website/.vitepress/config.mts (4)
155-155: Improved clarity in sidebar navigationThe change from "Builder macros" to "#[derive(Builder)] / #[builder]" enhances the specificity and accuracy of the documentation structure. This modification better reflects the actual Rust syntax used for the Builder derive macro, making it more intuitive for developers familiar with Rust.
159-160: Enhanced consistency with Rust terminologyThe change from "Top-Level Attributes" to "Item attributes" improves the alignment of the documentation with standard Rust terminology. This modification enhances the overall consistency of the documentation and makes it more intuitive for Rust developers. The corresponding link update maintains the navigation integrity.
189-190: Consistent terminology improvementThe change from "Member-Level Attributes" to "Member attributes" continues the trend of aligning the documentation with standard Rust terminology. This modification maintains consistency with the previous change and further enhances the overall clarity of the documentation structure. The updated link ensures seamless navigation.
Line range hint
155-190: Overall improvement in documentation structureThe changes made to the sidebar navigation structure in this configuration file collectively enhance the clarity and usability of the documentation. By adopting more precise Rust terminology and maintaining consistent naming conventions, these modifications create a more intuitive and developer-friendly documentation experience. The updates align well with the PR objectives of improving the usability of the
bonlibrary and its documentation.website/changelog.md (1)
40-40: LGTM! Important fix for conditional compilation.The fix for
#[cfg/cfg_attr()]expansion on function arguments with doc comments or other attributes is a valuable improvement. This enhances support for conditional compilation, which is crucial for cross-platform code.bon/tests/integration/ui/compile_fail/attr_derive.stderr (5)
1-5: LGTM: Correct error for invalid attribute syntaxThis error message correctly identifies the syntax issue in the
#[builder(derive())]attribute. It's likely an intentional test case to ensure the compiler catches incorrect attribute usage.
19-30: LGTM: Consistent errors for missingCloneimplementationThese error messages correctly identify the lack of
Cloneimplementation forNoTraitImpls. This is likely intentional to test the compiler's ability to detect missing trait implementations in various contexts (struct fields, function parameters, etc.).Also applies to: 158-169, 297-313
82-99: LGTM: Consistent errors for missingDebugimplementationThese error messages correctly identify the lack of
Debugimplementation forNoTraitImpls. This is likely intentional to test the compiler's ability to detect missing trait implementations in various contexts (struct fields, function parameters, etc.).Also applies to: 221-238, 366-379
7-18: LGTM: Correct errors for invalid derive macro usageThese error messages correctly identify issues with the
derivemacro usage within thebuilderattribute:
- The missing
boundsfield in#[builder(derive(Clone()))].- The incorrect delimiter usage (curly braces instead of parentheses).
These are likely intentional test cases to ensure the compiler catches misuse of the
derivemacro in the context of thebuilderattribute.
1-454: LGTM: Comprehensive error checking forbuilderattribute and derive macrosThis file contains a well-structured set of compiler error messages that test various aspects of the
builderattribute and its interaction with derive macros. The error messages consistently and correctly identify:
- Incorrect attribute syntax
- Missing
Cloneimplementations- Missing
Debugimplementations- Errors in derive macro usage within the
builderattributeThese error messages form an effective test suite to ensure the compiler catches and reports various misuses of the
builderattribute and related features. This helps maintain the robustness and reliability of thebonlibrary's implementation.website/reference/builder.md (1)
5-10: Excellent restructuring and clarification of the document!The updated title and introduction provide a clearer overview of the builder macros. The categorization of attributes into item and member attributes helps users understand the different levels at which these can be applied. This restructuring will significantly improve the readability and usability of the documentation.
bon/tests/integration/ui/compile_fail/attr_transparent.stderr (6)
1-6: Error message accurately reflects misuse of#[builder(transparent)]attributeThe error message correctly identifies that
#[builder(transparent)]can only be applied to members of typeOption<T>.
7-12: Error message correctly flags incompatible attributesstart_fnandtransparentThe error message appropriately indicates that
start_fncannot be specified together withtransparent.
13-18: Error message correctly flags incompatible attributesfinish_fnandtransparentThe error message appropriately indicates that
finish_fncannot be specified together withtransparent.
19-24: Error message correctly flags incompatible attributesskipandtransparentThe error message appropriately indicates that
skipcannot be specified together withtransparent.
25-38: Error message accurately identifies missing methodmaybe_memberThe error message correctly states that
ValidBuilderdoes not have a method namedmaybe_memberand suggestsmemberas a possible correct method name.
39-59: Error message correctly identifies unset required memberUnset<arg1>The error message accurately indicates that the member
Unset<arg1>was not set before callingbuild(), and explains the required trait bounds.
|
Finally, this mega-PR is ready to merge. There are two low-effort features that I'd like to include in the upcomming release and the remaining work is left only for the documentation and a blog post. More details on the remaining work are in the "TODO (separate PR)" section of the PR description. I'll try to tackle that quickly. I'll do that in separate followup PRs. Then I'll cut off a |
Closes #21
For anyone who wants to try out the changes here, you can do so using the following line in
Cargo.toml(UPD this feature is already in master):Example of how this feature works (it already compiles from this branch and existing tests in
bonpass):According to my research any slightest usage of associated type projections in the generated code considerably increases the compile time. In the most minimal approach where assoc types are used as trait bounds on setter methods and they aren't used inside of the struct to change its shape the compilation performance of
frankenstein(~320 structs) increases from 19 to 20 seconds (from scratch). In the worst case, when assoc types are used to define the type of optional members in the struct the compile time increases to 26 seconds 💥.I think the ideal balance between safety and compilation performance is such that we:
Option<T>to store all named members, including required. We don't use theState::{Member}assoc types in the struct declaration itself to avoid compilation time overhead.Introduce a new private method(this doesn't bring noticeable compile perf. improvements)transition<NewState>(self) -> TBuilder<NewState>that changes thePhantomDatatype that holds the type state. It's technically an unsafe method (see the following), but it doesn't actually use any unsafe operations. This method will be used by all the generated setters and should reduce the amount of tokens generated considerably (especially for builders with tens of members).unwrap_uncheckedfor required members in thefinish_fnmethod because we know they are always initialized according to the type state in that method. This is needed because the safe "unwrap" isn't optimized-out by the compiler according to the benchmarks and generated assembly.#[builder(setters = {name})] override#[builder(setters)]mutable:overwritable,no_overwrite_protection...requiredoroptionalto the default docscrateorselfinpub(in ...)onshould behave like a match and short circuit on the first match (?)Optionalrender in a box in docs?TODO (separate PR)
#[builder(on(..., transparent))]support#[builder(crate = ...)]override supportexpose_positional_fn. Add#[builder(separate)]instead.IsSetandIsUnsettratis (cleanup lib.rs)IsSettraitdocs { ... }overrides attributes added in Add docs override attributes for various generated items #139vis(...)configs and how the 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)ReferencesectionNot included in this release
optional()and!operators to type patterns expressions. Take a look atrustc_on_unimplementedmatching syntax. (Need to decide on the behavior here: should there be short circuit or not?).IsCompletetrait for the state which may serve as a replacement.#[builder(field)]to add custom fields to the builder (restriction: custom field names must not collide with other names of members).#[builder(getter)]to derive getters for every member (start_fn and named).#[doc(hidden)],#[doc = include_str!("...")]todocs {}overrides and docs copied from the fields/argsChangelog
Changed the visibility syntax fromDecided not to do this. I thinkvis = "..."tovis(...). Usevis(pub(self))for private visibility.= ...syntax reads better.builder_typeandfinish_fn#[builder(with = closure)]syntax to customize setters#[builder(transparent)]forOptionfields to opt out from their special handling which makes such fields optional#[builder(state_mod)]to configure the builder type state API module#[builder(overwritable)]and#[builder(on(..., overwritable)]to make it possible to call setters multiple times for the same member#[builder(setters)]to fine-tune the setters names, visibility and docs#[builder(derive(Clone/Debug(bounds(...))]to allow overriding trait bounds on theClone/Debugimpl block for builder{member}(T)setter mention themaybe_{member}(Option<T>)setter and vice versa.__prefixes for generic types and lifetimes from internal symbols. Instead the prefixes added only if the macro detects a name collision.e.g. #[builder()]or#[builder]with no parameters on a member.#[bon(...)]attribute. This attribute will accept some parameters in future releasesbuilder_type,finish_fn,start_fnandonattributes syntax. Only parentheses are accepted e.g.#[builder(finish_fn(...))]or#[builder(on(...))]. This no longer works:#[builder(finish_fn[...])]or#[builder(on{...})]Points for discussion
#[builder(builder_mod(vis = "pub"))]#[builder(transparent)]to disable the special handling for members of typeOption<T>. Alternatives:#[builder(opaque)],#[builder(blindfold)].Optionalbe also rendered in the box, or should it intentionally stand out less)?Summary by CodeRabbit
New Features
README.mdfor the compilation benchmarks, detailing dependencies and usage instructions.Bug Fixes
Documentation
Chores