Skip to content

chore: resolve in-source TODOs#194

Merged
davydog187 merged 3 commits into
mainfrom
chore/source-todos
May 6, 2026
Merged

chore: resolve in-source TODOs#194
davydog187 merged 3 commits into
mainfrom
chore/source-todos

Conversation

@davydog187
Copy link
Copy Markdown
Contributor

Resolve in-source TODOs (compiler error handling, stacktrace formatting, load reader chunks)

Plan: .agents/plans/A11-source-todos.md
Closes #172

Goal

Clean up three in-source TODOs that have been lingering since the rewrite,
now that the prerequisite features are in place.

Success criteria

  • mix test passes (1357 → 1369, no regressions)
  • All three TODOs resolved or removed with a comment explaining why
    they're being intentionally deferred.
  • grep -n "TODO" lib/ shows three fewer hits in the targeted files
    (in fact lib/ now has 0 TODOs).

Changes

 lib/lua/compiler.ex                     |   8 +++-
 lib/lua/compiler_exception.ex           |  18 +++-----
 lib/lua/vm/stdlib.ex                    |  79 +++++++++++++++++++++++++-------
 test/language/load_test.exs             | 102 ++++++++++++++++++++++++++++++++++++++++++
 test/lua/compiler_exception_test.exs    |  56 +++++++++++++++++++++++ (new)

lib/lua/compiler.ex — TODO #1

The compiler still raises rather than returning {:error, _}, so the
TODO's premise ("now that the compiler can return errors") isn't yet
true. The dead case branch in compile!/2 has been replaced with a
clearer forward note explaining what would need to change first
(threading results through codegen instead of raising).

lib/lua/compiler_exception.ex — TODO #2

Compile-time errors don't have a runtime stack — they happen before any
code runs. The rich source context (line, column, pointer to the
offending token) is already produced upstream by
Lua.Parser.Error.format/2, and CompilerException.message/1 already
forwards it under the "Failed to compile Lua!" header. The stale
"Re-add stacktrace formatting" comment has been replaced with a note
explaining this. New tests in test/lua/compiler_exception_test.exs
assert the formatted output includes line, column, and the offending
source line.

lib/lua/vm/stdlib.ex — TODO #3

Implements reader-function load(reader) per Lua 5.3 spec:

  • Accept {:lua_closure, ...} and {:native_func, ...} callees.
  • Call the reader repeatedly with no args; accumulate string pieces.
  • End the chunk on nil, "", or no return value.
  • Return (nil, error_message) if the reader returns a non-string
    non-nil value.
  • Reject non-string non-function arguments with a clear error.
  • Pass the concatenated source through the existing parse+compile path.

Seven new tests in test/language/load_test.exs cover concatenation,
all three end-of-chunk signals, non-string returns, syntax errors in
reader-supplied source, and rejection of non-string non-function args.

Discoveries

  • Test file location. Plan asked for tests in
    test/lua/vm/stdlib_load_test.exs, but the existing convention is
    test/language/load_test.exs. I added the reader-function tests there
    to keep with the codebase convention; new compiler-exception tests
    live in test/lua/compiler_exception_test.exs (no prior file
    existed).
  • TODO Pull in code #1 stays partially deferred but with a clearer note. The
    plan anticipated this case: "If no, document why and leave the TODO
    in place with a clearer explanation." The TODO comment is gone but
    replaced with a forward note about the codegen refactor required to
    make the error path real.
  • TODO Add CI #2 already mostly worked. The parser produces beautifully
    formatted errors with source context already; the missing piece in
    the original TODO was a runtime stacktrace, which doesn't apply at
    compile time. The remaining work was clarifying that and adding
    tests asserting the format includes source location.
  • lib/ now has zero TODOs. Outpaced the success criterion of
    "three fewer hits in the targeted files."

Verification

mix format
mix compile --warnings-as-errors
mix test
mix test --only lua53

All green:

Generated lua app
52 doctests, 45 properties, 1369 tests, 0 failures, 36 skipped
29 tests, 0 failures, 25 skipped (1437 excluded)   # --only lua53

Out of scope (intentional)

  • Other TODOs not on this list (none remain in lib/ after this PR).
  • Restructuring how errors flow through the compiler — that's the
    prerequisite for the original TODO Pull in code #1 and stays deferred until
    codegen is rewritten to thread results.

davydog187 added 3 commits May 6, 2026 07:47
Implements load() with a reader-function argument (Lua 5.3 spec), wires
compile-error formatting through the parser's source-context output, and
clarifies the compiler.ex error-handling note now that the compiler still
raises rather than returning {:error, _}.

- stdlib.load: accept a reader function; concat pieces until nil/empty
  string; reject non-string non-function args; pass-through to the
  existing parse+compile path.
- CompilerException.message/1: drop the stale 'Re-add stacktrace'
  comment. Compile-time errors don't have a runtime stack; rich source
  context (line, column, pointer) is already produced upstream by
  Lua.Parser.Error.format/2.
- Compiler.compile!/2: replace the dead-branch TODO with a forward note
  explaining the {:error, _} branch is reserved for when codegen is
  converted to thread results instead of raising.

Plan: A11
Closes #172
@davydog187 davydog187 merged commit abb026b into main May 6, 2026
3 checks passed
@davydog187 davydog187 deleted the chore/source-todos branch May 6, 2026 18:05
davydog187 added a commit that referenced this pull request May 6, 2026
- Status: bump date to 2026-05-06 and unit count to 1369/36 skipped
- Polish: strike A11 with shipped-in PR #194 reference
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.

Resolve in-source TODOs (compiler error handling, stacktrace formatting, load reader chunks)

1 participant