fix: fixed augmented assignment 🦾 - #192
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7000d86d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 376f4a1baf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Follow-up: move temporary-slot allocation metadata into the analyserThe current Longer term, the analyser should record the source-local count for the top-level frame and for each function. The compiler can then initialize
The REPL/resume path will need the top-level high-water mark to remain monotonic across analysis batches, while nested functions need their own independent count. This is not blocking for this PR. The current scan is functionally correct, but this would be a worthwhile follow-up refactor. |
376f4a1 to
6e296ed
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e296ed10d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
timfennis
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57789c58a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c2d6efba9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub enum AugmentedAssignmentPlan { | ||
| Unresolved, | ||
| Resolved(Binding), | ||
| } |
There was a problem hiding this comment.
Isn't this just Option<Binding> with extra steps? There is precedent for using Option<Binding>? correct?
1c2d6ef to
3da4d2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3da4d2ea27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| let compiled = match Compiler::compile(expressions.into_iter()) { | ||
| let compiled = match Compiler::compile(expressions.into_iter(), Default::default()) { |
There was a problem hiding this comment.
For any generated valid program containing a function declaration, this empty metadata makes compile_function_decl return missing source-local count; the immediately following Err(_) => return then silently skips both nested-function compilation and VM execution. Take the analyser result and pass its source_local_counts here so the property test continues exercising function-containing programs.
Useful? React with 👍 / 👎.
Follow-ups from the review of #192, stacked on #193. The review's bigger findings (fuzzer metadata wiring) were made obsolete by the temp-layout refactor in #193; these are the pieces that survived. ## Changes - **analyser**: named the `Binding::None` arms in the augmented-assignment plan match (fixes the `match_wildcard_for_single_variants` clippy warning introduced by #192) and extracted the twice-duplicated "index type is an int-range slice" check into `index_type_is_slice`. - **compiler**: `compile_batch` now takes the expression iterator directly instead of forcing every caller to `.collect()` into a `Vec` first. - **manual**: the augmented-assignment page only documented variable targets; added sections for indexed/slice/string targets, the once-each evaluation-order guarantee, and the type-checking rules (in-place operators preserve the target type, fallback operators may widen). All new examples are verified to run. Remaining clippy warnings (`chunk.rs` `use_self`, `only_used_in_recursion`, doc backticks) pre-date this stack on master and are left for a separate cleanup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
The current augmented assignment implementation is messy and incomplete. While implementing augmented assignment for struct fields multiple problems were discovered. This PR attempts to fix these issues by having the analyzer plan the augmented assignment during analysis and generalizing the compilation of augmented assignment to also be applicable to situations like the one below: