Better expression ergonomics#5
Merged
Merged
Conversation
…it of ParselyWrite
the current code shows a half-completed implementation of moving generics out of ParselyWrite's type and into associated types. Then ParselyWrite can be used as a trait bound, which is useful when trying to coerce types correctly when writing (e.g. the 'map' ambiguity problem). Doing this requires an additional type (WriteAdaptor) which doesn't work in its current form: instead of a single impl defined in parsely we'd need to generate impls dynamically for every type otherwise we run into "can't-define-foreign-trait-on-foreign-type" problems when we generate impls in a calling crate.
instead: it looks like this accomplishes enabling ParselyWrite as a trait bound but without requiring the 'WriteAdaptor' type to still enable a generic buffer type as opposed to a concrete one. I still think the 'Ctx' type might work best as an associated type and we don't need generics there, so need to try that.
around expressions that may or may not return a ParselyResult. there's still cleanup to do (arguably the ParselyRead trait should be tweaked to match ParselyWrite now)
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the synchronization and mapping handling for Parsely traits while also cleaning up codegen and documentation. Key changes include:
- Renaming sync-related attributes and refactoring StateSync to use an associated type.
- Enhancing expression ergonomics by allowing mapping closures with implicit result handling.
- Updating generated code in both read and write implementations for better type inference and consistency.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ui/pass/sync.rs | Renamed attribute from sync_func to sync_expr and updated comment. |
| tests/ui/pass/map.rs | Updated mapping attributes for ParselyRead/ParselyWrite and associated tests. |
| impl/src/parsely_write.rs | Refactored StateSync and ParselyWrite traits to use associated types. |
| impl/src/model_types.rs | Introduced MapExpr and Assertion types and updated context handling. |
| impl/src/code_gen/gen_write.rs | Adjusted code generation for sync expressions and mapping closures. |
| impl/src/code_gen/gen_read.rs | Removed obsolete map read helper and integrated new map expression generation. |
| Notes.md | Updated discussion and rationale regarding sync and mapping ergonomics. |
| let field_name = f.ident.as_ref().expect("Field has a name"); | ||
| let field_name_string = field_name.to_string(); | ||
| if let Some(ref sync_func) = f.sync_func { | ||
| if let Some(ref sync_expr) = f.sync_expr { |
There was a problem hiding this comment.
Ensure that sync_expr expressions consistently return a type that is convertible via into_parsely_result to avoid unexpected type coercion failures during synchronization.
| syn::parse2(quote! { | ||
| (#e).into_parsely_result().with_context(|| format!("{}: expression {}", #context, #idx))? | ||
| }) | ||
| .unwrap() |
There was a problem hiding this comment.
Consider handling potential parse errors instead of using unwrap in Context::expressions() to improve robustness in error reporting.
Suggested change
| .unwrap() | |
| .map_err(|e| darling::Error::custom(format!("Error parsing expression {}: {}", idx, e))) |
bbaldino
force-pushed
the
parsely_write_trait_bound
branch
2 times, most recently
from
d9befc7 to
a282514
Compare
bbaldino
force-pushed
the
parsely_write_trait_bound
branch
from
a282514 to
5cd49b3
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a bunch of code that all sorta bled together. Some of it could probably be split off but, here we are.
First changes were to change how
StateSyncis defined: theCtxvar was moved to an associated type andStateSyncis now a required supertrait ofParselyWrite. The idea here was to try and eliminate the possibility of a 'forgotten' sync call.Then I got into the idea of making expressions that are passed in attributes more ergonomic. Previously, when using something like a closer, the return type needed to be explicitly state and the result had to be a
ParselyResult. This makes the annotations feel a lot more verbose. I want to be able to go from:#[parsely_read(map = "|v: u8| -> ParselyResult<String> { Ok(v.to_string()) }")]to
#[parsely_read(map = "|v: u8| { v.to_string() }")]while also allowing
(where v.parse::() returns a
Resulttype) all while keeping things terse. This sent me down a whole rabbit hole that resulted in refactoring the definition of theParselyWritetrait to shift around some generics, but so far it does appear to be working as expected.I also cleaned up some of the codegen code. It's in better shape but lots of work still necessary there.