fix: analyzer false positives, MCP cross-file diagnostics, macro expansion, and semantic audit#262
Merged
sam-at-luther merged 26 commits intomainfrom Apr 3, 2026
Merged
Conversation
…analyzers - Add analyzeCond to skip resolving bare 'else'/'true' in cond test position, matching the runtime's special handling (Fixes #258) - Fix analyzeSet to model runtime set semantics: PutGlobal always writes to package scope. In nested scopes, reference the existing global binding instead of creating a spurious local one (Fixes #260)
Replace ScanWorkspaceAllWithConfig with PrescanWorkspace in buildWorkspaceState to populate all config fields (PackageSymbols, PackageImports, DefaultPackage, WorkspaceRefs) — matching the LSP server's buildWorkspaceIndex. Also copy all fields in loadDocument so per-file analysis has full workspace context. Fixes #259
Benchmark Comparison (main baseline vs PR)Click to expand benchstat outputCompared against cached baseline from the latest main push. |
Add MacroExpander interface and EnvMacroExpander that uses a live LEnv to expand user-defined macro calls during static analysis. When an expander is available, the analyzer expands macro calls and walks the expanded AST, resolving symbols introduced by the macro (e.g. lambda parameters from quasiquote templates). Expansion failures fall back gracefully to the existing opaque-macro behavior. Wired into both the LSP server and MCP server — when WithEnv() provides a runtime environment, macro expansion activates automatically. Fixes #261
sam-at-luther
changed the title
… macros QA review findings addressed: - Strengthen expander test to verify expanded AST structure (not just type) - Add reference-based assertions for macro-expanded lambda params - Add negative test: cross-file symbols flagged without workspace_root - Add test: genuinely undefined symbols in expanded code get InsideMacroCall - Add test: expansion error returns nil gracefully - Add test: empty form returns nil - Strengthen set-in-lambda test with global scope and reference assertions Fix: analyzeCall now tries MacroExpander even when the macro isn't in the analysis scope (e.g. macros from the embedder's registry or cross-file). Previously, scope.Lookup had to succeed AND return SymMacro before expansion was attempted.
[P0] Add not-a-macro cache to EnvMacroExpander — avoids repeated env.Get() on every unresolved function call when expander is set. [P1] Fix analyzeSet to look up in root scope only — scope.LookupInPackage walked the full chain and could find let-local shadows, but PutGlobal always targets the package scope. [P1] Add sync.Mutex to EnvMacroExpander — MacroCall mutates shared Runtime state (call stack, package pointer). Serializes concurrent expansion calls from LSP handlers. [P2] Add expansion depth limit (64) to prevent stack overflow on self-expanding macros. [P2] Fix copyright year to 2026 on new files. Add cross-reference comment between isCondDefaultSymbol and lint's isCondDefault.
- Add depth limit test with self-expanding macro (loop-forever) - Add reference assertions to nested macro expansion test - Strengthen set-creates-global test with Kind and negative scope checks - Rename PanicRecovery test to ExpansionErrorGracefulReturn (honest naming)
…notMacro cache test TestAnalyze_MacroExpansion_WhenMacro previously used a defun parameter (x) which resolves without expansion, making the test partially redundant. Changed to use a with-default macro that introduces a let binding — default-val only exists after expansion, proving the expander actually contributes to resolution. Added TestEnvMacroExpander_NotMacroCached to verify that the notMacro negative-lookup cache on EnvMacroExpander is populated after the first call and persists on subsequent calls.
…forms Add three missing cases to the analysis package's `analyzeExpr` switch: - `macrolet`: Creates a ScopeMacrolet scope, registers bindings as SymMacro, skips analyzing macro template bodies (like defmacro), and analyzes the body in the new scope. Fixes false "undefined symbol" for local macros. - `qualified-symbol`: Skips resolving the argument since it is data (a name), not a variable reference. Fixes false positives on bare symbol arguments. - `thread-first`/`thread-last`: Explicit pass-through to analyzeCall with comment explaining symbol resolution is unaffected by threading.
- Add nested macrolet test (inner scope sees outer + inner macros) - Add qualified-symbol extra-args test (args beyond Cells[1] analyzed) - Add thread-last undefined-symbol negative test (matches thread-first) - Sharpen depth limit assertion to check for 'loop-forever' specifically
LintConfig was missing a MacroExpander field, so embedders that boot a full environment (e.g. shirotester) had no way to enable macro expansion through the CLI lint path. The MCP and LSP servers already set it, but the lint package was the missing link.
Collect raw defmacro AST nodes during workspace prescan (MacroDefs field on WorkspacePrescan). Add LoadWorkspaceMacros helper that evals these into a runtime env so EnvMacroExpander can expand them. Wire through all three paths: - LintConfig.Env: embedders just pass their env, BuildAnalysisConfig handles LoadWorkspaceMacros + expander creation automatically - LSP buildWorkspaceIndex: loads workspace macros into s.env - MCP buildWorkspaceState: loads workspace macros into s.env Embedder usage is now one line: set Env on LintConfig and workspace- defined macros like def-case-verification expand correctly.
[P1] Log LoadWorkspaceMacros errors in all callers: - LSP: window/logMessage warnings - MCP: slog.Warn - lint BuildAnalysisConfig: slog.Warn [P1] Add Reset() method on EnvMacroExpander to clear notMacro cache. Document cache lifetime assumption. [P2] Fix package context for workspace macro loading. MacroDef struct pairs each defmacro AST with its source package (from in-package tracking). LoadWorkspaceMacros calls env.InPackage before each eval so macros register in the correct package scope. [P2] Fix misleading cond comment — runtime only special-cases 'else'; 'true' works because it evaluates to a truthy value, not because it's a keyword.
- Add TestEnvMacroExpander_Reset_ClearsCache: proves stale cache prevents expansion, Reset() fixes it (the key mutation-resistant test) - Add TestLoadWorkspaceMacros_MultiplePackages: two macros in different packages in one call, verifies each registers in its own package - Add TestPrescanWorkspace_MacroDefs: integration test verifying prescan collects defmacro ASTs with correct package context from in-package - Rename ErrorIncludesMacroName to ErrorWithNonSymbolName (honest name, verifies <unknown> appears for non-symbol macro names)
Workspace packages (e.g. "acre") don't exist in the boot env — they're defined by workspace code. LoadWorkspaceMacros now calls DefinePackage and UsePackage(lang) before InPackage, so workspace-only packages are created on demand with lang builtins imported. Added TestLoadWorkspaceMacros_PackageAutoCreated to verify this works with a package that is NOT pre-defined in the env.
…cros Workspace macro bodies may reference functions from imported packages (e.g. flatten from utils). LoadWorkspaceMacros now accepts packageImports from prescan and applies use-package for each workspace package's imports before eval'ing its macros. Also ensures imported packages are created via DefinePackage (they may be workspace-only packages). The preparation phase runs once per unique package across all macro defs, then each macro is eval'd in its prepared package context.
Instead of manually reconstructing package state from MacroDef structs and packageImports, collect all preamble forms (in-package, use-package, export, defmacro) from workspace files in source order and eval them sequentially. This mirrors the runtime's (load) behavior — package context, imports, and macro definitions build up naturally. Changes: - WorkspacePrescan.Preamble replaces MacroDefs (flat []*LVal in order) - LoadWorkspaceMacros takes []*LVal, saves/restores env package - evalPreambleForm auto-creates workspace packages + imports lang - Drop MacroDef struct, preparePackage, packageImports parameter - All callers simplified: just pass prescan.Preamble - Tests rewritten to use preamble sequences (in-package + defmacro) - New tests: PackageRestored, UsePackageImports
ExpandMacro now accepts a pkg parameter — the analyzer's currentPkg. The env is temporarily switched to this package before env.Get so that unqualified symbol resolution matches the file's package scope, the same way the runtime resolves symbols during eval. This fixes workspace macros defined in non-default packages (e.g. acre) not being found by the expander, which was in the user package after LoadWorkspaceMacros restored it. The notMacro cache key is now package-qualified (pkg\x00name) since the same symbol name may be a macro in one package but not another.
Macros that call workspace-defined functions (e.g. flatten) failed during expansion because defun forms weren't loaded into the env. defun is safe to eval — it captures a lambda without evaluating the body, creating only a package-scope binding with no side effects. Added end-to-end test: workspace defun + defmacro that calls it, verified expansion resolves lambda params correctly.
At runtime, (load-file "template.lisp") inherits the caller's package. Files without in-package should have their definitions placed in the workspace's DefaultPackage (from main.lisp), not "user". PrescanWorkspace now prepends a synthetic (in-package 'defaultPkg) to bare files' preambles before aggregation, so LoadWorkspaceMacros places their defun/defmacro forms in the correct package. Also confirmed recursive defun works correctly in preamble loading — defun binds the name before the body executes, so self-references resolve at call time. Added test for both cases.
At runtime, (load-file "bare.lisp") inherits the caller's package at the call site. If main.lisp switches packages between load-file calls, each bare file gets a different package. Use the prescan's loadTree (which already tracks per-file package context from load-file call sites) instead of the global defaultPkg. Falls back to defaultPkg for files not in the load tree. Added tests: BareFileInheritsLoadContext (single package) and BareFileInheritsPackageSwitch (package changes between load-file calls).
Preamble forms were aggregated in filepath.Walk order (alphabetical), but the runtime loads files in the order specified by load-file calls in main.lisp. This means a helper function in a.lisp could be eval'd after the macro in b.lisp that depends on it. buildLoadTree now returns the DFS visit order alongside the package map. PrescanWorkspace uses this to emit preamble forms in load order: first files from the load tree (in DFS traversal order), then files not in the load tree. This ensures macros see all definitions in the same order as the runtime.
- Add set to preamble forms — bare template files using (set 'name ...) now load into the correct package, matching runtime behavior - Fix stale LoadWorkspaceMacros docstring to list all 6 preamble forms - Fix stale scanFileFull preamble comment - Add TestLoadWorkspaceMacros_LoadOrderMatters — proves DFS order matters (helpers loaded before macros that call them) - Add TestLoadWorkspaceMacros_SetInBareFile — proves set globals from bare template files land in the inherited package - Update TestPrescanWorkspace_Preamble to verify set is collected
The existing DefaultPackage remapping already handles set definitions in bare template files — they get remapped from user to the load-tree's package. Added end-to-end test: prescan a workspace with a bare template.lisp containing (set 'default-template ...), then analyze a file in myapp that references default-template. Confirms it resolves.
…file The shouldRemap guard required loadTree to be non-empty, which meant bare files not referenced by load-file in main.lisp kept their definitions in user instead of DefaultPackage. This caused set globals in template files to be invisible to files in the workspace's main package. Two fixes: - Remove len(loadTree) > 0 guard — remap whenever DefaultPackage is set - Bare files not in the load tree fall back to DefaultPackage instead of being skipped entirely Added test: bare template.lisp with set, NOT loaded via load-file, verifies it remaps to myapp and resolves cross-file.
…tion
Expanded the shouldRemap block comment to explain why DefaultPackage
is the workspace's "real" default package, and the three rules:
(1) bare files in load tree → load-tree package context
(2) bare files NOT in load tree → DefaultPackage (e.g. template files
loaded conditionally inside function bodies)
(3) non-bare files → only defs before first in-package
sam-at-luther
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 4 issues + semantic audit gaps + workspace macro loading for embedders.
Issue fixes
elseincondtest position #258:undefined-symbolfalse positive for bareelse/trueincondtest position — addedanalyzeCondworkspace_root— alignedbuildWorkspaceStatewith LSP'sbuildWorkspaceIndexsetwith quoted symbol inside lambda produces falseunused-variablewarning #260:set 'xinside lambda produces falseunused-variable—analyzeSetnow modelsPutGlobalsemanticsMacroExpanderinterface,EnvMacroExpander, preamble-based workspace loadingSemantic audit
macroletscope handling (newScopeMacrolet)qualified-symbolargument skipped (data, not code)thread-first/thread-lastexplicit dispatchWorkspace macro loading
in-package,use-package,export,defmacro,defun,set) collected per file in source orderLoadWorkspaceMacrosreplays preamble into env, mirroring runtime's(load)behaviorbuildLoadTreeensures definitions precede macros that use themDefaultPackageExpandMacro(form, pkg)resolves macros in the file's package contextsync.Mutex+notMacrocache with package-qualified keysEmbedder API
Test plan
go test ./...— all passmake static-checks— 0 issuesFixes #258, Fixes #259, Fixes #260, Fixes #261