Skip to content

Remove leftover code and split long compiler functions - #161

Merged
itsfuad merged 1 commit into
mainfrom
fix/clean-code-audit
Oct 9, 2026
Merged

itsfuad merged 1 commit into
mainfrom
fix/clean-code-audit

Conversation

@itsfuad

@itsfuad itsfuad commented Oct 7, 2026

Copy link
Copy Markdown
Member

Summary

  • What changed: Removed code that lost its last caller in earlier redesigns, removed parameters and results nobody uses, merged several duplicated blocks, and split four long functions into named steps.
  • Why this change is needed: A whole-compiler clean-code audit found dead code, repeated logic and very long functions that made the code harder to read and change.
  • Owning phase/package/component: Parser, lexer, binder, resolver, typechecker, place, flow/ownership analysis, THIR, MIR lowering, LLVM backend, diagnostics, LSP, CLI, pkg/toml, pkg/manifest, pkg/distribution, pkg/registry.
  • Scope boundaries / non-goals: No language, diagnostic or output change. Not included: nil-receiver guard cleanup, parser table rewrite, moving ParseIntegerBuiltin or CompilerOp, and the output-ordering issue described under Risks.

Depends on #160; retarget to main after that PR merges.

Production ownership

  • Current non-test production consumer(s) of each new production symbol:
    • (*thir.Match).Arm: analysis/ownership.go, analysis/flow_thir.go.
    • (*lowerer).storeInTemp: 19 cases in mir/expr_lower.go.
    • scanQuoted: three call sites in diagnostics/syntax_highlighter.go.
    • stringHandler(prefixLen, kind): two lexer patterns.
    • handleRequest: hover, definition, completion and rename in the LSP loop.
    • Step functions split out of GenerateLLVMIR, checkForInStmt, workspaceIndex.rebuild and Run: one caller each, the function they came from.
  • Execution path showing where the new behavior is reached in production: No new behavior. The same paths run through the renamed or extracted functions.
  • Existing implementation reused, extended, replaced, or intentionally left unchanged: THIR-based origin resolution in analysis/flow_thir.go remains the only resolver; the old AST-based one in place is removed. peeper remove keeps its own logic in cmd/cli/remove.go.
  • Wrappers, aliases, duplicate paths, compatibility shims, or experimental scaffolding removed: private callableType (merged into CallableTypeFor), place.Binding, typechecker copies of two flow diagnostics, cstringHandler, bindModuleScopeType, bindModuleScopeTypeIfUnset, three copies of the quote scanner, four copies of the LSP request block, the parser context stack.
  • Any new production symbol without a current production consumer: None new. Emitter.printUnderlineIndent, toml.Data.HasSection, toml.Data.EnsureSection and toml.ParseBytes already had no caller; they are kept at maintainer request and now carry a comment saying so.

Design and behavior

  • Previous behavior: Unchanged.
  • New behavior: Unchanged.
  • Key invariants preserved: Operand evaluation order and temp numbering in MIR lowering; LLVM text and value numbering; diagnostic text, codes and labels; zip timestamps (still UTC 1980-01-01); lazy creation of project contexts in the LSP workspace scan.
  • Intentional behavioral changes: None.
  • Error / edge-case handling: reportUnresolved keeps its nil-node and nil-bag guard. tar.TypeRegA was dropped from one case; the Go tar reader already maps it to TypeReg.
  • Why this implementation belongs in this layer rather than another phase/package: Each helper lives beside the code it replaced. Arm sits on thir.Match because its callers moved to THIR.

Validation

gofmt -l cmd internal pkg scripts
(no output)

go build ./... && go vet ./...
(no output)

go test ./...
52 packages ok, 0 failures

go run ./scripts/bundle.go
exit 0

PEEPER_BIN=$PWD/build/bin/peeper go test ./x_test/ -count=1
ok  compiler/x_test

go run mvdan.cc/unparam@latest ./...
(no findings)

go run honnef.co/go/tools/cmd/staticcheck@latest ./...
production code: only printUnderlineIndent (kept on purpose)

git diff --check
(no output)
  • Targeted tests added or updated: cmd/cli tests call GetCommand(nil) instead of the removed installAllDependencies; place tests use the new resolver signature; ServerState.recompile moved to internal/lsp/state_test.go.
  • Existing regression coverage exercised: Full Go suite and all x_test fixtures.
  • Manual verification, if applicable: Built all 288 x_test projects with -keep-gen before and after. 249 .ll files (compared with line order ignored) and all stdout/stderr are identical. Ran peeper remove on a scratch project.
  • Checks not run: Cross-target builds (Linux/386, Windows, macOS) were not run separately; no target-specific code changed.

Diff sanity

  • Unrelated production changes: pkg/distribution/pack.go also turns two if / else if chains into switch. Same behavior.
  • Duplicated logic introduced: None.
  • Dead or unreachable code introduced: None.
  • Temporary/debug code remaining: None.
  • Comments/docs that became stale because of this change: Updated docs/architecture/cli-and-packages.md, frontend.md and semantics-bindings.md. Code-tour.md section 12 still shows the instruction switch inside GenerateLLVMIR; it now lives in emitBlock. The snippet is marked "simplified" and was left as is.

Risks and follow-up

  • Known risks or assumptions: Eight tests in internal/semantics/place/origin_test.go were deleted with the AST-based resolver they covered. Nothing in production called that resolver since 2026-09-21.
  • Behavior intentionally changed: None.
  • Behavior intentionally not changed: analysis/effects.go keeps its own arm loop: it visits every arm with a case, not only the first, which can matter for programs with duplicate arms.
  • Remaining gaps: Found during validation and not fixed here: two identical compiler runs can differ in MIR type numbering and in the order of LLVM declare lines. Content is the same when line order is ignored.
  • Linked issues / follow-up work: None linked.
  • Anything that should block merge: None.

Clean-code pass with no change to compiler behaviour, diagnostic text or
generated output. Output for all 288 x_test projects was compared before
and after: 249 LLVM files and every message are identical.

Remove code that lost its last caller in earlier redesigns:
- parser context stack and owner constants
- AST-based place resolution (Resolve, LocalRoot and helpers) with the
  tests that only covered it, and the Binding wrapper it needed
- typechecker copies of two flow diagnostics, Match.Arm
- unused backend, IR, emitter and CLI helpers and struct fields
- manifest.RemoveDependency (peeper remove does not use it)

Simplify:
- drop parameters and results no caller uses
- merge CallableTypeFor with its private twin
- share one quote scanner in the syntax highlighter, one string lexer
  handler, one temp-store helper in MIR lowering, one arm lookup on
  thir.Match, and one request helper in the LSP loop
- replace deprecated zip and tar calls

Split GenerateLLVMIR, checkForInStmt, workspace rebuild and the LSP Run
loop into named steps by moving code unchanged.

Keep printUnderlineIndent and the unused TOML helpers at maintainer
request, with comments saying so. Move ServerState.recompile into a test
file since only tests call it. Update architecture docs for removed
names.
@itsfuad
itsfuad changed the base branch from fix/mir-typed-nil-validation to main October 9, 2026 11:17
@itsfuad itsfuad closed this Oct 9, 2026
@itsfuad itsfuad reopened this Oct 9, 2026
@itsfuad
itsfuad merged commit ba8ad04 into main Oct 9, 2026
17 checks passed
@itsfuad itsfuad added this to the 0.2 Language Foundations milestone Oct 9, 2026
@itsfuad
itsfuad deleted the fix/clean-code-audit branch October 9, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant