Skip to content

test/goroot: remove stale bug388 expectation - #2266

Closed
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:fix/goroot-bug388-diagnostics
Closed

test/goroot: remove stale bug388 expectation#2266
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:fix/goroot-bug388-diagnostics

Conversation

@MeteorsLiu

@MeteorsLiu MeteorsLiu commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Removes the stale Go 1.26 not-applicable entry for fixedbugs/bug388.go after #2274 made cmd/compile syntax diagnostics authoritative.

The implementation includes:

  • Removes fixedbugs/bug388.go from test/goroot/notapplicable.yaml.
  • Relies on the loader behavior merged in fix: prefer compiler syntax diagnostics #2274 and introduces no production code changes.
  • Verifies the case with the Go 1.26.5 GOROOT errorcheck runner and the expectation configuration tests.

This keeps the GOROOT expectations aligned with current LLGo behavior and prevents a passing case from being reported as an unexpected success.

@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: suppress type errors after syntax errors

Focused, correct change. Verified that the diagnostic suppression cannot cause a failed build to appear successful: buildSSAPkgs returns fmt.Errorf("cannot build SSA for packages") whenever len(errs) > 0, and package pass/fail is driven by IllTyped (set in internal/packages/load.go whenever len(lpkg.Errors) > 0), independent of which diagnostics are printed. Suppression is correctly scoped per-package (hasParseError is reset each errPkg iteration), and a pure type-error package still prints its diagnostics. No security or performance concerns (this runs only on the error path).

Two maintainability suggestions inline. Nothing blocking.

  • The added gopackages alias is justified — the existing internal/packages alias re-exports Error/Package types but not the ParseError/TypeError kind constants. (Optionally, re-exporting those two constants from internal/packages would let this file keep a single packages identifier.)

Comment thread internal/build/build.go Outdated
})
if len(errs) > 0 {
for _, errPkg := range errs {
hasParseError := false

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.

Two small maintainability points on this block:

  1. The first loop can collapse into a single pass — slices is already imported, so hasParseError := slices.ContainsFunc(errPkg.Errors, func(e packages.Error) bool { return e.Kind == gopackages.ParseError }) avoids iterating errPkg.Errors twice.
  2. Silently dropping TypeError output when a ParseError is present is a deliberate decision (type errors are cascading noise once a file fails to parse). A one-line comment explaining the rationale would prevent a future reader from mistaking it for accidental error swallowing.

@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

LLGo baseline benchmarks

5787cd3ec27a | 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% 285.548 ms -1.8% (better) 1.283 ms -3.5% (better)
Linux fmtprintf 1829968 B +0.0% 2.978 s +0.3% (worse) 3.086 ms -1.0% (better)
Linux println 68008 B +0.0% 289.121 ms +0.6% (worse) 1.574 ms -0.7% (better)
macOS cprintf 84672 B +0.0% 365.997 ms -49.7% (better) 4.917 ms -31.0% (better)
macOS fmtprintf 1869328 B +0.0% 5.181 s +85.1% (worse) 16.491 ms +27.4% (worse)
macOS println 121200 B +0.0% 361.533 ms -44.5% (better) 3.953 ms -24.9% (better)
Core language and compiler benchmarks
Platform Benchmark ns/op vs base
Linux BenchmarkLookupPCRandom 13.310 ns/op +0.2% (worse)
Linux BenchmarkMergeCompilerFlags 150.800 ns/op -0.2% (better)
Linux BenchmarkMergeLinkerFlags 94.310 ns/op -0.4% (better)
Linux BenchmarkChannelBuffered 33.650 ns/op -0.2% (better)
Linux BenchmarkChannelHandoff 26707 ns/op -1.2% (better)
Linux BenchmarkDefer 49.230 ns/op +0.0%
Linux BenchmarkDirectCall 1.556 ns/op +0.0%
Linux BenchmarkGlobalRead 1.557 ns/op +0.1% (worse)
Linux BenchmarkGlobalWrite 2.477 ns/op -0.1% (better)
Linux BenchmarkGoroutine 41302 ns/op +32.1% (worse)
Linux BenchmarkInterfaceCall 8.095 ns/op -0.0% (better)
Linux BenchmarkRuntimeGetG 2.181 ns/op +0.0% (worse)
macOS BenchmarkLookupPCRandom 12.800 ns/op -2.2% (better)
macOS BenchmarkMergeCompilerFlags 167.600 ns/op +30.6% (worse)
macOS BenchmarkMergeLinkerFlags 91.760 ns/op +4.6% (worse)
macOS BenchmarkChannelBuffered 24.260 ns/op -29.0% (better)
macOS BenchmarkChannelHandoff 7557 ns/op -24.2% (better)
macOS BenchmarkDefer 29.710 ns/op -25.7% (better)
macOS BenchmarkDirectCall 1.088 ns/op -2.1% (better)
macOS BenchmarkGlobalRead 1.292 ns/op -4.0% (better)
macOS BenchmarkGlobalWrite 1.179 ns/op -3.6% (better)
macOS BenchmarkGoroutine 28837 ns/op -49.7% (better)
macOS BenchmarkInterfaceCall 4.957 ns/op -6.0% (better)
macOS BenchmarkRuntimeGetG 2.502 ns/op -14.4% (better)

Compared with 6670dae3884d measured in the same runner job.

@MeteorsLiu
MeteorsLiu force-pushed the fix/goroot-bug388-diagnostics branch from 5446982 to ee982ba Compare August 5, 2026 04:40
@MeteorsLiu
MeteorsLiu force-pushed the fix/goroot-bug388-diagnostics branch from ee982ba to 5787cd3 Compare August 5, 2026 07:19
@MeteorsLiu MeteorsLiu changed the title fix: suppress type errors after syntax errors test/goroot: remove stale bug388 expectation Aug 5, 2026
@MeteorsLiu MeteorsLiu closed this Aug 5, 2026
@MeteorsLiu

Copy link
Copy Markdown
Contributor Author

Superseded by #2265, which now includes the fixedbugs/bug388.go expectation cleanup together with the filtered go list diagnostic handling.

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.

1 participant