Skip to content

Task 2.4.4 blocked: Prerequisites (bytecode infrastructure) not implemented - #560

Closed
rogeralsing with Copilot wants to merge 1 commit into
mainfrom
copilot/remove-ast-fields-instructions
Closed

Task 2.4.4 blocked: Prerequisites (bytecode infrastructure) not implemented#560
rogeralsing with Copilot wants to merge 1 commit into
mainfrom
copilot/remove-ast-fields-instructions

Conversation

Copilot AI commented Jan 10, 2026

Copy link
Copy Markdown
Contributor

Task 2.4.4 requires removing AST fields from IR instructions and replacing them with bytecode representations. Analysis shows this cannot proceed—the prerequisite bytecode infrastructure doesn't exist.

Current State

All IR instructions use AST fields exclusively:

// Current (Instructions.cs:377)
internal sealed record BranchInstruction(
    ExpressionNode Condition,
    int ConsequentIndex,
    int AlternateIndex
) : ExecutionInstruction(InstructionKind.Branch, -1);

// Expected after prerequisites (per issue)
internal sealed record BranchInstruction(
    int TrueTarget,
    int FalseTarget,
    ImmutableArray<ExpressionOp> ConditionOps  // doesn't exist yet
) : ExecutionInstruction;

Blocking Issues

  • No bytecode types exist (ExpressionOp referenced in issue description is not defined)
  • Handlers still evaluate AST: instruction.Condition.EvaluateExpression(environment, context)
  • Emitters create instructions with AST fields only
  • 15+ instructions affected (BranchInstruction, ThrowInstruction, ReturnInstruction, etc.)

Prerequisites Required

  1. Task 2.4.1: Design expression bytecode format
  2. Task 2.4.2: Update handlers to interpret bytecode instead of evaluating AST
  3. Task 2.4.3: Update emitters to generate bytecode

Recommendation

Close this PR and create/prioritize issues for Tasks 2.4.1-2.4.3. This cleanup task is correctly scoped as "simple" but depends on substantial prerequisite work that represents the actual complexity.

Detailed analysis: /tmp/task-2.4.4-analysis.md

Original prompt

This section details on the original issue you should resolve

<issue_title>Task 2.4.4: Remove AST fields from instructions</issue_title>
<issue_description>Parent: #414

Goal

Remove deprecated AST fields from IR instructions (cleanup).

Prerequisites

  • All handlers updated (Task 2.4.2)
  • All emitters updated (Task 2.4.3)
  • Tests passing with bytecode-only

Approach

// Remove AST field
record BranchInstruction(
    int TrueTarget,
    int FalseTarget,
    ImmutableArray<ExpressionOp> ConditionOps  // now required, not optional
) : ExecutionInstruction;

Files

  • src/Asynkron.JsEngine/Execution/Instructions/Instructions.cs

Complexity: Simple

  • Remove nullable AST fields
  • Make bytecode fields required
  • Final cleanup step</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@coderabbitai

coderabbitai Bot commented Jan 10, 2026

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI changed the title [WIP] Remove deprecated AST fields from IR instructions Task 2.4.4 blocked: Prerequisites (bytecode infrastructure) not implemented Jan 10, 2026
Copilot AI requested a review from rogeralsing January 10, 2026 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task 2.4.4: Remove AST fields from instructions

2 participants