Skip to content

internal/packages: normalize Go compiler syntax diagnostics - #2219

Draft
cpunion wants to merge 4 commits into
xgo-dev:mainfrom
cpunion:codex/xfail-parser-recovery-next-20260730
Draft

internal/packages: normalize Go compiler syntax diagnostics#2219
cpunion wants to merge 4 commits into
xgo-dev:mainfrom
cpunion:codex/xfail-parser-recovery-next-20260730

Conversation

@cpunion

@cpunion cpunion commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • treat Go command/compiler syntax diagnostics as canonical when go/parser reports recovery diagnostics on the same source line
  • normalize the missing-package-clause diagnostic class to the wording emitted by go tool compile
  • remove the GOROOT harness's source-text-specific matching; renamed identifiers and equivalent formatting now follow the same rule
  • enable seven Go 1.26 errorcheck cases fixed by the generic normalization

Correctness boundaries

Only scanner.ErrorList entries are filtered. A compiler diagnostic must be a packages.ListError and be anchored by one of:

  • a positioned compiler syntax diagnostic
  • an explicit syntax error: line in a # package compiler block
  • an exact positioned duplicate of a scanner diagnostic

Filtering then applies only to scanner recovery diagnostics from the same file and line. Parser-only diagnostics, other lines/files, I/O errors, type errors, and import/module/build diagnostics remain visible. Relative/absolute paths and //line virtual filenames are supported without inspecting source text.

The review counterexamples are covered with renamed identifiers and spacing changes (func renamed ( ) and a renamed/formatted top-level composite literal).

GOROOT coverage enabled

  • fixedbugs/bug050.go
  • fixedbugs/bug228.go
  • fixedbugs/bug435.go
  • fixedbugs/issue13274.go
  • fixedbugs/issue4776.go
  • syntax/chan1.go
  • syntax/vareq1.go

Validation

  • go test -vet=off ./internal/packages ./test/go ./test/goroot -count=1 on Go 1.26.5
  • normalization tests and all seven GOROOT cases on Go 1.24.11, Go 1.25.0, and Go 1.26.5
  • all remaining 133 Go 1.26 errorcheck xfails still fail for their recorded reasons
  • all new normalization helpers have 100% statement coverage

-vet=off is used for the full test/go run because the repository currently hits an unrelated Go analyzer panic; the targeted compatibility test passes independently.

Scope

No CI, wasm, DWARF, compile-flag, or unrelated xfail changes are included.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: match declaration recovery diagnostics

Focused, well-guarded change to the test/goroot errorcheck harness. The two new parser-recovery pairings are only consumed when the corresponding primary already matched an expected ERROR at the same canonical file+line, and each pair is consumed at most once (consumed flag in discardPairedParserDiagnostics), so the very generic secondary strings (expected ';', found '(' / '{') can't leak across unrelated diagnostics. Removing the two xfail.yaml entries in lockstep tightens coverage rather than suppressing it, and the new table-driven test asserts the three properties that matter: pair-passes, secondary-alone-fails, and near-match-primary-does-not-activate. I ran go test ./test/goroot -run 'TestCheckExpectedErrorsDiscardsPairedDeclarationRecoveryDiagnostics|TestCheckExpectedErrorsDiscardsExactParserPair' and it passes.

One finding below is best placed in the body since it sits on an unchanged context line.

test/goroot/runner_test.go:1651-1652 — stale doc comment count. The comment says parserRecoverySecondaries is "deliberately limited to exact diagnostic pairs emitted by the five GOROOT cases". This PR adds two more primaries (now ~7-8 distinct labels), so the count is now inaccurate (it was arguably already imprecise). Since this PR deliberately grows the set, consider dropping the hard-coded number, e.g. "...limited to exact diagnostic pairs emitted by a small, fixed set of GOROOT cases...", so the comment does not silently rot with each future addition.

Performance and security passes found nothing (test-only, constant-string mapping, t.TempDir()-scoped I/O).

Comment thread test/goroot/runner_test.go Outdated
@cpunion

cpunion commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review body note in ad11d2c2b: parserRecoverySecondaries no longer hard-codes the number of enabled GOROOT cases.

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cpunion
cpunion force-pushed the codex/xfail-parser-recovery-next-20260730 branch from ad11d2c to db603e4 Compare July 30, 2026 01:15
@cpunion
cpunion force-pushed the codex/xfail-parser-recovery-next-20260730 branch 2 times, most recently from dbeb8e0 to d3bf3eb Compare July 30, 2026 13:07
@cpunion
cpunion marked this pull request as draft July 31, 2026 02:20
@cpunion cpunion changed the title test/goroot: match declaration recovery diagnostics internal/packages: normalize Go compiler syntax diagnostics Jul 31, 2026
@cpunion cpunion added the go-test-compat Go standard-library and GOROOT test compatibility label Aug 9, 2026
@cpunion
cpunion force-pushed the codex/xfail-parser-recovery-next-20260730 branch from fec402c to af79f24 Compare August 9, 2026 04:36
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

LLGo baseline benchmarks

af79f2420cb2 | 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% 291.084 ms +0.4% (worse) 1.251 ms +2.1% (worse)
Linux fmtprintf 1861480 B +0.0% (worse) 3.022 s +0.3% (worse) 3.209 ms +2.1% (worse)
Linux println 68008 B +0.0% 291.485 ms -0.1% (better) 1.583 ms +4.0% (worse)
macOS cprintf 84672 B +0.0% 607.334 ms +32.2% (worse) 4.259 ms -15.3% (better)
macOS fmtprintf 1869328 B +0.0% 3.392 s +7.0% (worse) 12.915 ms -5.4% (better)
macOS println 121200 B +0.0% 589.248 ms +27.5% (worse) 5.408 ms +28.4% (worse)
Core language and compiler benchmarks
Platform Benchmark ns/op vs base
Linux BenchmarkLookupPCRandom 13.390 ns/op +0.0%
Linux BenchmarkMergeCompilerFlags 150 ns/op -1.3% (better)
Linux BenchmarkMergeLinkerFlags 94.300 ns/op +0.3% (worse)
Linux BenchmarkChannelBuffered 34.050 ns/op +0.1% (worse)
Linux BenchmarkChannelHandoff 27352 ns/op +0.4% (worse)
Linux BenchmarkDefer 47.970 ns/op +0.1% (worse)
Linux BenchmarkDirectCall 1.556 ns/op -0.1% (better)
Linux BenchmarkGlobalRead 1.559 ns/op +0.2% (worse)
Linux BenchmarkGlobalWrite 2.480 ns/op -0.0% (better)
Linux BenchmarkGoroutine 31344 ns/op +0.0% (worse)
Linux BenchmarkInterfaceCall 8.101 ns/op +0.0% (worse)
Linux BenchmarkRuntimeGetG 1.875 ns/op +0.4% (worse)
macOS BenchmarkLookupPCRandom 15.450 ns/op +19.1% (worse)
macOS BenchmarkMergeCompilerFlags 146.300 ns/op +28.7% (worse)
macOS BenchmarkMergeLinkerFlags 103.400 ns/op +55.3% (worse)
macOS BenchmarkChannelBuffered 21.500 ns/op -21.4% (better)
macOS BenchmarkChannelHandoff 7136 ns/op -21.5% (better)
macOS BenchmarkDefer 28.850 ns/op -28.5% (better)
macOS BenchmarkDirectCall 1.022 ns/op -15.6% (better)
macOS BenchmarkGlobalRead 1.031 ns/op -31.1% (better)
macOS BenchmarkGlobalWrite 1.027 ns/op -31.9% (better)
macOS BenchmarkGoroutine 32266 ns/op +1.5% (worse)
macOS BenchmarkInterfaceCall 4.575 ns/op -11.3% (better)
macOS BenchmarkRuntimeGetG 2.122 ns/op -23.1% (better)

Compared with cdd86117df30 measured in the same runner job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go-test-compat Go standard-library and GOROOT test compatibility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant