Skip to content

Isolate packages.Load behind internal/load - #145

Closed
crhntr wants to merge 13 commits into
mainfrom
refactor/load-pacakges
Closed

crhntr wants to merge 13 commits into
mainfrom
refactor/load-pacakges

Conversation

@crhntr

@crhntr crhntr commented Sep 12, 2026

Copy link
Copy Markdown
Member

internal/load is now the only caller of packages.Load. Route resolution, generation and analysis take plain go/types values and template sets (muxt.Source), so they can be tested with inputs built in memory.

  • internal/typestest type checks source against stub standard library packages in microseconds.
  • Snapshot tests (internal/{generate,analysis}/testdata/*.txtar, -update to rewrite): generate coverage 4% → 92%, analysis 10% → 56%, each suite well under a second.
  • Path parameter types are resolved before generation; generation no longer mutates the routes it reads.
  • Generated files no longer rely on goimports to find imports (it was adding fmt/errors and walking the module cache). Output is unchanged.

cmd/muxt integration tests are untouched and pass. Gremlins on internal/generate: 89% efficacy in 55s (needs --timeout-coefficient 100).

🤖 Generated with Claude Code

https://claude.ai/code/session_01UjkpA4fvZ7xQAsY65Beprp

Loading a package runs the go command, and it was the one slow thing
asteval did: everything else in it reads trees and types already in
hand. Sharing a package meant that anything wanting a string literal
evaluated, or a template parsed, linked go/packages and sat one import
away from calling it.

internal/load now holds what needs the go command, or its result: the
package load, the parse errors it recovered from, finding a package by
directory or path, evaluating a templates variable, finding a receiver
type, and the diagnostics for a lookup that came up empty. The code moves
unchanged; the names lose the prefix the package now says
(load.Packages, load.Templates), and asteval.Templates, which only its
own test called, is folded into load.HTMLTemplates.

This is the first step to a seam: the next ones take the package list
out of route resolution and generation, so they can be handed types
built in memory.

Assisted-by: Claude:claude-opus-5 gofumpt
Every stage below the command line took []*packages.Package and dug
through it for what it needed: a *types.Package to find the templates
package's functions, a FileSet for positions, and a search through the
imports for net/http or encoding. So nothing could run without a module
on disk and the go command, and every question about how a route
resolves, or what a handler looks like, was asked of the 80 second
integration suite.

The stages now take plain values:

- muxt.Package is a FileSet, the package's types, and a lookup for the
  other packages it needs. muxt.Templates is a templates variable's set,
  its Funcs functions, and where each template name was written.
  muxt.Source bundles them with the receiver type, in the order the
  variables were named.

- load.Source builds a muxt.Source from a load, and load.AnalysisSource
  what the type-checking commands need. That is where a go/packages
  result stops. A variable that fails to load carries its error, so a
  command still reports it at the point it reaches that variable and
  errors keep the order they had.

- generate.TemplateRoutesFiles, analysis.NewRoutes, analysis.Check and
  the template call listings take those values. muxt.Definitions takes
  a muxt.Templates, so it no longer needs check.DefinitionFinder.

- The fake server, which hands the package list to counterfeiter, moves
  to internal/fakeserver; goimports formatting moves from astgen into
  generate, the one package that formats files.

muxt no longer links go/packages or typelate/check, and generate no
longer links go/packages. generate's own tests drop their package load
and run in milliseconds instead of half a second.

Assisted-by: Claude:claude-opus-5 gofumpt
A test that wants to know how a route resolves needs go/types values: a
receiver whose methods take an *http.Request, a parameter that
implements encoding.TextUnmarshaler. Getting those from a real package
runs the go command, and type checking the standard library from source
instead takes over five seconds, most of it net/http.

Resolution only ever asks a handful of questions of the standard
library, so internal/typestest declares just that API -- net/http,
context, net/url, mime/multipart, io, encoding, encoding/json, time and
a few more -- under the real import paths, and type checks source
against them in memory. The stubs check in microseconds, once per test
binary.

TestArgument is its first user: its 75 cases read testdata/example
through the stubs rather than packages.Load, and the muxt package's
tests go from 0.15s to 0.01s. The example no longer needs a go.mod.

Assisted-by: Claude:claude-opus-5 gofumpt
A route path helper takes each path parameter as the type the handler
parses it into, and that type was only known once the handler had been
generated: argument parsing wrote it onto the definition as it went, and
the route path helpers, generated afterwards, read it back. It worked
because the two ran in that order, and the path helpers could not be
asked about a route without generating its handler first.

The type is a fact about the call, so resolution records it now. The
rule is the one parsing followed: the first place the call passes a
parameter, depth first in argument order, decides -- its parameter type,
unless a string is assignable to it, when the value is passed along
unparsed. TestPathValueTypes states it.

Argument parsing also rewrote the definition's call expression in place,
swapping each argument for the local it declared, so generating a
handler twice from one definition read the first run's rewrites. It
rewrites a copy now, and generation leaves the routes it reads as they
were resolved.

The new generate tests build their input in memory -- a package
checked against the stub standard library, templates parsed from a
string -- and call TemplateRoutesFiles and the handler assembly directly.

Assisted-by: Claude:claude-opus-5 gofumpt
goimports does two jobs on a generated file: it drops the imports the
file does not use, and it adds an import for any selector it cannot
resolve. The second job had been doing muxt's work. The routes file took
its import list before building TemplateData and its methods, so the
fmt, errors, io and strings they register arrived too late, and goimports
found and added them.

Finding a package means walking the file's directory and the module
cache, so every generate paid for a search whose answer muxt already
had, and what it found depended on the machine. With no sibling files to
declare the templates variable -- as in a unit test -- it searched for a
package named templates, and one generation took 80ms.

The routes file now fills in its imports after the declarations that
register them. Unused imports are dropped with goimports' own rule -- a
selector on an identifier that resolves to nothing in the file names a
package -- and goimports is run with FormatOnly, keeping its import
grouping and formatting. Every generated file in the integration suite
is unchanged, and generating a file in memory takes under a
millisecond.

Assisted-by: Claude:claude-opus-5 gofumpt
generate's own tests covered 4% of it: what a handler looks like for a
form struct, an sse route, a status code in the name or a redirect in
the template was only stated by the integration suite, which compiles
and runs every scratch module and takes 80 seconds.

testdata/*.txtar now holds one case per feature: the receiver's Go
source, the templates, the configuration as JSON, and the files and log
lines generation produces. TestSnapshots type checks the source against
the stub standard library and generates in memory, so the 32 cases run
in 70ms and cover 92% of the package. go test -run TestSnapshots
-update rewrites the want/ files; the diff is the review.

A snapshot says what the generator does, not that the result compiles
or serves requests; that stays the integration suite's job. What it
adds is an assertion on every line generation writes, at a speed
mutation testing can afford. Each case's files were checked once
against the real loader and CLI in a scratch module, and matched.

Assisted-by: Claude:claude-opus-5 gofumpt
muxt check is the command the integration suite runs most, and its
reports -- a field the data type lacks, a template nothing renders, a
route waiting for muxt generate -- were only stated there, behind a
package load per script. analysis had 10% coverage of its own.

analysis/testdata/*.txtar now holds cases for check, the route listing,
and the template caller and call listings, run the same way as generate's
snapshots: the Go source is type checked against the stub standard
library, the templates are parsed from the archive, and the test finds
the ExecuteTemplate calls and wires the trees and definitions the way
internal/load does. Each case's report was compared once with the real
CLI in a scratch module; they matched.

typestest gains CheckSyntax, which also returns the files and type
information a test walks, and stubs for html/template and embed.

Assisted-by: Claude:claude-opus-5 gofumpt
The mutation run loaded packages with their test files through its own
packages.Load call. It moves into load.PackagesWithTests unchanged, so
internal/load is the only caller of go/packages, and the claim the
contributor guide now makes is true.

The guide's architecture section named packages that no longer exist
(internal/muxt/parse, internal/source). It now draws the run as it is --
the load, the in-memory source, resolved routes, and what is generated
or reported -- and points at the snapshot tests and internal/typestest
before an integration script, which is for what needs the go command.

Assisted-by: Claude:claude-opus-5 gofumpt
Copilot AI lite review requested due to automatic review settings September 12, 2026 22:25
@crhntr
crhntr force-pushed the refactor/load-pacakges branch from 548e6e1 to cdef5ba Compare September 12, 2026 22:33

Copilot AI 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.

🟡 Changes recommended

Three unresolved findings remain, including two critical defects and one moderate CLI failure.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR isolates Go package loading behind internal/load, enabling route resolution, generation, and analysis to use plain go/types values and in-memory fixtures.

Changes:

  • Centralizes packages.Load with muxt.Package and muxt.Source adapters.
  • Adds type-checking stubs and snapshot-based test coverage.
  • Refactors generation, analysis, mutation testing, CLI, and fake-server plumbing.
File summaries
File Summary
internal/typestest/typestest.go In-memory type-checking helpers.
internal/typestest/typestest_test.go Type-checking helper tests.
internal/typestest/stubs.go Standard-library stubs.
internal/muxt/unmarshal.go Package-based type lookup.
internal/muxt/testdata/example/go.mod Example module fixture.
internal/muxt/path_value_types_test.go Path-value type tests.
internal/muxt/package.go Package abstraction.
internal/muxt/name_error_test.go Name-error tests.
internal/muxt/definition.go Resolved path-value storage.
internal/muxt/definition_test.go Definition tests.
internal/muxt/definition_internal_test.go Internal definition tests.
internal/muxt/call_test.go critical (1 vote; issue in call.go): repeated path names can retain the first type and skip a required conversion.
internal/muxt/call_internal_test.go Internal call tests.
internal/mutation/traverse.go Mutation traversal refactor.
internal/mutation/plan.go Mutation plan loading.
internal/mutation/diff.go Mutation diff updates.
internal/load/templates_test.go Template-loading tests.
internal/load/source.go Loaded-package source adapter.
internal/load/package.go Centralized package loading.
internal/load/package_test.go Package-loading tests.
internal/load/diagnostic.go Loading diagnostics.
internal/load/diagnostic_test.go Diagnostic tests.
internal/generate/validation_test.go Generation validation tests.
internal/generate/testdata/synthesized_method_note.txtar Generation snapshot fixture.
internal/generate/testdata/route_without_call.txtar Generation snapshot fixture.
internal/generate/testdata/response_argument.txtar Generation snapshot fixture.
internal/generate/testdata/request_body.txtar Generation snapshot fixture.
internal/generate/testdata/redirect.txtar Generation snapshot fixture.
internal/generate/testdata/receiver_method_sets.txtar Generation snapshot fixture.
internal/generate/testdata/nested_calls.txtar Generation snapshot fixture.
internal/generate/testdata/multipart.txtar Generation snapshot fixture.
internal/generate/testdata/marshal_json.txtar Generation snapshot fixture.
internal/generate/testdata/last_event_id.txtar Generation snapshot fixture.
internal/generate/testdata/inferred_methods.txtar Generation snapshot fixture.
internal/generate/testdata/form_values.txtar Generation snapshot fixture.
internal/generate/testdata/form_struct.txtar Generation snapshot fixture.
internal/generate/testdata/flag_unexported_identifiers.txtar Generation snapshot fixture.
internal/generate/testdata/flag_muxt_version.txtar Generation snapshot fixture.
internal/generate/testdata/flag_logger_path_prefix_middleware.txtar Generation snapshot fixture.
internal/generate/testdata/flag_htmx.txtar Generation snapshot fixture.
internal/generate/testdata/flag_custom_names.txtar Generation snapshot fixture.
internal/generate/testdata/execute_callback.txtar Generation snapshot fixture.
internal/generate/testdata/err_signals_without_datastar.txtar Generation snapshot fixture.
internal/generate/testdata/err_route_paths_method_collision.txtar Generation snapshot fixture.
internal/generate/testdata/err_response_state_with_response_argument.txtar Generation snapshot fixture.
internal/generate/testdata/err_resolution.txtar Generation snapshot fixture.
internal/generate/testdata/err_name_errors.txtar Generation snapshot fixture.
internal/generate/testdata/err_duplicate_pattern.txtar Generation snapshot fixture.
internal/generate/sse.go SSE generation refactor.
internal/generate/source_test.go Source generation tests.
internal/generate/snapshot_test.go Snapshot test harness.
internal/generate/routes_test.go Route generation tests.
internal/generate/html.go HTML generation updates.
internal/generate/groups.go Route group generation.
internal/generate/generated_test.go Generated-output tests.
internal/generate/format.go critical (1 vote): explicit import aliases can be pruned, leaving generated code uncompilable.
internal/generate/file.go Generated file construction.
internal/generate/file_test.go Generated file tests.
internal/fakeserver/fakeserver.go Fake-server generation extraction.
internal/cli/commands.go moderate (2 votes): output files in subdirectories can fail because the output directory is not loaded.
internal/astgen/format.go AST formatting updates.
internal/asteval/parse.go AST evaluation parsing.
internal/analysis/testdata/routes.txtar Analysis snapshot fixture.
internal/analysis/testdata/check_wrong_field.txtar Analysis snapshot fixture.
internal/analysis/testdata/check_unused_templates.txtar Analysis snapshot fixture.
internal/analysis/testdata/check_template_not_found.txtar Analysis snapshot fixture.
internal/analysis/testdata/check_passes.txtar Analysis snapshot fixture.
internal/analysis/testdata/check_bad_route_name.txtar Analysis snapshot fixture.
internal/analysis/testdata/calls.txtar Analysis snapshot fixture.
internal/analysis/testdata/callers.txtar Analysis snapshot fixture.
internal/analysis/testdata/callers_match.txtar Analysis snapshot fixture.
internal/analysis/templates.go Template analysis refactor.
internal/analysis/template_calls.go Template-call analysis.
internal/analysis/template_callers.go Template-caller analysis.
internal/analysis/routes.go Route analysis.
internal/analysis/check.go Analysis checks.
CLAUDE.md Architecture and workflow documentation.
Review details

Files not reviewed (1)

  • internal/generate/generated_test.go: Generated file
  • Files reviewed: 86/87 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/generate/format.go Outdated
Comment on lines +69 to +70
if id, ok := sel.X.(*ast.Ident); ok && id.Obj == nil {
used[id.Name] = true
Comment thread internal/muxt/call.go
Comment on lines +155 to +160
case ArgumentTypeRequestPathValue:
if seen[arg.Identifier] || IsSSEArgument(arg.Identifier) {
continue
}
seen[arg.Identifier] = true
if !isStringAssignable(arg.ParamType) {
Comment thread internal/cli/commands.go Outdated
warnPartialAST(log.New(cmd.ErrOrStderr(), "", 0), pl)
files, err := generate.TemplateRoutesFiles(*workingDirectory, config, fileSet, pl, log.New(stdout, "", 0))
// The routes file belongs to the package in its own directory.
src, err := load.Source(filepath.Dir(filepath.Join(*workingDirectory, config.OutputFileName)), pl, load.SourceConfiguration{
The mutation run's planning -- which templates each ExecuteTemplate call
reaches, with what dot, and which variations apply -- read the package
list directly: the loaded templates, the checker built on them, and the
syntax trees it searched for the string literal a template was written
in. So stating any of it took a module on disk and the go command.

Planning now runs on an input: the package's types and each templates
variable as type checking reads it. loadInput builds one with the go
command; planFrom and revisionOf read nothing else. A template's string
literal is found by parsing its file's text, which planning already holds,
instead of searching the loader's syntax trees.

The templates variable a type checker reads moves from analysis to
internal/templateset, so the mutation package shares it without importing
analysis, and load.TemplateSets builds it for both.

internal/load/loadtest builds what a package load returns without the go
command: it writes the files, type checks them against typestest's stub
standard library (which gains io/fs and more of html/template), and
assembles the *packages.Package. Tests then go through load's real
template evaluation, definitions and ExecuteTemplate calls.

internal/mutation/testdata/*.txtar snapshots a dry run's report for ten
cases -- a literal template, partials and trims, a template pattern,
skipped mutants, the operand budget, other delimiters, a --diff revision,
and the errors a plan returns -- planned from a loadtest package in
milliseconds. Each report was compared once with the real CLI in a
scratch module, --diff against a git repository, and matched.

Assisted-by: Claude:claude-opus-5 gofumpt
A large share of the integration suite checks that flags mean what they
say, and it could only do that by running the whole command: load a
package, generate, compile. Deciding what a command line means happens
before any of that, but nothing let a test stop there.

Each command now builds its configuration -- flags parsed, defaults
applied, what cannot work rejected -- and hands it to a runner, which
loads the package and runs the implementation. Commands wires the real
runners; configurationOf wires ones that record what they are handed.
TestCommandLineConfigurations states what command lines parse into, as
composite literals, and TestCommandLineRejections the ones refused, with
the error a user sees. The listing commands' configurations carry every
templates variable, rather than one set per loop pass, so the literal is
the whole run. load.GenerateSource and load.RoutesSource hydrate a
configuration into the implementation's input.

The generate and analysis snapshots take their configuration from the
same kind of literal, inline in snapshots_test.go with the command line it
stands for, instead of a config.json layered over test defaults. An
implementation test only ever sees a configuration a command line can
produce; rejecting the rest is the command line's job. The generate
snapshots now run with the defaults the command line applies, so
TemplateData gains its MuxtVersion method in each, and the version case
becomes the case that leaves it out.

Assisted-by: Claude:claude-opus-5 gofumpt
What internal/load handed on came in two shapes for one idea: a
muxt.Source for generate and the route listing, and a package with
[]templateset.Variable for check, the listings and mutation. Neither was
data. A package found its imports through a function, a variable reported
name positions through another, and trees and definitions through
interfaces backed by check.Templates. A variable that failed to load
carried its error inside it. ExecuteTemplate calls carried their AST node,
so reading a position needed the FileSet. A test could not write any of it
as a literal; the analysis snapshots re-implemented a definition finder to
fake one.

internal/source now holds the one shape: a Package with its FileSet, its
types, its imports by path, and its templates variables. A Variable holds
its template set, its functions, its definitions by template name, and its
calls, each with a position, the template name and the type of dot. No
function fields, no interfaces, no errors. muxt, generate, analysis and
mutation read it; analysis and mutation build their check.Global from it.
muxt.Package, muxt.Source, muxt.Templates and internal/templateset are
gone.

load.Package reads a load into it and fails at the first variable that
does not evaluate, rather than carrying errors for a later stage to report.
That only changes which error is reported when two variables fail
differently, which no command line in the integration suite exercises.
load.GenerateSource and load.RoutesSource add the receiver and still report
a missing package, then a missing receiver, before any variable;
TestHydration states that order.

The generate and analysis snapshots now load through loadtest and load's
hydration, as the commands do, rather than building their input by hand.
Every generated file and error is unchanged; the analysis reports move by
the lines their sources no longer spend declaring the templates variable.
loadtest includes encoding, fmt and net/http in a load, as load.Packages
does.

Assisted-by: Claude:claude-opus-5 gofumpt
A generated file could declare imports it did not use, and formatFile
dropped them after the fact. The cause was one import registry shared by
every file a run writes: with --output-multiple-files, each per-file routes
file and the main one declared every package any of them had registered.
The registry lived on the same value that held the loaded packages, so
nothing marked where one file's imports ended and the next began.

Each file a run writes now builds its declarations against a File of its
own, so the imports it declares are exactly the ones its declarations
registered. With that, no file in the snapshots or the integration suite
has an import to drop -- checked by failing on any before removing the
filter -- so formatFile no longer parses the output to find them.
astgen.ImportManager loses Types: finding a loaded package is not what an
import registry is for, and the one caller asks the output package.

The snapshot tests now fail on a generated file that imports a package it
does not use, which is what the filter used to hide.

Assisted-by: Claude:claude-opus-5 gofumpt
With each file declaring exactly the imports it uses, goimports had one
job left: laying the import block out in groups, the standard library
before everything else. That kept golang.org/x/tools/imports -- and its
walks of the module cache when asked for more -- in generate's build.

formatFile now writes the import block itself, grouped the way goimports
groups it (a path whose first element has a dot after those that have
none), sorted within each group and set apart by a blank line, and formats
the file with go/format. TestFormatFileImports states the layout, which
was compared once with goimports' output for the same imports. The fake
server's main.go, whose imports its template writes out, is formatted the
same way.

generate no longer links any of golang.org/x/tools. Every generated file
in the snapshots and the integration suite is unchanged.

Assisted-by: Claude:claude-opus-5 gofumpt
@crhntr

crhntr commented Sep 14, 2026

Copy link
Copy Markdown
Member Author

Split into a stack of smaller PRs, starting with #146 (then #147, #148, #149, #150, #151, #152, #153). The branch refactor/load-pacakges is left as it was, for reference.

@crhntr crhntr closed this Sep 14, 2026
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.

2 participants