Replace runtime-type quotation with splice-into-source typechecking#706
Conversation
ba6761e to
c4059f2
Compare
|
My intuition is mypy failed with stress test. Checking.. |
c4059f2 to
0375180
Compare
eb8680
left a comment
There was a problem hiding this comment.
Thanks, this seems like a big improvement. There are several heuristics and silent failures that I worry will undermine its correctness, though.
0a36f8a to
9dd1252
Compare
Type-check LLM-generated Callables by splicing them into the Template's real module source -- in place at the template def's position -- and running mypy on the whole module, raising only on diagnostics inside the spliced region. This removes the brittle quotation layer (type_to_ast, signature_to_ast, collect_*, _RenameTransformer) and the runtime/local-type stubbing it depended on, which mishandled common types such as Enum and runtime-created Pydantic models. - mypy_type_check now takes (generated, anchor); the anchor (Template.__default__) is provided by the type_check_anchor effect op, bound per call by Template.__apply__ (so nested synthesis sees the right one) and rebound to None around tool-argument decoding, so the argument path skips the check. - Region-scoped diagnostics: unrelated pre-existing errors elsewhere in the module never block synthesis (no spurious retry burn). - Leaves the template def's decorators untouched (mypy checks the body against its declared return type regardless of decorators); explicit pre-scan for non-nestable constructs (star-import, __future__); source recovery via getsourcelines/file/ linecache (REPL-defined templates); fatal mypy errors are surfaced not swallowed. - Name collisions (#542) are handled structurally by nesting/shadowing, so the AST-rename pass is deleted. - Delete the quotation emitters + their unit tests; add behavior tests covering collision shadowing, the gate, closures, method/static/class templates, Enum/dataclass context, injected-name, non-nestable, and linecache recovery.
9dd1252 to
b3ec9c3
Compare
The scan is provider-independent AST validation, not type-checking; lift it into the decode as a precondition. Keep it gated on the anchor -- the constructs it rejects are illegal only once nested by the splice, and legal on the argument path's module-level exec.
mypy_type_check scraped mypy's human-readable output with a hand-rolled regex (_MYPY_LINE_RE) to pull line numbers and filter to the spliced region -- an undocumented format where a real error line that failed to match was silently dropped, letting bad code pass the gate. Switch to --output=json (documented, one object per diagnostic) and filter on mypy's own severity/line fields. A fatal exit (status >= 2) emits text not JSON, so it's handled before parsing.
…py failure The mypy region was [def-line, end], so it flagged the Template's own signature -- pre-existing source the LLM never generated. For notebook/REPL templates whose recovered source is a single cell missing other cells' imports, the signature's annotations (Literal, Callable) look undefined to mypy, spuriously failing synthesis. Narrow the region to the spliced body only. Also: a mypy exit status >= 2 is a tool failure, not a type error -- raise RuntimeError, and check status before deriving a type-error TypeError from diagnostics.
Drop the redundant os.path.isfile + open branch: linecache.getlines reads real files from disk too (as inspect does), so one path covers real modules and linecache-registered REPL/exec/notebook sources alike. The function stays -- inspect.getsource(inspect.getmodule(fn)) can't replace it, since getmodule/getsource fail for exec- and __main__-defined templates that _recover_module_source handles via the function's co_filename.
Both single-use helpers. _recover_module_source folds into its one caller, collapsing its three return-None paths into the existing skip check. _unwrap is replaced by inspect.unwrap(anchor): staticmethod/classmethod implement __wrapped__ (== __func__), so the stdlib call returns the underlying function identically and drops the hand-rolled isinstance heuristic.
Split mypy_type_check into _splice_into_source (construct the modified module source) and _mypy_check_region (run mypy on a given source), with mypy_type_check as a thin orchestrator -- so the mypy step is decoupled from splicing. Also correct the tool-argument anchor-suppression comments (completions.py, encoding.py): the reason isn't 'no source anchor' (the Template's source is available) but that a tool-arg Callable's contract is the tool parameter's type, not the Template's return type, so the Template is the wrong splice target.
When the anchor's def can't be located in the recovered source, that's source drift (the file changed since the anchor was compiled), not a normal skip condition. Raise RuntimeError with a clear message instead of silently skipping the type check.
…ydantic context decoding
eb8680
left a comment
There was a problem hiding this comment.
Nearly there, just a few small things
| env, template.__signature__.return_annotation, **self.config | ||
| env, | ||
| template.__signature__.return_annotation, | ||
| anchor=template.__default__, |
There was a problem hiding this comment.
I'm not sure template.__default__ will work as anchor for bound-method Templates. You should make sure this case is covered in a test, which you'll probably then need to fix.
There was a problem hiding this comment.
I added an integration test, and it works ok after multiple retries (due to LLM flakiness), also added the replay record.
There was a problem hiding this comment.
The relay buffer gives me this:
┌───────┬────────────────────────────────────────────────────┬────────┐
│ Round │ Generated code │ Valid? │
├───────┼────────────────────────────────────────────────────┼────────┤
│ 0 │ def add(a, b) -> int:\n return a + b<br>\n\nadd │ ✗ │
├───────┼────────────────────────────────────────────────────┼────────┤
│ 1 │ def add(...): ...<br>\n\n# comment\n\nadd │ ✗ │
├───────┼────────────────────────────────────────────────────┼────────┤
│ 2 │ def add(...): ...<br>\n\nadd │ ✗ │
├───────┼────────────────────────────────────────────────────┼────────┤
│ 3 │ def add(a: int, b: int) -> int:\n return a + b │ ✓ │
└───────┴────────────────────────────────────────────────────┴────────┘
The function body is correct every time — that's the part that "looks correct." But rounds 0–2 append a trailing bare add (the function name on its own line, as a "here's the result" hint). That makes the module's last statement a bare Expr, and decode requires it to be a FunctionDef:
Value error, decode() requires the last statement to be a function definition, got Expr
I think this is more of the mypy thing but not sure we should try adding fix in this PR cause
I'm not sure how sound/complete the solution could be, and it still works after multiple retries.
…od integration test type_check(source, lo=None, hi=None) now just runs mypy on a given source, reporting only diagnostics in [lo, hi] (whole file if omitted); splicing is a separate public splice_into_source(generated, anchor), and decode does the two steps explicitly. Removes the mypy_type_check convenience (tests splice + call the op). Error report uses json.dumps(e) per diagnostic and drops the (large) source from the message. Adds a bound-method Template synthesis integration test (replayed) covering the template.__default__ anchor for a bound method.
Closes #696. Addresses #577, #576.
Currently, for synthesis, we are reconstructing a
Template's lexical context as synthetic type stubs. This is brittle and likely unsound.This PR splices the generated code into the Template module, and type checks the whole file. This come with a few changes
mypy_type_checknow parameterized withgenerated(code) andanchor(the Template). The anchor is used to recover its module source,Note, in practice there is a few more tweaks:
from x import *,__future__) are caught by an explicit AST pre-scan, since mypy silently accepts a nested star-import; a fatal mypy error (exit ≥ 2) is surfaced rather than silently passed; unrecoverable source (no file, nolinecache) is skipped with a logged reason.Name collisions (Synthesized Callable Name shouldn't matter #542) are now handled structurally: the generated def is a function-body local that shadows a colliding context name rather than redefining it at module level, so the AST-rename pass is gone. The whole quotation layer is deleted —
type_to_ast,signature_to_ast,collect_variable_declarations,collect_runtime_type_stubs,collect_imports, and_RenameTransformer/_generate_unique_name(net +510 / −1991).Behavior change. Runtime-only / local types, and names with no lexical presence at the splice point, now raise instead of being (partially, fakily) stubbed. The check is sound on annotated generated code; unannotated bodies are checked at their signature only (
--check-untyped-defsis deliberately off — it rejects runnable heterogeneous-container code).Tests.
Enum+ dataclass context (Decoding runtime-only Pydantic models in Callables fails #576), injected-name, non-nestable,linecache-backed recovery (the REPL tool code should be typechecked when possible #690 path), annotation-collision, and end-to-end decode throughEncodable[Callable].mypy effectful/handlers/llm/clean,ruffclean, full LLM suite green.