Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds full Elixir-style with: defines Changes
Sequence DiagramsequenceDiagram
participant Source as "Elixir source"
participant Transformer as "Transformer"
participant IR as "IR.With"
participant Encoder as "Encoder"
participant JS as "Generated JS"
participant Interpreter as "Interpreter.with"
participant Context as "Context"
Source->>Transformer: parse & transform `with` form
Transformer->>IR: build `IR.With` (clauses, body, else_clauses)
IR->>Encoder: request JS encoding
Encoder->>JS: emit `Interpreter.with(body, clauses, else_clauses, context)`
JS->>Interpreter: invoke `with(...)`
Interpreter->>Context: clone context per clause
Interpreter->>Interpreter: evaluate pattern match & guards
alt clause matches and guards pass
Interpreter->>Context: merge bindings
Interpreter->>Interpreter: evaluate `body(context)`
else clause fails or guards fail
Interpreter->>Interpreter: evaluate `else_clauses` or call `raiseWithClauseError`/`errorCallback`
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/hologram/compiler/ir.ex`:
- Around line 390-396: The IR.With struct's type spec currently sets body:
IR.Block.t(), but the transformer (transform/1) can return any IR.t() (e.g., for
single-expression do-blocks), so update the `@type` t for %IR.With{} to use body:
IR.t() instead of IR.Block.t(); change the spec in the IR.With definition (the
defstruct and `@type` for IR.With) to body: IR.t() to match transformer output and
avoid Dialyzer mismatches.
In `@test/javascript/interpreter_test.mjs`:
- Around line 6934-6962: The test's else clause body references an undefined
identifier expected which would raise a ReferenceError instead of the intended
WithClauseError; update the else clause body in the Interpreter.with call to
return a literal (e.g., a literal atom/object) or define a local fallback
variable and use that instead of expected so the test asserts the
WithClauseError properly when no else clause matches (locate the
Interpreter.with invocation and the else clause body in this test and replace
expected with a defined literal/fallback).
- Around line 6905-6933: The test currently exercises match-failure because the
first clause in Interpreter.with uses match: Type.atom("error") against an :ok
value; change that clause to match: Type.atom("ok") and make the guard express
equality to :no so the clause matches but the guard fails (e.g. replace
Erlang["/=/2"](context.vars.b, Type.atom("no")) with a positive equality check
such as Erlang["==/2"](context.vars.b, Type.atom("no"))), ensuring the
Interpreter.with call (the clause with match/guards/body) now tests
guard-failure rather than match-failure.
🧹 Nitpick comments (2)
assets/js/interpreter.mjs (1)
212-213: Consider using??(nullish coalescing) instead of||for the default.
||would also catch any other falsy value (e.g.,0,"",false). WhileerrorCallbackis realistically only ever a function orundefined, using??better communicates intent:- errorCallback = errorCallback || Interpreter.raiseCaseClauseError; + errorCallback = errorCallback ?? Interpreter.raiseCaseClauseError;lib/hologram/compiler/transformer.ex (1)
445-472: Avoid O(n²) clause accumulation inwithtransformation.
acc.clauses ++ [item]grows quadratically. Prepend and reverse once to keep it linear.♻️ Suggested refactor
- Enum.reduce( - parts, - initial_acc, - fn - do_and_else, acc when is_list(do_and_else) -> - do_part = - do_and_else - |> Keyword.get(:do) - |> transform(context) - - else_part = - do_and_else - |> Keyword.get(:else, []) - |> Enum.map(&transform(&1, context)) - - %{acc | body: do_part, else_clauses: else_part} - - clause, acc -> - %{acc | clauses: acc.clauses ++ [transform(clause, context)]} - end - ) + result = + Enum.reduce( + parts, + initial_acc, + fn + do_and_else, acc when is_list(do_and_else) -> + do_part = + do_and_else + |> Keyword.get(:do) + |> transform(context) + + else_part = + do_and_else + |> Keyword.get(:else, []) + |> Enum.map(&transform(&1, context)) + + %{acc | body: do_part, else_clauses: else_part} + + clause, acc -> + %{acc | clauses: [transform(clause, context) | acc.clauses]} + end + ) + + %{result | clauses: Enum.reverse(result.clauses)}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/hologram/compiler/transformer.ex`:
- Around line 936-952: The with-clause transformer is treating the left side of
`<-` as an expression instead of a pattern because transform/2 is called without
pattern context; update transform_with_clause to set pattern?: true in the
context when transforming the pattern part of a `<-` match (i.e., when
transforming the match/pattern node inside the clause). Specifically, in
transform_with_clause (the clause-handling clause where you currently do %{acc |
clauses: [transform(clause, context) | acc.clauses]}), decompose the clause into
its match and expression parts and call transform(match, Map.put(context,
:pattern?, true)) (or otherwise set context.pattern? = true) before transforming
the RHS, so struct patterns are emitted as pattern-matching IR rather than
__struct__/1 constructors.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/hologram/compiler/transformer.ex`:
- Around line 974-976: The transform_with_clause/3 currently prepends raw
transformed nodes for bare with-clauses which breaks the interpreter that
expects IR.Clause structs; change transform_with_clause to check the result of
transform(clause, context) and if it is not an %IR.Clause{} wrap it in an
%IR.Clause{match: transformed_node, guards: [], body: fn _ctx ->
transformed_node end} (or equivalent body function that returns the node in the
interpreter's expected shape), then prepend that clause into acc.clauses; keep
using transform_with_clause, IR.Clause, and transform to locate the code to
modify.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
bartblast
left a comment
bartblast
left a comment
There was a problem hiding this comment.
Hey @prehnRA, thanks for picking this up! Great to see with getting implemented. I do have a number of concerns that need to be addressed before we can merge though. Since with is a special form - one of the core building blocks of the language that tons of Elixir code relies on - we really need to nail the implementation and match Elixir's semantics exactly. It has quite a few behavioral nuances (hexdocs) and getting any of them wrong would cause subtle, hard-to-debug issues for users.
Bug
No-else-clause match failure raises WithClauseError instead of returning the non-matched value. When there's no else block and a <- match fails, Elixir returns the non-matched value directly:
with {:ok, x} <- :error do
x
end
# => :error (returned directly, no exception)The current implementation always routes through Interpreter.case(condition, elseClauses, ...). When elseClauses is [], this falls through to raiseWithClauseError(). Fix: when elseClauses is empty, return condition directly.
Design concerns
-
IR.Clause.bodysemantic inversion - ReusingIR.Clauseforwith/<-clauses creates a confusing inversion: incase/->clausesbodyis the result (called after match succeeds), but inwith/<-clausesbodyis the input (evaluated before matching). The same field means the opposite thing depending on context. Sincewithclauses can also include plain expression clauses (e.g.,double_width = width * 2) intermixed with<-clauses - and they must stay in a single ordered list - we'd need a dedicated struct (e.g.,IR.WithClausewith anexpressionfield) for the<-clauses, while plain expression clauses could use a separate type. Both would coexist in theclauseslist. -
case()coupling - Adding anerrorCallbackparameter tocase()makes it serve double duty. Let's extract the matching loop into a private helper that bothcase()andwith()call independently, keeping their error semantics separate. -
Variable scoping in
else- In Elixir, variables bound inwithclauses are not accessible in theelseblock (it's a compile error). Currently, when the Nth clause fails, thecontextpassed to the else branch already contains bindings from clauses 1..N-1. In practice this doesn't blow up because the Elixir compiler prevents theelseAST from referencing those variables - but it means the JS runtime is silently leaking bindings that shouldn't exist. This needs to be handled properly: pass the original context (before anywithclause bindings) to the else branch, so the scoping semantics actually match Elixir's.
Test coverage
The current tests are too basic - we need robust, granular tests for each function changed or introduced.
Transformer tests - We need both "AST from source code" and "AST from BEAM file" variants, since the AST structure can differ between the two. The current test only covers source code. Each clause type needs its own test:
<-clause without guards<-clause with guards- Plain expression clause (e.g.,
x = val, bare function call) doblock (single and multi-expression)elseblock with multiple clauses- No
elseblock - etc.
Encoder tests - Same clause variants as above to verify correct JS generation for each.
JS interpreter tests - Need dedicated tests for each with behavior from the hexdocs:
- Match failure without
else→ returns non-matched value (this would have caught the bug above) - Guard failure without
else→ returns non-matched value - Multiple chained clauses with variable propagation (variable bound in clause 1 used in clause 2)
- Plain (non-
<-) expressions mixed with<-clauses - Multi-expression
dobody elseclause matching the correct failed value- etc.
Down the road we'll also need browser-level tests that double as Elixir/JS consistency tests, but that can come once the core implementation is solid.
|
@bartblast Perfect. Thanks for the detailed feedback. It all makes sense to me. I'm mostly offline for the next few days, but I'll pick it back up later this week or next one. |
|
Update: I've been working on this and have almost completed the requested updates-- fixed the bug, made design changes, added a bunch of Elixir tests. I'm in progress adding more JS tests, and then I'll clean everything up and push updates. @bartblast How would you prefer the plain clauses in the with be modeled? Option A: %WithClause{
match: %MatchPlaceholder{},
guards: [],
expression: ... # whatever the IR is for the plain expression itself
}Option B: %PlainWithClause{
expression: ... # the IR for the plain expression
}Option C: directly use the IR for the plain expression. Pros / Cons (in my view):
Thoughts? |
2 participants
Addresses #690
Summary by CodeRabbit
New Features
Bug Fixes
Tests