fix(packages): generalize go list diagnostic preference for Go sources

[MeteorsLiu]

Extends #2274's authoritative syntax diagnostic handling to go list build diagnostics that originate from a package's compiled Go files.

The implementation includes:

• Recognizes only ListError diagnostic blocks whose source path matches the package's CompiledGoFiles.
• Keeps metadata, assembly, cgo, and unrelated-package errors outside the authoritative diagnostic path.
• Suppresses local parser and type-checker follow-on diagnostics for matching packages.
• Leaves the existing deduplication preload and diagnostic-line matching behavior unchanged.
• Removes stale Go 1.26 errorcheck expectations for cases that now pass with go list diagnostics.

This aligns LLGo's frontend errors with cmd/compile while limiting the behavior to verified Go source diagnostics.

[fennoai]

Review summary

Rewrites the go/types diagnostic name X not exported by package … into the gc-style undefined: pkg.X so the go1.26 errorcheck case runtime.go passes, and drops the corresponding xfail.yaml entry. The mechanism is sound: keying by err.Pos (== sel.Sel.Pos()) is exact and collision-free, closure-capture ordering is safe (the map is populated before NewChecker(...).Files(...) runs), and the message-prefix guard makes an unintended rewrite effectively impossible since that message is package-exclusive in go/types.

One correctness concern worth addressing (inline), plus a minor efficiency note below.

Minor: eager, unconditional AST walk on the per-package load path

internal/packages/load.go (~line 373): the ast.Inspect walk and map[token.Pos]string are built for every package on every load, even when the package type-checks cleanly and no message ever needs rewriting. It also records all ident-qualified selectors (v.Field, x.Method, …), not just import-qualified ones, so the map is larger than the single use case requires. This is pure overhead for the common (no-error) case. Since it runs once per package before type-checking, the cost is likely modest relative to type-checking itself — but building the map lazily inside the types.Error branch (only when the target message is actually seen), or restricting insertion to import-qualified selectors, would eliminate it. Non-blocking.

View job run

[fennoai]

Review: link-time DCE for ABI method slots + per-package frontend options

Thanks for this — the DCE analysis is well-structured, the cl.Options refactor that replaces the process-wide Enable* globals is clean and consistently threaded, the sync.Once around llssa.Initialize is safe (init flags are a compile-time constant), and the RewriteMainPrefix removal left no dangling references. Comprehensive tests accompany the new packages.

Findings below, ordered by severity. The correctness-class items (a live method being pruned, or a panic on valid input) matter most because they surface only under -deadcodedrop.

Correctness / robustness

• internal/dcepass/dcepass.go:147 — panic on an unsupported global value is reachable. cloneGlobalValue handles only IsAFunction and IsAGlobalVariable; any other GlobalValue (GlobalAlias, GlobalIFunc) hits panic("dcepass: unsupported global value"). llgo does emit aliases (ssa/decl.go:148 llvm.AddAlias), so if a pruned ABI type descriptor ever references one — directly or through a cloned field — the whole build aborts. Prefer rebinding aliases by name to a dst declaration (as functions are handled) or keeping the value, rather than panicking. (inline)

• internal/dcepass/dcepass.go:114-127 — cloneConst does not recurse into ConstantExpr. It recurses only through ConstantStruct; a ConstantExpr (GEP / bitcast / inttoptr, all produced elsewhere in codegen) falls through to return v, leaving the dst-module initializer pointing back into the source module's value graph. Latent today under opaque pointers where the observed fields are plain global refs, but a typed-pointer build or a codegen change that materializes a field as a constant-expression would produce a cross-module reference. Consider recursing through ConstantExpr/ConstantArray operands, or explicitly asserting the field shapes you support. (inline)

• internal/dcepass/dcepass.go:68-76 — method-array element read as a 4-operand struct without validation. methodArray validates the element type has 4 fields, but the drop branch unconditionally reads orig.Operand(0..3) (and Operand(2/3).Name() when verbose) on each element value. A ConstantAggregateZero element has OperandsCount()==0, so Operand(i) would be out of range. Safe today because llgo always emits non-null ifn/tfn, but it is an unguarded assumption on IR shape. (inline)

• internal/deadcode/analyze.go:46-52 / internal/build/build.go (dceEntryRootCandidates) — missing entry roots are silently dropped. Analyze skips any root name absent from the summary. If a mandatory entry root such as main.main ever fails to resolve (e.g. a metadata-collection regression), the pass silently treats far more slots as dead and prunes live code with no diagnostic. A cheap guard/verbose-warning when main.main/main.init resolve to nothing would turn a silent miscompile into a visible error.

Performance

• internal/deadcode/analyze.go:279-284 — popWork is O(n) per dequeue → O(n²) flood. copy(d.workQueue, d.workQueue[1:]) shifts the entire remaining slice on every pop, so draining N enqueued symbols costs O(N²) over a whole-program analysis. The loop runs to a fixpoint, so FIFO vs LIFO is irrelevant to correctness — popping from the tail (d.workQueue[len-1]; d.workQueue = d.workQueue[:len-1]) is O(1), or use a head index. Single highest-value change in the analysis. (inline)

• internal/meta/global.go:264-272 — OrdinaryEdges grows via append with no pre-size on the hot flood path, unlike the structurally identical TypeChildren (line 282) which uses make([]Symbol, len(local)). Pre-size to len(edges) for consistency. Separately, markUsedInIface re-enqueues an already-reachable symbol, causing flood to re-walk its ordinary edges + demands purely to reach the usedInIface branch — a separate "already flooded" guard would avoid that duplicate work.

Minor / docs

• internal/dcepass/dcepass.go:20-21 — doc says "clears IFn/TFn" but the code replaces both with the unreachableMethod stub (the slots stay populated, pointing at a trap). "redirects IFn/TFn to an unreachable stub" describes it accurately. (inline)

• internal/dcepass/dcepass.go:53 — redundant map write. emitTypeOverride sets e.values[srcType] = dstType, but ensureOverrideGlobal (called the line above) already assigns the same key. The line-53 write is dead.

• cl/compile.go:2117-2119 — comment steers multi-package drivers to a now-deprecated function. The block comment says multi-package drivers "use NewPackageExWithEmbed", which this PR marks Deprecated: use NewPackageExWithEmbedMetaOptions. Point the guidance at the new entry point.

No blocking issues; the panic-robustness and the O(n²) dequeue are the two I'd most want addressed before this runs on large programs.

Findings without inline locations

• internal/dcepass/dcepass.go:147: cloneGlobalValue handles only functions and global variables; any other GlobalValue (GlobalAlias, GlobalIFunc) reaches this panic. llgo does emit aliases (ssa/decl.go:148), so if a pruned ABI type descriptor references one — directly or via a cloned field — the build aborts on valid input. Consider rebinding aliases by name to a dst declaration (like functions above) or falling back to keeping the value, rather than panicking.
• internal/dcepass/dcepass.go:120: cloneConst recurses only through ConstantStruct. A ConstantExpr (GEP/bitcast/inttoptr) falls through to return v here, leaving the dst-module initializer referencing the source module's value graph. Latent under opaque pointers (observed fields are plain global refs), but a typed-pointer build or a codegen change that materializes a field as a constant-expression would produce a cross-module reference. Consider recursing through ConstantExpr/ConstantArray operands, or asserting the shapes you support.
• internal/dcepass/dcepass.go:70: The drop branch unconditionally reads orig.Operand(0..3) (and Operand(2/3).Name() when verbose). methodArray validated the element type has 4 fields, but not that each element value is a materialized 4-operand ConstantStruct — a ConstantAggregateZero element has OperandsCount()==0 and would fault here. Safe today since ifn/tfn are always non-null, but it's an unguarded assumption on IR shape.
• internal/deadcode/analyze.go:283: copy(d.workQueue, d.workQueue[1:]) shifts the whole remaining slice on every pop, making flood() O(n²) over a whole-program analysis. The loop runs to a fixpoint so ordering is irrelevant — popping from the tail is O(1):
• internal/dcepass/dcepass.go:20: Nit: "clears IFn/TFn" — the code replaces both fields with the unreachableMethod stub (slots stay populated, pointing at a trap) rather than clearing them. "redirects IFn/TFn to an unreachable stub" would be accurate.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fennoai]

Review (corrected): align unexported-package-member diagnostics

⚠️ My earlier review on this PR reviewed the wrong diff — it compared the head branch against the current main tip and so pulled in dozens of already-merged, unrelated commits (the DCE feature, the cl.Options refactor, etc.). Please disregard that review. This PR's real base is 2310ff87 and it changes only 3 files. This review supersedes it.

The actual change makes llgo report undefined: alias.hidden (matching gc) instead of name hidden not exported by package ... when source references an unexported member of another package, and removes the now-passing runtime.go go1.26 errorcheck case from xfail.yaml. It's focused and well-tested (load_test.go asserts both the new message and the absence of the old one).

Only minor observations — nothing blocking.

Observations

• internal/packages/load.go:291 — the rewrite is coupled to go/types' exact error wording. The trigger is strings.HasPrefix(err.Msg, "name "+name+" not exported by package "). If a future Go toolchain rephrases that diagnostic, the rewrite silently stops firing and the original (non-gc) message resurfaces. The failure mode is graceful (you fall back to the upstream message, and the goroot errorcheck test would flag the regression), so this is acceptable — but a brief comment noting the dependency on the go/types string would help the next reader understand why the match is so literal.

• internal/packages/load.go:383-385 — packageSelectors records every ident-qualified selector, not just package qualifiers (x.field, v.Method, and pkg.Name all land in the map). This is harmless because the rewrite at line 289-293 only activates on the name X not exported by package message, which go/types emits exclusively for package-qualified access to an unexported identifier — so a non-package qualifier can never reach the rewrite. Worth being aware of if that message guard is ever loosened; today it is safe by construction.

• internal/packages/load.go:260,289 — nil-map read is safe. packageSelectors is declared nil at line 260 and only populated at line 376, after parse/driver errors but before type-checking. The line-289 lookup runs only for types.Error (emitted during type-checking, after population), and a read from a nil map returns the zero value regardless — so there is no ordering hazard or panic risk. Noting it only to confirm it was checked.

Overall this looks correct and appropriately scoped. No changes required.

[github-actions]

LLGo baseline benchmarks

15c415828e8a | workflow run | long-term charts

Program measurements

Platform	Workload	File size	vs base	Build	vs base	Run	vs base
Linux	cprintf	18456 B	+0.0%	326.428 ms	+10.8% (worse)	1.376 ms	+0.4% (worse)
Linux	fmtprintf	1829968 B	+0.0%	2.999 s	-1.3% (better)	3.509 ms	+0.2% (worse)
Linux	println	68008 B	+0.0%	293.546 ms	-2.0% (better)	1.752 ms	+1.7% (worse)
macOS	cprintf	84672 B	+0.0%	314.724 ms	-7.2% (better)	2.605 ms	-3.1% (better)
macOS	fmtprintf	1869328 B	+0.0%	2.613 s	+1.1% (worse)	11.910 ms	+1.7% (worse)
macOS	println	121200 B	+0.0%	332.236 ms	+4.1% (worse)	3.586 ms	+2.7% (worse)

Core language and compiler benchmarks

Platform	Benchmark	ns/op	vs base
Linux	BenchmarkLookupPCRandom	12.310 ns/op	-0.2% (better)
Linux	BenchmarkMergeCompilerFlags	144.300 ns/op	-0.5% (better)
Linux	BenchmarkMergeLinkerFlags	93.770 ns/op	-0.6% (better)
Linux	BenchmarkChannelBuffered	37.060 ns/op	+0.0% (worse)
Linux	BenchmarkChannelHandoff	25548 ns/op	+0.7% (worse)
Linux	BenchmarkDefer	51.210 ns/op	+4.5% (worse)
Linux	BenchmarkDirectCall	1.760 ns/op	+0.1% (worse)
Linux	BenchmarkGlobalRead	1.758 ns/op	+0.0%
Linux	BenchmarkGlobalWrite	2.810 ns/op	+0.0%
Linux	BenchmarkGoroutine	31176 ns/op	+0.2% (worse)
Linux	BenchmarkInterfaceCall	9.148 ns/op	+0.0% (worse)
Linux	BenchmarkRuntimeGetG	2.113 ns/op	+0.0% (worse)
macOS	BenchmarkLookupPCRandom	11.510 ns/op	-2.0% (better)
macOS	BenchmarkMergeCompilerFlags	103.500 ns/op	+2.3% (worse)
macOS	BenchmarkMergeLinkerFlags	67.250 ns/op	-2.6% (better)
macOS	BenchmarkChannelBuffered	22 ns/op	+0.2% (worse)
macOS	BenchmarkChannelHandoff	6949 ns/op	-0.5% (better)
macOS	BenchmarkDefer	27.120 ns/op	-9.9% (better)
macOS	BenchmarkDirectCall	1.024 ns/op	+0.1% (worse)
macOS	BenchmarkGlobalRead	1.012 ns/op	-10.9% (better)
macOS	BenchmarkGlobalWrite	1.025 ns/op	-8.3% (better)
macOS	BenchmarkGoroutine	40900 ns/op	+42.8% (worse)
macOS	BenchmarkInterfaceCall	4.515 ns/op	+2.7% (worse)
macOS	BenchmarkRuntimeGetG	2.049 ns/op	-3.3% (better)

Compared with 6670dae3884d measured in the same runner job.

[fennoai]

Review summary

This PR rewrites the go/types diagnostic name X not exported by package … into the gc-style undefined: qualifier.X, adds a regression test, and drops the now-reproduced go1.26 errorcheck runtime.go entry from notapplicable.yaml. The approach is sound and correctly gated: the rewrite runs only for locally generated types.Errors (the hasCompilerSyntaxError/hasCompilerDiagnostics early-returns still win), and the packageSelectors map is only built after the NeedTypes == 0 return, so it is skipped when types aren't requested. Cross-file token.Pos collisions are not possible since both the map key (sel.Sel.Pos()) and the lookup key (err.Pos) come from the shared ld.Fset.

A few points worth addressing — see inline comments. None are blocking.

Additional note (no reliable inline anchor):

• Overlap with the test harness normalizer. test/goroot/runner_test.go already has normalizeGoTypesDiagnosticMessage, which reshapes several go/types diagnostics for GOROOT errorcheck matching and is explicitly documented as affecting only the test harness. This PR instead moves an equivalent transform into the production loader path (lpkg.Errors), which is a user-facing behavior change. Please confirm the two paths don't double-transform this message and that the placement (production vs. harness) is intentional and consistent with the existing convention.

Findings without inline locations

• internal/packages/load.go:405: sel.X is only checked to be an *ast.Ident; it is not verified to be a package qualifier. This records every ident.member selector (local vars, receivers, params, field access), not just package-qualified references. Correctness is preserved today only because the rewrite is additionally gated on the "name X not exported by package " message prefix, which go/types emits solely for package-member access. It's worth a short comment stating that the message-prefix check — not the AST classification — is the real filter, so a future reader (or a future go/types wording that reused similar phrasing) doesn't get a misleading undefined: x.field rewrite for a struct-field selector.
• internal/packages/load.go:311: This hard-codes the exact go/types wording "name %s not exported by package %s", which is a compiler-internal diagnostic string with no stability guarantee and has changed across Go versions. If the wording changes, the rewrite silently becomes a no-op (graceful, but invisible) and only the new test would catch it. Please add a comment anchoring this to the go/types version whose wording it matches — matching the version-coupling comments already present elsewhere in this file (e.g. the go1.16 / go list skew note) and the intentionally affects only the test harness note in runner_test.go.
• internal/packages/load.go:310: name is re-derived by splitting the stored "qualifier.Sel" string (selector[strings.LastIndexByte(selector, '.')+1:]) that was itself built by joining qualifier.Name + "." + sel.Sel.Name. That's a join-then-resplit round-trip. Storing a small struct {qualifier, sel string} (or two maps) in the inspect loop would drop the re-parse and make intent clearer. Safe today since identifiers can't contain ., but needlessly indirect.
• internal/packages/load.go:396: The full-AST walk and map allocation run unconditionally for every type-checked package, on the common no-error path, purely to service a rare error-message rewrite. loadPackageEx runs per package across the whole transitive import graph (concurrent fan-out), so this adds a second whole-program AST traversal plus a map holding a large fraction of each package's selectors, almost always thrown away unused. Consider building the map lazily on the first "not exported by package " error, or resolving just the single failing err.Pos on demand. Not blocking, but the eager index is speculative work on the hot path.

[MeteorsLiu]

NOTE: BAD performance

[MeteorsLiu]

/review

[fennoai]

@MeteorsLiu Thanks for the trigger request.

This repository currently enforces member-only triggers, and your account does not have repository membership access, so Fenno cannot run for this request.

To proceed, you can:

• Ask a repository maintainer to add you as a collaborator/member, or
• Update .fennoai.yml and set require_member_to_trigger: false.

[fennoai]

@MeteorsLiu Thanks for the trigger request.

This repository currently enforces member-only triggers, and your account does not have repository membership access, so Fenno cannot run for this request.

To proceed, you can:

• Ask a repository maintainer to add you as a collaborator/member, or
• Update .fennoai.yml and set require_member_to_trigger: false.

[fennoai]

Review Summary

This replaces the fragile substring-based hasCompilerSyntaxError check with a positional match (hasGoSourceListDiagnostics + sameDiagnosticFile) that confirms a go list ListError diagnostic actually targets one of the package's CompiledGoFiles. The design is a clear improvement — anchoring "authoritative go list diagnostic" to the compiled Go source correctly avoids false positives from forwarded child-tool failures (e.g. an asm.s error being attributed to an asm.go package). Doc comments are thorough and the table tests cover absolute/relative paths, cgo output, unpositioned errors, and non-list errors.

Findings are all low-severity; none are blocking. The most substantive is the package-wide granularity of error suppression (see inline). Documentation accuracy: no issues found.

Additional notes (no reliable inline anchor)

• internal/packages/load.go — empty compiledGoFiles is untested. When compiledGoFiles is empty, hasGoSourceListDiagnostics returns false and follow-on errors are (correctly) not suppressed. This safe default is not exercised by TestHasGoSourceListDiagnostics; consider adding a case to lock it in against regressions.
• internal/packages/load.go — format-fragility, fail-open (informational). Matching depends on the exact "# " header prefix and the ": " separator in strings.Cut. If a future Go toolchain changes go list's block header or spacing, detection silently returns false and suppression stops — real errors would then be shown rather than hidden, so this degrades gracefully. Worth a test pinning the expected go list format.

[fennoai]

@MeteorsLiu Thanks for the trigger request.

This repository currently enforces member-only triggers, and your account does not have repository membership access, so Fenno cannot run for this request.

To proceed, you can:

• Ask a repository maintainer to add you as a collaborator/member, or
• Update .fennoai.yml and set require_member_to_trigger: false.

[fennoai]

Review summary

This is a solid, well-tested refactor. It replaces the narrow ": syntax error: " substring match with a more general check (hasGoSourceListDiagnostics) that treats a positioned go list diagnostic tied to one of the package's compiled Go files as authoritative, suppressing local parser/type follow-ons. The helpers are small, single-purpose, and covered by TestHasGoSourceListDiagnostics (absolute/relative/different-file/cgo/unpositioned/metadata/non-list cases). The removed test/goroot xfail/notapplicable entries are consistent with the broadened suppression and leave no dangling references. Operator precedence and filepath.IsAbs handling in sameDiagnosticFile are correct.

One substantive robustness finding is left inline. The rest are minor/informational and non-blocking:

• Behavior broadening (load.go:263, :283/:296): The new hasCompilerDiagnostics suppresses both scanner.ErrorList and types.Error follow-ons whenever go list has any positioned diagnostic on any compiled Go file in the package — not just syntax errors and not just the specific file. Previously only syntax errors triggered suppression. This appears intentional and matches the removed xfail entries, but it is a genuine widening; worth a one-line comment on the hasCompilerDiagnostics assignment documenting the intended granularity.
• sameDiagnosticFile relative suffix (load.go:664-672): A relative diagnostic path like example/load.go matches any absolute compiled path ending in /example/load.go. Fine in the normal flow (the relative path comes from the same go list invocation), but combined with the inline finding it slightly widens the accidental-match surface.
• diagnosticFileLine (load.go:608-624): Accepts a column-less file:N as a valid positioned diagnostic and, for malformed middle segments (e.g. load.go:2a:3), silently reinterprets a column as a line. Not reachable from well-formed go list output; informational only.

No performance or documentation issues found (nested loops are gated to the compile-failure path and bounded by small per-package inputs; doc-comment examples match actual behavior).

[MeteorsLiu]

@cpunion request for review, please.