Skip to content

Interpret #%variable-reference core form#77

Merged
pmatos merged 1 commit into
mainfrom
issue-13
Jul 1, 2026
Merged

Interpret #%variable-reference core form#77
pmatos merged 1 commit into
mainfrom
issue-13

Conversation

@pmatos

@pmatos pmatos commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add a VariableReference AST value node and parse the three fully-expanded
    forms of the core syntax: (#%variable-reference),
    (#%variable-reference id), and (#%variable-reference (#%top . id)).
  • The interpreter evaluates it to itself — an opaque, self-evaluating value that
    prints as #<variable-reference>, matching Racket v9.2.
  • Wire the new node through the ASTVisitor interface (Interpreter,
    AnalysisFreeVars) and the parser (parseVariableReference, tried in
    parseExpr before parseApplication).

This form appears throughout expander/expander.rktl (always as
(variable-reference-from-unsafe? (#%variable-reference))); previously the
parser had the #%variable-reference token but no production for it, so these
expressions failed to parse.

Fixes #13

Test plan

  • ctest --preset debug — Catch2 + lit/FileCheck, 14/14 passing
  • New unit test Parsing variable references covers all three forms and the
    non-match (plain application) case
  • New integration tests variable-reference{,1,2}.rkt — each prints
    #<variable-reference>
  • Warning-clean build (warnings-as-errors) and clang-format clean

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.82927% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.61%. Comparing base (6922772) to head (e0820ac).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/Parse.cpp 75.00% 12 Missing ⚠️
src/AST.cpp 14.28% 6 Missing ⚠️
src/AnalysisFreeVars.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   74.44%   75.61%   +1.16%     
==========================================
  Files          24       24              
  Lines        2270     2391     +121     
  Branches      311      332      +21     
==========================================
+ Hits         1690     1808     +118     
- Misses        580      583       +3     
Flag Coverage Δ
integration 75.61% <76.82%> (+1.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@pmatos

pmatos commented Jul 1, 2026

Copy link
Copy Markdown
Owner Author

Regarding the codecov coverage report (patch coverage 77.10%, 19 lines missing):

Deferred (diminishing returns) — Of the 19 uncovered lines, the src/AST.cpp (6) lines are the debug-only dump() method and the src/AnalysisFreeVars.cpp (1) line is a no-op visitor not wired into any runtime path — both are intentionally left uncovered here, matching existing repo convention. The src/Parse.cpp (12) lines are the parseVariableReference error-recovery branches; those are genuinely testable, but the three success paths and the non-match case are already covered by unit + integration tests, the codecov/patch check is green, and overall project coverage rose (+0.14%), so covering the malformed-input branches is a low-value follow-up rather than PR-blocking work.

Tracked as #86 (#86).

This assessment was made by two independent AI reviewers (Claude Opus 4.8 and Codex GPT-5.5). If you disagree, please reply and we'll re-evaluate.

Parse the three fully-expanded forms — (#%variable-reference),
(#%variable-reference id) and (#%variable-reference (#%top . id)) — into a
new VariableReference value node that evaluates to itself, an opaque value
printing as #<variable-reference>, matching Racket.
@pmatos pmatos merged commit 29cbfd1 into main Jul 1, 2026
20 checks passed
@pmatos pmatos deleted the issue-13 branch July 1, 2026 20:32

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0820ac3d5

ℹ️ 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".

Comment thread src/Interpreter.cpp
Comment on lines +288 to +290
void Interpreter::visit(ast::VariableReference const &VR) {
LLVM_DEBUG(llvm::dbgs() << "Interpreting VariableReference\n");
Result = std::unique_ptr<ast::ValueNode>(VR.clone());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate referenced ids before returning a variable reference

For the operand forms, the Racket reference says #%variable-reference represents the location of an id that must be bound as a variable (https://docs.racket-lang.org/reference/Locations____variable-reference.html), but this visitor ignores VR.getId() and always succeeds. With input such as (#%variable-reference missing), interpretation now silently returns #<variable-reference> instead of reporting an unbound variable/reference error, so invalid expanded code is accepted.

Useful? React with 👍 / 👎.

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.

Interpret %variable-reference

1 participant