From 7bd97c9615fb99f1d70e65ce85eeb3abba820500 Mon Sep 17 00:00:00 2001 From: itsfuad Date: Sun, 4 Oct 2026 00:22:13 +0600 Subject: [PATCH 1/6] Fix atomic object-cache publication Stage managed objects on the cache filesystem and publish without moving existing cache entries aside. Reuse a competing publisher's complete object when replacement fails. Scope staging cleanup to each object compilation. Add concurrency, failure-cleanup, cross-filesystem, and cache-reuse regressions, and run native cache tests in the GitHub Actions matrix. --- .github/workflows/ci.yml | 4 + cmd/build.go | 79 +++--- cmd/build.go.bugfix.md | 55 ++++ cmd/build_linux_test.go | 49 ++++ cmd/build_test.go | 293 ++++++++++++++++++++++ cmd/build_windows_test.go | 67 +++++ x_test/cache_linux_test.go | 122 +++++++++ x_test/managed_object_cache/peeper.toml | 6 + x_test/managed_object_cache/src/main.peep | 8 + 9 files changed, 655 insertions(+), 28 deletions(-) create mode 100644 cmd/build.go.bugfix.md create mode 100644 cmd/build_linux_test.go create mode 100644 cmd/build_windows_test.go create mode 100644 x_test/cache_linux_test.go create mode 100644 x_test/managed_object_cache/peeper.toml create mode 100644 x_test/managed_object_cache/src/main.peep diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9722ba36..7d6f3c3f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -136,6 +136,10 @@ jobs: run: bash scripts/build.sh - name: Diagnose installation run: "build/bin/peeper${{ matrix.exe }} doctor --json" + - name: Test native object-cache publication + env: + CCACHE_DISABLE: "1" + run: go test -count=1 -run '^TestCompileObject' -v ./cmd - name: Run source fixtures env: PEEPER_BIN: ${{ github.workspace }}/build/bin/peeper${{ matrix.exe }} diff --git a/cmd/build.go b/cmd/build.go index 05283c50..856e862a 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -84,36 +84,10 @@ func buildExecutable(ctx *project.CompilerContext, entry *module.Module, outputP if module == nil { continue } - ir := strings.TrimSpace(module.LLVMIR) - if ir == "" { - return fmt.Errorf("empty LLVM IR for module %s", module.ID.ImportPath) - } - cachePath := objectCachePath(ctx, profile, ir) - if cachePath != "" { - if _, err := os.Stat(cachePath); err == nil { - objectPaths = append(objectPaths, cachePath) - continue - } else if !os.IsNotExist(err) { - return fmt.Errorf("inspect cached object for %s: %w", module.ID.ImportPath, err) - } - } - llPath := filepath.Join(artifactDir, fmt.Sprintf("mod_%d.ll", i)) - if err := os.WriteFile(llPath, []byte(ir), 0o644); err != nil { - return fmt.Errorf("write llvm ir: %w", err) - } - objectPath := filepath.Join(artifactDir, fmt.Sprintf("mod_%d.o", i)) - if err := runCompilerTool(profile.ClangPath, profile.ObjectArgs(llPath, objectPath, ctx.Config.IsDebugBuild), "compile LLVM module "+module.ID.ImportPath); err != nil { + objectPath, err := compileObject(ctx, profile, module, filepath.Join(artifactDir, fmt.Sprintf("mod_%d", i))) + if err != nil { return err } - if cachePath != "" { - if err := os.MkdirAll(filepath.Dir(cachePath), 0o755); err != nil { - return fmt.Errorf("create object cache directory: %w", err) - } - if err := os.Rename(objectPath, cachePath); err != nil { - return fmt.Errorf("publish cached object: %w", err) - } - objectPath = cachePath - } objectPaths = append(objectPaths, objectPath) } if len(objectPaths) == 0 { @@ -144,6 +118,55 @@ func buildExecutable(ctx *project.CompilerContext, entry *module.Module, outputP return replacePath(stagedPath, outputPath) } +// compileObject owns one module's cache staging lifetime, ending before linking. +func compileObject(ctx *project.CompilerContext, profile toolchain.Profile, module *module.Module, objectBase string) (string, error) { + ir := strings.TrimSpace(module.LLVMIR) + if ir == "" { + return "", fmt.Errorf("empty LLVM IR for module %s", module.ID.ImportPath) + } + cachePath := objectCachePath(ctx, profile, ir) + if cachePath != "" { + if _, err := os.Stat(cachePath); err == nil { + return cachePath, nil + } else if !os.IsNotExist(err) { + return "", fmt.Errorf("inspect cached object for %s: %w", module.ID.ImportPath, err) + } + } + llPath := objectBase + ".ll" + if err := os.WriteFile(llPath, []byte(ir), 0o644); err != nil { + return "", fmt.Errorf("write llvm ir: %w", err) + } + objectPath := objectBase + ".o" + if cachePath != "" { + cacheDir := filepath.Dir(cachePath) + if err := os.MkdirAll(cacheDir, 0o755); err != nil { + return "", fmt.Errorf("create object cache directory: %w", err) + } + // Clang must finish on the destination filesystem before publication. + stageDir, err := os.MkdirTemp(cacheDir, "."+filepath.Base(cachePath)+"-stage-*") + if err != nil { + return "", fmt.Errorf("stage cached object for %s: %w", module.ID.ImportPath, err) + } + defer os.RemoveAll(stageDir) + objectPath = filepath.Join(stageDir, filepath.Base(cachePath)) + } + if err := runCompilerTool(profile.ClangPath, profile.ObjectArgs(llPath, objectPath, ctx.Config.IsDebugBuild), "compile LLVM module "+module.ID.ImportPath); err != nil { + return "", err + } + if cachePath != "" { + // Never move a published object aside: another linker may be opening it. + if err := os.Rename(objectPath, cachePath); err != nil { + // A competing build may have won this key. Reuse its complete object + // when replacement is blocked (e.g. an open file on Windows). + if info, statErr := os.Stat(cachePath); statErr != nil || !info.Mode().IsRegular() { + return "", fmt.Errorf("publish cached object: %w", err) + } + } + return cachePath, nil + } + return objectPath, nil +} + func objectCachePath(ctx *project.CompilerContext, profile toolchain.Profile, ir string) string { if ctx == nil || !profile.IsManaged || ctx.Config.RootDir == "" { return "" diff --git a/cmd/build.go.bugfix.md b/cmd/build.go.bugfix.md new file mode 100644 index 00000000..375be697 --- /dev/null +++ b/cmd/build.go.bugfix.md @@ -0,0 +1,55 @@ +# Bugfix: object-cache publication + +Source reports: conversation code review P1/P2 and follow-up validation; +`docs/end-to-end-code-review.md`, finding 3. + +Date: 2026-10-03 +Mode: applied directly + +## Results + +1. **P1 — Non-atomic backup-first cache publication — Fixed.** + `compileObject` stages compiler output on the cache filesystem, then publishes + with direct `os.Rename`. Existing cache paths are never moved aside. When + replacement fails, another publisher's regular cache object can be reused; + without one, the original rename error is returned with `%w`. +2. **P2 — Missing production failure-cleanup coverage — Fixed.** + Compiler/publication failure tests exercise the production object lifecycle, + assert errors and stage cleanup, and verify incomplete output stays hidden. + Cross-device probes require `EXDEV`. A bundled Peeper fixture tests real + managed compilation/linking across filesystems and subsequent cache reuse. + +## Evidence and validation + +- Before fix: Linux filesystem-event test detected published cache object moved + away; concurrent reader observed missing file. Both pass after fix. +- Focused `cmd` and `internal/toolchain` tests pass, including cache hits, + compiler failures, publication failures, conflicting directories, winner + reuse, output permissions, and per-object cleanup. +- `go test -race -count=1 -run '^TestCompileObject' ./cmd` passes. +- Full `go test -count=1 ./...`, `go vet ./...`, fresh bundle, complete executable + `x_test` suite, formatting check, and `git diff --check` pass. +- Windows/amd64 and macOS/arm64 test binaries cross-compile. Native execution + remains unverified here. Existing GitHub Actions native matrix now runs + `TestCompileObject` on all six hosts, including Windows locked-reader test. + +## Files changed + +- `.github/workflows/ci.yml` +- `cmd/build.go` +- `cmd/build_test.go` +- `cmd/build_linux_test.go` +- `cmd/build_windows_test.go` +- `x_test/cache_linux_test.go` +- `x_test/managed_object_cache/peeper.toml` +- `x_test/managed_object_cache/src/main.peep` +- This summary and ignored `review-followups.localplan.md`. + +## Rules check + +`compileObject` replaces existing loop/staging logic and owns a real per-object +cleanup lifetime. It has a current production caller. Obsolete `stageCachedObject` +removed. No pass-through wrapper, stale alias, ignored parameter, duplicated +cache decision, or production test hook added. Cache keys, compiler arguments, +diagnostics, error identity, and permissions preserved; publication and staging +lifetime intentionally repaired. diff --git a/cmd/build_linux_test.go b/cmd/build_linux_test.go new file mode 100644 index 00000000..887d27bc --- /dev/null +++ b/cmd/build_linux_test.go @@ -0,0 +1,49 @@ +package main + +import ( + "encoding/binary" + "errors" + "os" + "path/filepath" + "strings" + "syscall" + "testing" +) + +func TestCompileObjectNeverMovesPublishedCache(t *testing.T) { + ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) + cachePath := objectCachePath(ctx, profile, strings.TrimSpace(mod.LLVMIR)) + if err := os.MkdirAll(filepath.Dir(cachePath), 0o755); err != nil { + t.Fatal(err) + } + watch, err := syscall.InotifyInit1(syscall.IN_CLOEXEC | syscall.IN_NONBLOCK) + if err != nil { + t.Fatal(err) + } + defer syscall.Close(watch) + if _, err := syscall.InotifyAddWatch(watch, filepath.Dir(cachePath), syscall.IN_MOVED_FROM|syscall.IN_DELETE); err != nil { + t.Fatal(err) + } + // The fixture publishes a winner after our cache miss, before our publication. + // Watching directory events catches even a gap too short for polling readers. + t.Setenv("PEEPER_TEST_OBJECT_MODE", "winner") + if _, err := compileObject(ctx, profile, mod, base); err != nil { + t.Fatal(err) + } + var events [65536]byte + n, err := syscall.Read(watch, events[:]) + if errors.Is(err, syscall.EAGAIN) { + return + } + if err != nil { + t.Fatal(err) + } + for offset := 0; offset < n; { + nameLen := int(binary.NativeEndian.Uint32(events[offset+12 : offset+16])) + name := strings.TrimRight(string(events[offset+16:offset+16+nameLen]), "\x00") + if name == filepath.Base(cachePath) { + t.Fatalf("published cache object was moved away or deleted: %s", name) + } + offset += syscall.SizeofInotifyEvent + nameLen + } +} diff --git a/cmd/build_test.go b/cmd/build_test.go index e8fb13b8..ebe08376 100644 --- a/cmd/build_test.go +++ b/cmd/build_test.go @@ -1,12 +1,111 @@ package main import ( + "errors" + "fmt" "os" + "os/exec" "path/filepath" "runtime" + "strings" + "sync" + "syscall" "testing" + "time" + + "compiler/internal/module" + "compiler/internal/moduleid" + "compiler/internal/project" + "compiler/internal/target" + "compiler/internal/toolchain" ) +// Run the test executable as a controlled compiler subprocess without shell or +// production hooks. Normal test invocations still run through testing.M. +func TestMain(m *testing.M) { + if os.Getenv("PEEPER_TEST_OBJECT_TOOL") == "1" && len(os.Args) > 1 && os.Args[1] == "-target" { + if err := objectCompilerFixture(); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(23) + } + os.Exit(0) + } + os.Exit(m.Run()) +} + +func objectCompilerFixture() error { + inputPath, outputPath := os.Args[len(os.Args)-3], os.Args[len(os.Args)-1] + ir, err := os.ReadFile(inputPath) + if err != nil { + return err + } + if err := os.WriteFile(outputPath, []byte("partial"), 0o640); err != nil { + return err + } + if err := os.WriteFile(inputPath+".trace", []byte(outputPath), 0o600); err != nil { + return err + } + cachePath := os.Getenv("PEEPER_TEST_OBJECT_CACHE") + mode := os.Getenv("PEEPER_TEST_OBJECT_MODE") + switch mode { + case "compiler-failure": + return fmt.Errorf("fixture compiler failed after partial output") + case "wait": + if err := waitForObjectFile(inputPath + ".release"); err != nil { + return err + } + case "directory-collision": + if err := os.Mkdir(cachePath, 0o755); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(cachePath, "keep"), []byte("keep"), 0o600); err != nil { + return err + } + case "winner", "winner-missing-output": + // Another compiler publishes the same key after our initial cache miss. + winner := outputPath + ".winner" + if err := os.WriteFile(winner, append([]byte("object:"), ir...), 0o640); err != nil { + return err + } + if err := os.Rename(winner, cachePath); err != nil { + return err + } + } + if mode == "missing-output" || mode == "winner-missing-output" { + return os.Remove(outputPath) + } + return os.WriteFile(outputPath, append([]byte("object:"), ir...), 0o640) +} + +func waitForObjectFile(path string) error { + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(path); err == nil { + return nil + } else if !os.IsNotExist(err) { + return err + } + time.Sleep(time.Millisecond) + } + return fmt.Errorf("timed out waiting for %s", path) +} + +func objectCompileFixture(t *testing.T, root string) (*project.CompilerContext, toolchain.Profile, *module.Module, string) { + t.Helper() + executable, err := os.Executable() + if err != nil { + t.Fatal(err) + } + host := target.Host() + ctx := &project.CompilerContext{Config: project.Config{RootDir: root, IsDebugBuild: true}} + profile := toolchain.Profile{IsManaged: true, ClangPath: executable, TargetOS: host.OS, TargetArch: host.Arch, LLVMTriple: host.LLVMTriple, DebugFormat: "dwarf"} + mod := &module.Module{ID: moduleid.ID{Origin: "local", ImportPath: "cache_test"}, LLVMIR: "\n define i32 @main() { ret i32 0 } \n"} + t.Setenv("PEEPER_TEST_OBJECT_TOOL", "1") + t.Setenv("PEEPER_TEST_OBJECT_MODE", "") + t.Setenv("PEEPER_TEST_OBJECT_CACHE", objectCachePath(ctx, profile, strings.TrimSpace(mod.LLVMIR))) + return ctx, profile, mod, filepath.Join(t.TempDir(), "mod_0") +} + func TestValidateNativeLinkTarget(t *testing.T) { if err := validateNativeLinkTarget(runtime.GOOS, runtime.GOARCH); err != nil { t.Fatalf("host target rejected: %v", err) @@ -45,3 +144,197 @@ func TestReplacePathReplacesExistingFile(t *testing.T) { t.Fatalf("staged path still exists: %v", err) } } + +func TestCompileObjectLifecycle(t *testing.T) { + for _, mode := range []string{"success", "unmanaged", "cache-hit", "compiler-failure", "missing-output", "directory-collision", "winner", "winner-missing-output"} { + t.Run(mode, func(t *testing.T) { + ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) + if mode == "unmanaged" { + profile.IsManaged = false + } + cachePath := objectCachePath(ctx, profile, strings.TrimSpace(mod.LLVMIR)) + want := "object:" + strings.TrimSpace(mod.LLVMIR) + t.Setenv("PEEPER_TEST_OBJECT_MODE", mode) + if mode == "cache-hit" { + if err := os.MkdirAll(filepath.Dir(cachePath), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cachePath, []byte(want), 0o640); err != nil { + t.Fatal(err) + } + t.Setenv("PEEPER_TEST_OBJECT_MODE", "compiler-failure") + } + objectPath, err := compileObject(ctx, profile, mod, base) + switch mode { + case "compiler-failure": + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 23 || !strings.Contains(err.Error(), "compile LLVM module cache_test") || !strings.Contains(err.Error(), "fixture compiler failed") { + t.Fatalf("compiler error = %v, want wrapped exit 23 with module and tool output", err) + } + case "missing-output", "directory-collision": + var linkErr *os.LinkError + if !errors.As(err, &linkErr) || !strings.Contains(err.Error(), "publish cached object") { + t.Fatalf("publish error = %v, want wrapped LinkError", err) + } + if mode == "missing-output" && !errors.Is(err, os.ErrNotExist) { + t.Fatalf("publish error lost missing-file identity: %v", err) + } + default: + if err != nil { + t.Fatal(err) + } + wantPath := cachePath + if mode == "unmanaged" { + wantPath = base + ".o" + } + if objectPath != wantPath { + t.Fatalf("object path = %q, want %q", objectPath, wantPath) + } + data, err := os.ReadFile(objectPath) + if err != nil || string(data) != want { + t.Fatalf("object = %q, %v; want %q", data, err, want) + } + info, err := os.Stat(objectPath) + if err != nil { + t.Fatal(err) + } + if runtime.GOOS != "windows" && info.Mode().Perm() != 0o640 { + t.Fatalf("object mode = %o, want 640", info.Mode().Perm()) + } + } + if mode == "cache-hit" { + if _, err := os.Stat(base + ".ll"); !os.IsNotExist(err) { + t.Fatalf("cache hit wrote LLVM input: %v", err) + } + return + } + trace, err := os.ReadFile(base + ".ll.trace") + if err != nil { + t.Fatal(err) + } + if mode == "unmanaged" { + if string(trace) != base+".o" { + t.Fatalf("uncached compiler output = %q", trace) + } + return + } + stageDir := filepath.Dir(string(trace)) + if filepath.Dir(stageDir) != filepath.Dir(cachePath) { + t.Fatalf("stage %q not on cache filesystem beside %q", stageDir, cachePath) + } + if _, err := os.Stat(stageDir); !os.IsNotExist(err) { + t.Fatalf("compileObject left stage directory: %v", err) + } + entries, err := os.ReadDir(filepath.Dir(cachePath)) + if err != nil { + t.Fatal(err) + } + if mode == "compiler-failure" || mode == "missing-output" { + if len(entries) != 0 { + t.Fatalf("failed compilation/publication left cache entries: %v", entries) + } + } else if len(entries) != 1 || entries[0].Name() != filepath.Base(cachePath) { + t.Fatalf("unexpected cache entries: %v", entries) + } + if mode == "directory-collision" { + if data, err := os.ReadFile(filepath.Join(cachePath, "keep")); err != nil || string(data) != "keep" { + t.Fatalf("publish failure damaged destination: %q, %v", data, err) + } + } + }) + } +} + +func TestCompileObjectConcurrentReaders(t *testing.T) { + ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) + t.Setenv("PEEPER_TEST_OBJECT_MODE", "wait") + cachePath := objectCachePath(ctx, profile, strings.TrimSpace(mod.LLVMIR)) + var compilers sync.WaitGroup + results := []chan error{make(chan error, 1), make(chan error, 1)} + for i := range results { + compilers.Add(1) + go func() { + defer compilers.Done() + _, err := compileObject(ctx, profile, mod, fmt.Sprintf("%s_%d", base, i)) + results[i] <- err + }() + } + t.Cleanup(func() { + for i := range results { + _ = os.WriteFile(fmt.Sprintf("%s_%d.ll.release", base, i), nil, 0o600) + } + compilers.Wait() + }) + for i := range results { + if err := waitForObjectFile(fmt.Sprintf("%s_%d.ll.trace", base, i)); err != nil { + t.Fatal(err) + } + } + if _, err := os.Stat(cachePath); !os.IsNotExist(err) { + t.Fatalf("partial compiler output visible in cache: %v", err) + } + if err := os.WriteFile(base+"_0.ll.release", nil, 0o600); err != nil { + t.Fatal(err) + } + if err := <-results[0]; err != nil { + t.Fatal(err) + } + stopReader := make(chan struct{}) + readerDone := make(chan error, 1) + go func() { + for { + data, err := os.ReadFile(cachePath) + if err != nil || string(data) != "object:"+strings.TrimSpace(mod.LLVMIR) { + readerDone <- fmt.Errorf("cache reader observed %q, %v", data, err) + return + } + select { + case <-stopReader: + readerDone <- nil + return + default: + } + } + }() + t.Cleanup(func() { + close(stopReader) + if err := <-readerDone; err != nil { + t.Error(err) + } + }) + if err := os.WriteFile(base+"_1.ll.release", nil, 0o600); err != nil { + t.Fatal(err) + } + if err := <-results[1]; err != nil { + t.Fatal(err) + } +} + +func TestCompileObjectAcrossFilesystems(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("cross-filesystem fixture requires Linux /dev/shm") + } + cacheRoot, err := os.MkdirTemp("/dev/shm", "peeper-object-cache-") + if err != nil { + t.Skipf("/dev/shm unavailable: %v", err) + } + defer os.RemoveAll(cacheRoot) + + ctx, profile, mod, base := objectCompileFixture(t, cacheRoot) + sourcePath := base + ".probe" + if err := os.WriteFile(sourcePath, []byte("object"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Rename(sourcePath, filepath.Join(cacheRoot, "probe")); err == nil { + t.Skip("test temporary directory and /dev/shm share a filesystem") + } else if !errors.Is(err, syscall.EXDEV) { + t.Fatalf("cross-filesystem probe = %v, want EXDEV", err) + } + objectPath, err := compileObject(ctx, profile, mod, base) + if err != nil { + t.Fatal(err) + } + if data, err := os.ReadFile(objectPath); err != nil || string(data) != "object:"+strings.TrimSpace(mod.LLVMIR) { + t.Fatalf("cached object = %q, %v", data, err) + } +} diff --git a/cmd/build_windows_test.go b/cmd/build_windows_test.go new file mode 100644 index 00000000..289a9096 --- /dev/null +++ b/cmd/build_windows_test.go @@ -0,0 +1,67 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "syscall" + "testing" +) + +func TestCompileObjectReusesLockedWinner(t *testing.T) { + ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) + t.Setenv("PEEPER_TEST_OBJECT_MODE", "wait") + cachePath := objectCachePath(ctx, profile, strings.TrimSpace(mod.LLVMIR)) + result := make(chan error, 1) + done := make(chan struct{}) + go func() { + defer close(done) + _, err := compileObject(ctx, profile, mod, base) + result <- err + }() + t.Cleanup(func() { + _ = os.WriteFile(base+".ll.release", nil, 0o600) + <-done + }) + if err := waitForObjectFile(base + ".ll.trace"); err != nil { + t.Fatal(err) + } + // Model a linker holding a competing publisher's complete object without + // FILE_SHARE_DELETE. Windows must reject replacing this open destination. + want := "object:" + strings.TrimSpace(mod.LLVMIR) + if err := os.WriteFile(cachePath, []byte(want), 0o640); err != nil { + t.Fatal(err) + } + name, err := syscall.UTF16PtrFromString(cachePath) + if err != nil { + t.Fatal(err) + } + handle, err := syscall.CreateFile(name, syscall.GENERIC_READ, syscall.FILE_SHARE_READ, nil, syscall.OPEN_EXISTING, syscall.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + t.Fatal(err) + } + defer syscall.CloseHandle(handle) + probe := filepath.Join(filepath.Dir(cachePath), "replacement-probe.o") + if err := os.WriteFile(probe, []byte(want), 0o640); err != nil { + t.Fatal(err) + } + if err := os.Rename(probe, cachePath); err == nil { + t.Fatal("replacement probe succeeded despite locked destination") + } + if err := os.Remove(probe); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(base+".ll.release", nil, 0o600); err != nil { + t.Fatal(err) + } + if err := <-result; err != nil { + t.Fatalf("failed to reuse locked winner: %v", err) + } + if data, err := os.ReadFile(cachePath); err != nil || string(data) != want { + t.Fatalf("winner changed: %q, %v", data, err) + } + entries, err := os.ReadDir(filepath.Dir(cachePath)) + if err != nil || len(entries) != 1 || entries[0].Name() != filepath.Base(cachePath) { + t.Fatalf("duplicate stage not cleaned: %v, %v", entries, err) + } +} diff --git a/x_test/cache_linux_test.go b/x_test/cache_linux_test.go new file mode 100644 index 00000000..8238c056 --- /dev/null +++ b/x_test/cache_linux_test.go @@ -0,0 +1,122 @@ +package xtest_test + +import ( + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "syscall" + "testing" + + "compiler/internal/target" + "compiler/internal/toolchain" + "compiler/pkg/peeper" +) + +func TestManagedObjectCacheFixture(t *testing.T) { + binary := os.Getenv("PEEPER_BIN") + if binary == "" { + t.Skip("PEEPER_BIN not set; managed cache fixture requires bundled compiler") + } + clang, err := exec.LookPath("clang") + if err != nil { + t.Fatal(err) + } + tempRoot, err := os.MkdirTemp("/dev/shm", "peeper-link-fixture-") + if err != nil { + t.Skipf("/dev/shm unavailable: %v", err) + } + defer os.RemoveAll(tempRoot) + projectRoot := t.TempDir() + probe := filepath.Join(tempRoot, "probe.o") + if err := os.WriteFile(probe, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := os.Rename(probe, filepath.Join(projectRoot, "probe.o")); err == nil { + t.Skip("test project and /dev/shm share a filesystem") + } else if !errors.Is(err, syscall.EXDEV) { + t.Fatalf("cross-device probe = %v, want EXDEV", err) + } + if err := os.Remove(probe); err != nil { + t.Fatal(err) + } + if err := os.CopyFS(projectRoot, os.DirFS("managed_object_cache")); err != nil { + t.Fatal(err) + } + installation := t.TempDir() + profileDir := filepath.Join(installation, "toolchains", "native") + for _, dir := range []string{filepath.Join(installation, "bin"), filepath.Join(profileDir, "link")} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + binary, err = filepath.Abs(binary) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(binary) + if err != nil { + t.Fatal(err) + } + installedBinary := filepath.Join(installation, "bin", "peeper") + if err := os.WriteFile(installedBinary, data, 0o755); err != nil { + t.Fatal(err) + } + for _, dir := range []string{"libs", "runtime"} { + if err := os.Symlink(filepath.Join(peeper.InstallationRootForExecutable(binary), dir), filepath.Join(installation, dir)); err != nil { + t.Fatal(err) + } + } + // Preserve argv[0] for Clang wrappers such as ccache. + for _, tool := range []string{"clang", filepath.Join("link", "clang")} { + if err := os.Symlink(clang, filepath.Join(profileDir, tool)); err != nil { + t.Fatal(err) + } + } + host := target.Host() + profile := toolchain.Profile{ + SchemaVersion: toolchain.ProfileSchemaVersion, ProfileID: "cache-fixture", + TargetOS: host.OS, TargetArch: host.Arch, LLVMTriple: host.LLVMTriple, + ClangPath: "toolchains/native/clang", LinkerPath: "toolchains/native/link/clang", + RuntimeArchive: "runtime/libpeeper_rt_v1.a", RuntimeABI: toolchain.RuntimeABIVersion, + LinkMode: "system", DebugFormat: "dwarf", + } + data, err = json.Marshal(profile) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(profileDir, "profile.json"), data, 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("TMPDIR", tempRoot) + t.Setenv("CCACHE_DISABLE", "1") + expectation := readFixtureExpectation(t, filepath.Join("managed_object_cache", "peeper.toml")) + expectation.Dir = projectRoot + runFixture(t, installedBinary, expectation) + objects, err := filepath.Glob(filepath.Join(projectRoot, "build", "*", "*", "artifacts", "*", "*.o")) + if err != nil || len(objects) == 0 { + t.Fatalf("managed build did not publish objects: %v, %v", objects, err) + } + // Keep the profile/key and linker identical, but make object compilation + // unavailable. A successful second build must consume published cache hits. + if err := os.Remove(filepath.Join(profileDir, "clang")); err != nil { + t.Fatal(err) + } + runFixture(t, installedBinary, expectation) + entries, err := os.ReadDir(tempRoot) + if err != nil || len(entries) != 0 { + t.Fatalf("link artifacts leaked: %v, %v", entries, err) + } + for _, object := range objects { + entries, err := os.ReadDir(filepath.Dir(object)) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if entry.IsDir() { + t.Fatalf("cache staging directory leaked: %s", entry.Name()) + } + } + } +} diff --git a/x_test/managed_object_cache/peeper.toml b/x_test/managed_object_cache/peeper.toml new file mode 100644 index 00000000..8ee969a2 --- /dev/null +++ b/x_test/managed_object_cache/peeper.toml @@ -0,0 +1,6 @@ +name = "managed_object_cache" +build = "program" + +[test] +mode = "run" +outcome = "success" diff --git a/x_test/managed_object_cache/src/main.peep b/x_test/managed_object_cache/src/main.peep new file mode 100644 index 00000000..c9fccbf7 --- /dev/null +++ b/x_test/managed_object_cache/src/main.peep @@ -0,0 +1,8 @@ +fn Compute(value: i32) -> i32 { + return value * 3 + 5; +} + +fn main() -> i32 { + if Compute(7) != 26 { return 1; } + return 0; +} From 5dac6e1120e4078a9009752e3ea7494fe2ffe544 Mon Sep 17 00:00:00 2001 From: itsfuad Date: Sun, 4 Oct 2026 01:15:46 +0600 Subject: [PATCH 2/6] Fix Linux cache fixture ABI and reconcile review findings --- cmd/build.go.bugfix.md | 18 +- docs/end-to-end-code-review.md | 804 ++++++++++----------------------- x_test/cache_linux_test.go | 14 +- 3 files changed, 274 insertions(+), 562 deletions(-) diff --git a/cmd/build.go.bugfix.md b/cmd/build.go.bugfix.md index 375be697..912ff2cb 100644 --- a/cmd/build.go.bugfix.md +++ b/cmd/build.go.bugfix.md @@ -30,8 +30,22 @@ Mode: applied directly - Full `go test -count=1 ./...`, `go vet ./...`, fresh bundle, complete executable `x_test` suite, formatting check, and `git diff --check` pass. - Windows/amd64 and macOS/arm64 test binaries cross-compile. Native execution - remains unverified here. Existing GitHub Actions native matrix now runs - `TestCompileObject` on all six hosts, including Windows locked-reader test. + also passed in PR #151 CI run 37146240861, including Windows locked-reader + regression. Linux native cache Go tests passed; Linux integration fixture + runtime exposed the test setup ABI mismatch described below. + +## Linux CI fixture follow-up + +Both Ubuntu architectures linked the synthetic managed fixture for musl with +system Clang, then failed execution because musl interpreter was absent. +Test-local Clang launchers now select `target.SystemLLVMTriple` as final tool +target while leaving installed-profile validation and production code intact. +Real compilation/linking, cross-filesystem assertions, executable checks, and +cache-hit proof remain enabled. + +Targeted fixture, full executable `x_test`, `internal/target` tests, `go vet +./x_test`, formatting, and diff checks pass locally. Clang dry-runs confirm glibc +interpreter selection for amd64 and arm64. Follow-up CI rerun pending publication. ## Files changed diff --git a/docs/end-to-end-code-review.md b/docs/end-to-end-code-review.md index 3296cc4f..81351201 100644 --- a/docs/end-to-end-code-review.md +++ b/docs/end-to-end-code-review.md @@ -1,621 +1,311 @@ -# End-to-End Code Review +# Compiler Review — Completed Work and Remaining Tasks -## Scope - -Review follows live execution paths from CLI/LSP entry points through dependency handling, compiler scheduling, semantic analysis, THIR/MIR lowering, LLVM emission, and user-facing documentation. - -This document preserves original findings and reproduction snippets as baseline evidence. Remediation status below describes current implementation; snippets inside finding sections are historical unless marked otherwise. - -## Remediation status - -| Finding group | Status | Chosen implementation | -|---|---|---| -| Interface conversion evidence | Resolved | Default substitution deep-clones each occurrence with fresh `NodeID`s and separate declaration/caller provenance maps. Semantic metadata copies from owning module; THIR/MIR consume unique per-occurrence interface evidence. | -| LSP diagnostics | Resolved | Compilation snapshots copy context, component files, generation, and versions under state lock. Publication is serialized, rejects stale generations, and writes without state lock. | -| CLI contract and artifacts | Resolved | One command registry drives dispatch/help/aliases. Compiler APIs are LLVM-only. `check` recursively discovers and groups roots by project. `_gen` publishes completed identity-based staging trees. | -| Dependency lifecycle | Resolved | Lock corruption propagates. `get`/`update` prepare in memory and publish coordinated manifest/lock state only after all work succeeds. Metadata commits before cache deletion. | -| Intrinsic and pipeline ownership | Resolved | One intrinsic definition slice drives compiler-owned free-function symbols, contextual discovery, operations, and signatures. Diagnostic-free scheduler completion requires every scheduled module at backend before runtime-symbol validation. | -| Documentation | Resolved | Interface ownership, target-sized carrier fields, README registry claim, and CLI diagrams now follow live owners. | - -This report groups related defects by owning subsystem. Severity reflects impact, not amount of code needed: - -- **P1:** valid input can crash, fail silently, corrupt state, or race. -- **P2:** architecture has multiple authorities, misleading abstractions, or silent failure paths that make extension unsafe. - -| Group | Highest severity | Main risk | -|---|---:|---| -| CLI and dependency lifecycle | P1 | Incorrect success, ignored input, damaged project state | -| Semantic evidence and intrinsic ownership | P1 | Valid program reaches backend with wrong type evidence | -| Compiler pipeline and backend boundary | P2 | Silent incomplete compilation and fake extensibility | -| LSP state and concurrency | P1 | Data race and stale diagnostics | -| Documentation and architectural truth | P2 | Maintainers implement against contracts that no longer exist | - -## 1. CLI and dependency lifecycle - -These defects belong together because they define command behavior and mutation of user-owned project state. - -### 1.1 `check` and `lint` do not implement their advertised input contract - P1 - -Help text says these commands typecheck a file or recursively check a folder. Actual command handling defaults to `.` and passes one path to the single-entry compiler path: - -```go -path := "." -if len(args) > 0 { - path = args[0] -} - -return compileEntry(path, options) -``` - -This has two observable failures: - -- `peeper check .` attempts to read the directory as a source file and fails. -- `peeper check valid.peep invalid.peep` ignores every path after the first and can return success despite invalid requested input. - -Why wrong: - -- Command accepts syntax it does not honor. -- Exit status no longer represents all user-requested work. -- Help text and implementation have different owners for the command contract. - -What should replace it: - -Resolve input shape once, before compilation. Either support all documented forms or reject unsupported forms explicitly. - -```go -type CheckTarget struct { - Files []string -} - -func resolveCheckTarget(args []string) (CheckTarget, error) { - // No args: discover project rooted at current directory. - // One directory: discover project/source files recursively. - // One or more files: validate and retain every file. - // Unsupported mixtures: return a usage error. -} -``` - -Compiler driver should receive resolved files or a project, not reinterpret raw CLI arguments. One owner must define which files form a check operation. - -Acceptance criteria: - -- Directory input checks intended project files. -- Multiple explicit files are all checked, or extra arguments are rejected. -- Any failed requested input produces nonzero exit status. -- Help and argument validation derive from same command definition. - -### 1.2 Corrupt lockfiles are treated as missing lockfiles - P1 - -`manifest.LoadLockfile` already distinguishes a missing file from other read or parse failures. `get` discards that distinction: - -```go -lock, err := manifest.LoadLockfile(projectDir) -if err != nil { - lock = manifest.NewLockfile() -} -``` - -A malformed or unreadable existing lockfile therefore becomes an empty lockfile. Later save can replace evidence needed to reproduce the existing dependency graph. - -Why wrong: - -- Recovery policy is duplicated outside the lockfile owner. -- Corruption becomes implicit destructive recovery. -- User receives no diagnosis before state changes. - -What should replace it: - -Trust `LoadLockfile` to own the missing-file case and propagate every remaining error: - -```go -lock, err := manifest.LoadLockfile(projectDir) -if err != nil { - return fmt.Errorf("load dependency lockfile: %w", err) -} -``` - -If explicit repair is desired, it should be a separately named operation with clear user intent-not normal `get` behavior. - -### 1.3 `update` reports success after dependency installation failures - P1 - -Current loop prints an error and continues: - -```go -if err := installUpdate(pkg); err != nil { - fmt.Fprintf(stderr, "failed to update %s: %v\n", pkg.Name, err) - continue -} -``` - -Command can then print an up-to-date message and exit successfully. A partially failed mutation is indistinguishable to scripts from complete success. - -Why wrong: - -- Process status contradicts emitted diagnostics. -- "No updates available" and "all updates failed" collapse into same result. -- CI and automation cannot safely consume command result. - -What should replace it: - -Collect failures while allowing independent work to continue, then return one failure result: - -```go -var failures []error - -for _, pkg := range updates { - if err := installUpdate(pkg); err != nil { - failures = append(failures, fmt.Errorf("%s: %w", pkg.Name, err)) - continue - } - updated++ -} - -if len(failures) != 0 { - return errors.Join(failures...) -} -``` - -Summary output must separately represent updated, unchanged, and failed counts. - -### 1.4 Dependency mutations are not transactional - P1 - -`get`, `remove`, and `update` modify several related resources: - -- manifest -- lockfile -- package cache/install tree - -These resources are saved or pruned in different orders. For example, pruning can happen before both metadata files are safely persisted. Individual atomic file writes do not make the whole operation atomic. - -Why wrong: - -- Failure between writes leaves manifest and lockfile describing different graphs. -- Early cache pruning can remove data still referenced by durable metadata. -- Retry behavior depends on where failure occurred. - -What should replace it: - -Use one dependency transaction owned by dependency management: - -```go -type DependencyPlan struct { - Manifest manifest.Manifest - Lockfile manifest.Lockfile - Installs []ResolvedPackage - Prunes []PackagePath -} - -func ApplyDependencyPlan(plan DependencyPlan) error { - // 1. Stage downloads and generated metadata. - // 2. Validate complete staged graph. - // 3. Commit manifest and lockfile as one coordinated operation. - // 4. Prune obsolete cache entries only after durable commit. -} -``` - -Exact disk mechanism can vary, but invariant cannot: before commit, old state remains usable; after commit, both metadata files describe same dependency graph. - -### 1.5 Command metadata has multiple authorities - P2 - -Command names, aliases, handlers, help, and backend policy are spread across separate switches and text blocks. Adding one command requires synchronized edits in several files. Alias and help drift already demonstrate this is not theoretical. - -What should replace it: - -One declarative registry should own command identity and dispatch metadata: - -```go -type CommandDefinition struct { - Name string - Aliases []string - Usage string - Run func(CommandContext, []string) error - BackendRule BackendRule -} - -var commands = []CommandDefinition{ - {Name: "check", Aliases: []string{"lint"}, Usage: "check [path...]", Run: runCheck}, - {Name: "build", Usage: "build [path]", Run: runBuild, BackendRule: LLVMOnly}, -} -``` - -Lookup, help generation, alias handling, and validation should derive from this registry. Do not add another wrapper around existing switches; remove split authorities when migrating. - -### 1.6 Generated IR artifacts can collide or remain stale - P2 - -`-keep-gen` uses identity-encoded module paths for generated files. MIR and LLVM artifacts are staged and replaced atomically, so old artifacts do not survive a successful publication. - -What should replace it: - -- Derive artifact path from canonical module/import identity, not basename alone. -- Write into a clean per-build staging directory. -- Publish completed artifacts only after successful generation. - -```text -build/generated/ - app/main.mir - app/main.ll - deps/example/main.mir - deps/example/main.ll -``` - -## 2. Semantic evidence and intrinsic ownership - -These issues concern semantic facts that later compiler phases trust. Such facts need one owner and identity matching the operation they describe. - -### 2.1 Interface-conversion evidence is keyed by expression, not conversion occurrence - P1 - -Typechecker stores interface implementation evidence using source expression `NodeID`: - -```go -tc.project.InterfaceImplementations[expr.ID()] = implementations -``` - -Default-argument substitution can reuse one caller expression for multiple parameters. If two expanded defaults convert that expression to different interfaces, both conversions share the same `NodeID`. Second write overwrites first conversion's slot set. - -THIR/MIR lowering later asks for evidence by expression ID: - -```go -implementations := l.project.InterfaceImplementations[expr.ID()] -``` - -By then target interface context is no longer sufficient to recover overwritten proof. Valid source can reach typed LLVM with wrong aggregate layout and panic, for example: - -```text -call @use argument 1 is i32, want { i8*, i8* } -``` - -Why wrong: - -- Evidence describes a conversion, but key identifies only reused input expression. -- Cache assumes one expression has at most one interface target. -- Later phase receives mutable last-writer state instead of exact semantic result. -- Backend panic is only symptom; ownership error begins in semantic handoff. - -What should replace it: - -Best model: make conversion explicit in typed semantic output so evidence travels with its occurrence: - -```go -type InterfaceConversion struct { - Expr ast.Expr - Target typeinfo.Type - Implementations []project.InterfaceImplementation -} -``` - -If current IR cannot represent explicit conversion nodes yet, use a conversion-site key that cannot collide across expanded arguments: +## Review boundary -```go -type ConversionSite struct { - ExpressionID ast.NodeID - CallID ast.NodeID - Parameter int +This document is a fresh-start work list built from the earlier whole-codebase +review and `final-code-review.md`. It is not a new whole-codebase audit. Each +finding below was checked against the current source before being listed as +open, resolved, or documentation-only. + +Current checkout used for this update: + +- branch: `main` +- HEAD: `ee3be84` (`Merge pull request #148 from PeeperLanguage/fix/shared-reference-projections`) +- worktree: clean +- production source/fixture changes, resets, commits, pushes, and GitHub + tracking: none + +The earlier note about uncommitted candidates in +`internal/ir/mir/expr_lower.go` and `internal/backend/llvm/instruction_emit.go` +and the associated rvalue/shift tests and fixtures does not reproduce in this +checkout. No files were removed to establish this baseline. + +Historical runtime results remain historical evidence. This update checked +current source, tests, fixtures, workflows, and documentation paths; it did +not claim to rerun the historical end-to-end reproductions. + +## Completed and revalidated work + +- [x] **Shared-reference and owner mutability:** committed in `1ae82933`. + Owning pointers do not independently grant mutation; immutable owners and + enclosing shared references cannot grant write access. Mutable owners and + `&mut` remain valid. Moving, freeing, and dropping an immutable owner remain + consumption operations and do not require `mut`. +- [x] **Nested/deferred semantic cases:** `1ae82933` covers optional reference + permissions, qualified constant mutable-borrow rejection, deferred + enum-field assignment compatibility, and durable late conversions consumed + by lowering. Go regressions and Peeper fixtures are included in that work. +- [x] **Artifact and AST cleanup:** `58a60595` and `76fbabc2` hardened + symbol-index snapshots and declaration ordering, sealed cloning, removed + unused `ast.Index`, and strengthened clone identity/isolation tests. +- [x] **LSP rename snapshot consistency:** rename uses one captured compiler + context instead of rereading `LastCtx` during background replacement. +- [x] **Transitive lockfile edge selection:** `71bdeef` records the recursive + resolver's selected package ID on parent edges. Regression covers competing + versions and persisted lockfiles. +- [x] **Structural type identity and layout:** `fbdbb52b` makes anonymous + struct field order part of identity and keeps named structs nominal. Distinct + struct types no longer convert; contextual literals still lower in + declaration order. +- [x] **THIR lookup experiment:** reverted. `Module.Node` uses its + module-owned index rather than repeated function scans. +- [x] **Lockfile direct-alias consistency:** current + `pkg/manifest/lockfile.go:SetDirectDependency` publishes the replacement + alias before demoting the old package and checks `isStillDirect` first. + Current tests cover one-alias demotion and preserving a package referenced + by another alias. This item is resolved in the current source; it is not an + open implementation task. +- [x] **CI toolchain classifier coverage:** current + `scripts/detect-changes.sh` includes `cmd/release-profile/*` in the + toolchain classification. The earlier omission is resolved. + +## Remaining compiler correctness work + +### 1. High — Capture rvalues before later operand side effects + +**Status:** open. Current source confirms the defect shape; no candidate is +present in this checkout. + +**Owner:** `internal/ir/mir/expr_lower.go`, with the source-to-IR place/value +boundary in `internal/ir/exprlower/lower.go`. + +`exprlower.LowerIdent` currently produces `ir.Ident`. MIR lowering of an +identifier produces a `RefName`, while LLVM `emitRef` loads that name when it +is emitted. A call argument or binary operand can therefore retain storage +identity until a later nested operand has already mutated the storage. MIR +lowering also emits nested call side effects while constructing the outer +call's arguments. + +Historical reproduction: + +```peep +#[extern("memset")] +fn Memset(p: rawptr, value: i32, size: usize) -> rawptr; +fn Clear(p: rawptr) -> i32 { Memset(p, 0, 4); return 0; } +fn Sum(a: i32, b: i32) -> i32 { return a + b; } + +fn main() { + let mut x: i32 = 7; + println(Sum(x, Clear(@x))); } - -type InterfaceEvidence map[ConversionSite][]project.InterfaceImplementation ``` -Key requirement: evidence identity must include conversion occurrence and target context. Adding target type alone may still be insufficient if same expression converts to same interface at separate sites with distinct ownership/lifetime facts later. +Historical result was `0`; expected result under left-to-right evaluation is +`7`. The same shape was reproduced with `println(x + Clear(@x))`. -Required regression fixture: +Required result: -```peeper -interface ReadA { - read_a(self: &Self) -> i32 -} - -interface ReadB { - read_b(self: &Self) -> i32 -} +- materialize rvalue reads at their evaluation point using existing MIR + mechanisms; +- retain storage identity for assignment, address, and borrow destinations; +- do not blindly snapshot place roots; +- distinguish MIR instruction behavior from language-level ownership + consumption. -fn use[T](x: T, a: &ReadA = x, b: &ReadB = x) { - // Both defaults reuse x but require distinct interface evidence. -} +Regression coverage must include call arguments, binary operands, aggregate +elements, index evaluation, preserved writes and borrows, and existing +move/drop/ownership behavior. The historical candidate paths +`internal/ir/mir/expr_lower_order_test.go`, +`x_test/runtime_mir_rvalue_eval_order_call_capture/`, and +`x_test/runtime_mir_rvalue_eval_order_binary_capture/` are absent here. -use(&c) -``` +### 2. High — Make narrow shift guards width-safe -Fixture must compile and execute through bundled `build/bin/peeper`, not stop at Go unit coverage. +**Status:** open. Current source still contains the lossy guard. -### 2.2 Predeclared intrinsic registry still has two authorities - P2 +**Owner:** `internal/backend/llvm/instruction_emit.go`. -One list says which operations are predeclared: +Current shift lowering computes operand width, then compares `right` against +`bits` using `right.Layout`: ```go -var predeclaredOperations = []symbols.CompilerOp{ - // operation identities -} +invalid := b.compare("icmp", "uge", right, + b.value(strconv.Itoa(bits), right.Layout)) ``` -A separate switch says which signatures those operations have: +For a narrow count such as `u5`, the bound `32` is not representable in the +count type. LLVM therefore receives an invalid or wrapped bound instead of a +comparison in a type that can represent both values. The later cast to the +shift operand type happens only after this guard. -```go -switch op { -case symbols.SomeOperation: - return someSignature(target) -// ... -default: - panic("missing intrinsic signature") -} -``` - -Adding an operation requires editing both. Missing one edit either omits symbol exposure or panics during predeclared-scope initialization. - -Why wrong: - -- Registry is unified by API name, not by data ownership. -- Startup correctness depends on synchronized declarations. -- Extension failure occurs far from operation declaration. - -What should replace it: - -One definition must contain identity, exposure, and signature factory: - -```go -type intrinsicDefinition struct { - Operation symbols.CompilerOp - Name string - Predeclared bool - Receiver receiverShape - Signature func(target.Info) *typeinfo.FuncType -} +Historical reproduction: -var intrinsicDefinitions = []intrinsicDefinition{ - { - Operation: symbols.SomeOperation, - Name: "some_operation", - Predeclared: true, - Signature: someOperationSignature, - }, -} +```peep +fn Shift(count: u5) -> u32 { return 1u32 << count; } +fn main() { println(Shift(1u5)); } ``` -`PredeclaredSymbols`, symbol lookup, and operation enumeration should derive from this same collection. A consistency test is useful defense, but it does not replace removing duplicate authority. - -## 3. Compiler pipeline and backend boundary - -These problems concern phase scheduling and ownership after semantic analysis. +Required result: validate signed negative and oversized counts in a width that +can represent the count and operand-width bound, then convert to the shift +operand width only after validation. Cover both directions, signed and +unsigned counts, zero and highest valid counts, the exact bound, wide counts +that would truncate to valid values, odd widths, and 32-/64-bit target +layouts. Historical candidate files are absent here. -### 3.1 Pipeline scheduler can stop without reporting incomplete modules - P2 +### 3. Medium — Publish cached objects across filesystems safely -Scheduler breaks when no module is ready or no phase advances. It can then return without proving every requested module reached backend completion. +**Status:** open. Current source still stages in system temporary storage and +renames across filesystems. -Conceptually, current control flow is: - -```go -for { - ready := findReadyModules() - if len(ready) == 0 { - break - } - - advanced := advance(ready) - if !advanced { - break - } -} +**Owner:** `cmd/build.go`. -return nil -``` +`buildExecutable` creates `artifactDir` with `os.MkdirTemp("", ...)`, then +publishes each compiled object with `os.Rename(objectPath, cachePath)`. A +managed build whose temporary directory and project cache use different +filesystems can fail with `invalid cross-device link`. -Later build code observes missing/empty LLVM IR, so diagnosis appears after scheduler lost information about blocked phase and prerequisite. +Required result: stage the object on the cache destination filesystem and +atomically publish it there. Incomplete objects must remain invisible to cache +readers. Test differing temporary/cache filesystems and cleanup on compiler or +publish failure. -Why wrong: +### 4. Medium — Preserve negative zero in runtime floating negation -- Loop termination is mistaken for successful compilation. -- Root cause is replaced by downstream empty-artifact failure. -- Adding a phase or dependency edge can silently create an unreported stall. +**Status:** open. Current source still emits `fsub` from positive zero for +floating unary minus. -What should replace it: +**Owner:** `internal/backend/llvm/instruction_emit.go`. -Scheduler must establish terminal invariant before returning success: +The floating branch of unary `-` currently emits: ```go -for _, module := range orderedModules { - if module.Phase != project.PhaseBackend { - return fmt.Errorf( - "pipeline stalled: module %s stopped at %s", - module.Path, - module.Phase, - ) - } -} -``` - -Production diagnostic should include unmet prerequisite or blocking diagnostic when available. Generic invariant error is fallback, not preferred user message. - -### 3.2 Backend abstraction is declared but not owned at one boundary - P2 - -Project state defines LLVM and WASM backend identities, but pipeline imports LLVM directly, module state stores `LLVMIR`, CLI rejects non-LLVM backends, and build consumes LLVM-specific output. - -Current effective flow: - -```text -CLI backend flag - -> project.TargetBackend - -> pipeline ignores abstraction - -> LLVM generator - -> module.LLVMIR - -> clang -``` - -Adding a backend requires edits across CLI, project core, scheduler, module representation, artifact handling, and build toolchain. `TargetBackend` therefore suggests an extension point that does not exist. - -Two valid directions exist: - -1. **Be honestly LLVM-only now.** Remove unsupported backend identities and generic configuration until a second backend is implemented. -2. **Create a real post-MIR backend boundary.** Pipeline selects one backend owner and stores a backend-neutral artifact. - -```go -type Backend interface { - Name() backend.Type - Emit(*mir.Module, target.Info, *diagnostics.DiagnosticBag) (Artifact, error) -} - -type Artifact struct { - Kind backend.Type - Text string -} +b.arithmetic("fsub", b.value("0.0", arg.Layout), arg) ``` -Do not add this interface merely to wrap `GenerateLLVMIR`. It is justified only when pipeline dispatch and artifact ownership actually move behind it. Until then, deleting false generality is simpler and more maintainable. - -### Desired phase ownership - -```text -Parser - produces syntax - | -Typechecker - produces types + conversion evidence - | -THIR/MIR lowering - consumes semantic evidence; does not rediscover it - | -MIR lowering - owns target-independent executable representation - | -Backend boundary - owns target/backend-specific representation and emission - | -CLI build - owns external tool invocation and final artifact placement -``` - -Each arrow is a typed contract. Later phases should not recover facts that an earlier phase already knew, and earlier phases should not store backend-specific output. - -## 4. LSP state and concurrency +This differs from constant folding for signed zero. Historical reproduction +used `1.0f64 / Neg(0.0f64)` and observed `inf` instead of `-inf`. -### 4.1 Diagnostic workers read workspace state outside its lock - P1 +Required result: emit LLVM `fneg` and prove constant/runtime parity for signed +zero in both `f32` and `f64`. -Each changed file can schedule a diagnostic goroutine. Recompile locks shared state only while compilation runs. After unlock, worker computes component files from `state.workspace`. Another worker can rebuild or replace that workspace concurrently. +### 5. Medium — Reconcile floating remainder with the specification -Simplified race: +**Status:** open. Current source is inconsistent across specification, +typechecking, constant folding, and backend emission. -```text -worker A: lock -> compile A -> unlock -worker B: lock -> rebuild workspace -> unlock -worker A: read workspace component files without lock -``` - -Consequences: +`docs/language-spec.md` says floating division and remainder keep IEEE +behavior. LLVM emission already selects `frem` for floating `%`, but +`internal/semantics/typechecker/check_expr.go` accepts `%` only for integral +types, and `internal/constvalue/value.go` has no floating `%` case. -- Go data race on workspace state. -- Diagnostics from one compiler context can be published using file membership from another workspace generation. -- Older worker can overwrite newer diagnostics. +Required result: confirm the intended language rule, then make typing, +constant evaluation, and runtime behavior agree. Do not remove specified +behavior merely to avoid implementation work. Cover floating boundary cases +and constant/runtime parity. -What should replace it: +### 6. Medium — Discover new files in existing empty directories -Create immutable publication snapshot while holding state lock. Release lock before writing protocol output: +**Status:** open. Current source retains the incomplete discovery shortcut. -```go -type diagnosticSnapshot struct { - Context *project.CompilerContext - Files []string - Version uint64 -} +**Owner:** `internal/lsp/workspace.go`. -func (s *ServerState) compileDiagnosticSnapshot(path string) (diagnosticSnapshot, error) { - s.mu.Lock() - defer s.mu.Unlock() +The fast path returns cached module paths when `directoriesUnchanged` is true. +`captureDirectoryStamps` records the workspace root, source directory, and +directories containing discovered files. It does not record every directory +traversed during discovery, including pre-existing empty nested directories. +Creating a source file in one of those directories can therefore leave the +directory stamps unchanged and omit the new file when no open-buffer overlay +exists. - ctx, err := s.recompileLocked(path) - if err != nil { - return diagnosticSnapshot{}, err - } +Required result: track all relevant traversed directories for invalidation, or +remove the incomplete shortcut. Add an unopened-file regression while +preserving existing discovery and incremental behavior. LSP workspace +concurrency checks are required for any implementation change. - return diagnosticSnapshot{ - Context: ctx, - Files: append([]string(nil), s.workspace.componentFiles(path)...), - Version: s.version, - }, nil -} -``` +## Broader repository findings to recheck before fixing -Before publishing, compare snapshot version with latest requested version for each document. This prevents slow old workers from replacing current diagnostics. +### 7. Lockfile alias/direct-dependency consistency -Do not hold state mutex while sending LSP messages. Output can block and should not serialize compilation state. +**Status:** resolved on current source; retained here as revalidated evidence. -Acceptance criteria: +`SetDirectDependency` now uses `isStillDirect` before clearing the previous +package's `IsDirect` flag. Tests cover replacement, shared aliases, and +dependency-edge `UsedBy` rewiring. Any later lockfile work must still audit +forward edges, reverse `UsedBy` edges, direct flags, pruning, and persistence +without inventing another mirrored representation. -- Workspace reads and writes follow one locking policy. -- Publication operates on copied immutable data. -- Stale worker cannot publish over newer document version. -- Race-enabled tests pass in a Go installation with working race runtime. +### 8. Installer release-signature verification -## 5. Documentation and architectural truth +**Status:** open. -Documentation defects are grouped because each describes a contract maintainers may use when extending compiler. +The release workflow currently builds an unsigned manifest and publishes +`SHA256SUMS`. Both `scripts/install.sh` and `scripts/install.ps1` authenticate +the manifest and components only by checksums downloaded from the same +release. The repository has signature-related source metadata paths, but the +installers do not use authenticated release signatures. -### 5.1 Interface layout documentation is stale - P2 +Required result: confirm the intended trust model and use authenticated +release metadata consistently on Unix and Windows. Checksums alone do not +establish publisher authenticity. Cover tampered and unsigned metadata under +the selected policy. -Ownership documentation says all interface carriers use `{rawptr, vtable}`. Backend layout for owned interfaces includes allocator/provenance state and is effectively `{data, dispatch, allocator}`. +### 9. CI change classification -Why wrong: +**Status:** resolved in current source. `scripts/detect-changes.sh` includes +`cmd/release-profile/*` in the toolchain case, matching toolchain fingerprint +inputs. Keep a regression if classifier rules are changed later. -- ABI work based on two-field model can omit ownership metadata. -- Calls, returns, drops, comparisons, and conversions may use incompatible layouts. +### 10. Current contributor documentation -Correct documentation should distinguish borrowed and owned carriers explicitly: +**Status:** open documentation-only work. -```text -borrowed interface: { data pointer, dispatch table } -owned interface: { data pointer, dispatch table, allocator/provenance } -``` +Current spot checks still find stale architecture claims: -Field names and exact LLVM types should be copied from canonical type-lowering owner, not maintained as an independent guessed ABI. +- `Code-tour.md` names `internal/project` as the owner of `Module` and lists + `hir` under `internal/ir`, while current ownership is in `internal/module` + and the repository has no HIR artifact. +- `docs/architecture/ir.md` names removed + `internal/ir/thir/build.go`; current THIR materialization is owned by + `internal/semantics/typechecker/thir_build.go`. +- Adjacent phase and infrastructure descriptions must be checked against live + source, including current `HasProvidedContent` and parser/workspace + fingerprint ownership. -### 5.2 Length/index documentation hardcodes `i64` - P2 +Required result: repair inaccurate paths, artifacts, phase descriptions, and +ownership claims only after source verification. Keep `FlowAnalyzer`, +`EffectBuilder`, and existing filenames. Do not repeat the rejected terminology +migration or add a documentation framework. -Allocator documentation describes dynamic array and string lengths as `i64`. Live lowering uses target `IndexType`, so width follows target pointer/index width. +## Architecture and performance follow-ups -Correct contract: +These are investigations, not proven fixes: -```text -length, capacity, and index fields use target usize/IndexType +- **Workspace rebuild cost:** `bdd87ec` shared project context setup and + `f18a320` reused one index pass across diagnostic components. Discovery, + manifest, hash, and graph work remains per refresh. Measure real rebuild work + before adding cache representations. +- **Safe per-function reuse:** identify one independently produced artifact, + every producer and consumer, and all semantic inputs: body/declaration, + scopes/imports, constants/defaults, generic instances, target/configuration, + diagnostics, and topology. FunctionID is identity, not cache validity. + Require demonstrated skipped work, clean-build parity, dependency-change + miss detection, and a full-module fallback when uncertain. Persistent + caching follows a proven in-memory boundary. +- **`TypeTable.Type` backing slices:** shallow returned descriptors still expose + mutable backing slices. No production mutation was found in the earlier + audit, so this remains a latent ownership hazard rather than a demonstrated + active defect. Recheck before changing APIs. + +## Validation and next step + +For each source fix, validate the affected package first, then affected +packages and the full suite. Language behavior changes require focused Go +tests, positive and applicable negative `x_test/` fixtures, bundled executable +validation, affected target widths, and backend output checks. The repository +validation set is: + +```sh +env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache go test -count=1 ./... +env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache go vet ./... +env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache go run ./scripts/bundle.go +env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache PEEPER_BIN="$PWD/build/bin/peeper" go test -count=1 ./x_test +git diff --check ``` -This matters on 32-bit targets. ABI documentation must state target-sized intent and validation must cover both 32-bit and 64-bit layouts. - -### 5.3 Intrinsic registry documentation overstates unification - P2 - -README describes a unified intrinsic registry, while predeclared membership and signature construction remain separate declarations. Documentation should not claim architectural work is complete until all consumers derive from one definition. - -### 5.4 CLI flow diagrams reference removed functions - P2 - -CLI diagrams still reference old parser and test-command functions. This makes code-flow navigation actively misleading. - -What should replace current maintenance model: - -- Update diagrams in same change that moves command ownership. -- Prefer function/module names verified from live source. -- Add a lightweight documentation check for referenced paths or symbols where practical. -- Mark conceptual nodes as conceptual, so they are not mistaken for real functions. - -## Implemented order - -1. Fixed default-expansion identity and semantic evidence ownership. -2. Made LSP diagnostic compilation and publication snapshot-safe. -3. Unified command contract, removed unused backend abstraction, fixed recursive checks, and staged generated artifacts. -4. Made dependency metadata returned-error atomic and cache cleanup post-commit. -5. Unified intrinsic ownership and enforced scheduler terminal state. -6. Aligned documentation with live compiler owners. - -## Validation evidence - -Remediation used live source and executable behavior, not diagrams as authority. - -- `go test -count=1 ./...` passed. -- `go vet ./...` passed. -- `go test -race -count=1 ./internal/lsp` passed. -- `bash scripts/build.sh` rebuilt bundled `build/bin/peeper` successfully. -- Bundled `check` and `run` passed for `x_test/default_interface_evidence`; process exited zero. -- Bundled checks rejected `x_test/negative_default_parameter_effect` and `x_test/negative_default_parameter_move`. -- `TestPipelineLowersEveryRegisteredIntrinsic` passed for 32-bit and 64-bit targets, including clang assembly. -- CLI subprocess tests cover recursive/multiple paths, mixed valid/invalid exit status, aliases/help, and same-basename generated artifacts. -- Dependency tests cover malformed locks, multi-package rollback, coordinated publish rollback, registry-scan failure, graph-only pruning, and post-commit cache cleanup retry. -- D2 renderer was unavailable in validation environment; diagram symbols were checked against live source, but rendered output was not verified. - -## Review boundary +Next implementation step: reproduce finding 1 or 2 against the current source, +then make one focused fix in its canonical owner. Do not discard or silently +overwrite unrelated work if a later checkout contains candidate changes. -Remediation implements all validated findings without commit, push, PR, or GitHub tracking changes. Suggested snippets in original finding sections remain historical design sketches, not current drop-in code. Coordinated dependency persistence guarantees restoration for returned publish errors; it does not claim journaled power-loss recovery or cross-process locking. +Preserve unrelated local artifacts and review inputs, including +`docs/superpowers/plans/2026-09-29-function-level-incremental-reuse.md`, +`final-code-review.md`, `review.md`, and `lsp.test` when they exist. diff --git a/x_test/cache_linux_test.go b/x_test/cache_linux_test.go index 8238c056..d111fba5 100644 --- a/x_test/cache_linux_test.go +++ b/x_test/cache_linux_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "syscall" "testing" @@ -68,13 +69,20 @@ func TestManagedObjectCacheFixture(t *testing.T) { t.Fatal(err) } } - // Preserve argv[0] for Clang wrappers such as ccache. + host := target.Host() + systemTriple, err := target.SystemLLVMTriple(host.OS, host.Arch) + if err != nil { + t.Fatal(err) + } + // Installed profiles require the canonical musl triple, but this cache-only + // fixture uses system Clang/libc. Select its host ABI so output does not depend + // on a musl loader being installed on the runner. The final --target wins. + driver := "#!/bin/sh\nexec '" + strings.ReplaceAll(clang, "'", "'\"'\"'") + "' \"$@\" --target=" + systemTriple + "\n" for _, tool := range []string{"clang", filepath.Join("link", "clang")} { - if err := os.Symlink(clang, filepath.Join(profileDir, tool)); err != nil { + if err := os.WriteFile(filepath.Join(profileDir, tool), []byte(driver), 0o755); err != nil { t.Fatal(err) } } - host := target.Host() profile := toolchain.Profile{ SchemaVersion: toolchain.ProfileSchemaVersion, ProfileID: "cache-fixture", TargetOS: host.OS, TargetArch: host.Arch, LLVMTriple: host.LLVMTriple, From 8efd1ca1348eafccf09f5d8473180dcda5bd622d Mon Sep 17 00:00:00 2001 From: itsfuad Date: Sun, 4 Oct 2026 02:04:57 +0600 Subject: [PATCH 3/6] Reject Non-Regular Cache Objects Validate cached paths before reuse and preserve existing directories on collision. Add regression coverage and refresh end-to-end review documentation. --- cmd/build.go | 5 +- cmd/build.go.bugfix.md | 69 --- cmd/build_test.go | 33 ++ docs/end-to-end-code-review.md | 804 +++++++++++++++++++++++---------- x_test/cache_linux_test.go | 18 + 5 files changed, 612 insertions(+), 317 deletions(-) delete mode 100644 cmd/build.go.bugfix.md diff --git a/cmd/build.go b/cmd/build.go index 856e862a..1f07c0f7 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -126,7 +126,10 @@ func compileObject(ctx *project.CompilerContext, profile toolchain.Profile, modu } cachePath := objectCachePath(ctx, profile, ir) if cachePath != "" { - if _, err := os.Stat(cachePath); err == nil { + if info, err := os.Stat(cachePath); err == nil { + if !info.Mode().IsRegular() { + return "", fmt.Errorf("inspect cached object for %s: %s is not a regular file", module.ID.ImportPath, cachePath) + } return cachePath, nil } else if !os.IsNotExist(err) { return "", fmt.Errorf("inspect cached object for %s: %w", module.ID.ImportPath, err) diff --git a/cmd/build.go.bugfix.md b/cmd/build.go.bugfix.md deleted file mode 100644 index 912ff2cb..00000000 --- a/cmd/build.go.bugfix.md +++ /dev/null @@ -1,69 +0,0 @@ -# Bugfix: object-cache publication - -Source reports: conversation code review P1/P2 and follow-up validation; -`docs/end-to-end-code-review.md`, finding 3. - -Date: 2026-10-03 -Mode: applied directly - -## Results - -1. **P1 — Non-atomic backup-first cache publication — Fixed.** - `compileObject` stages compiler output on the cache filesystem, then publishes - with direct `os.Rename`. Existing cache paths are never moved aside. When - replacement fails, another publisher's regular cache object can be reused; - without one, the original rename error is returned with `%w`. -2. **P2 — Missing production failure-cleanup coverage — Fixed.** - Compiler/publication failure tests exercise the production object lifecycle, - assert errors and stage cleanup, and verify incomplete output stays hidden. - Cross-device probes require `EXDEV`. A bundled Peeper fixture tests real - managed compilation/linking across filesystems and subsequent cache reuse. - -## Evidence and validation - -- Before fix: Linux filesystem-event test detected published cache object moved - away; concurrent reader observed missing file. Both pass after fix. -- Focused `cmd` and `internal/toolchain` tests pass, including cache hits, - compiler failures, publication failures, conflicting directories, winner - reuse, output permissions, and per-object cleanup. -- `go test -race -count=1 -run '^TestCompileObject' ./cmd` passes. -- Full `go test -count=1 ./...`, `go vet ./...`, fresh bundle, complete executable - `x_test` suite, formatting check, and `git diff --check` pass. -- Windows/amd64 and macOS/arm64 test binaries cross-compile. Native execution - also passed in PR #151 CI run 37146240861, including Windows locked-reader - regression. Linux native cache Go tests passed; Linux integration fixture - runtime exposed the test setup ABI mismatch described below. - -## Linux CI fixture follow-up - -Both Ubuntu architectures linked the synthetic managed fixture for musl with -system Clang, then failed execution because musl interpreter was absent. -Test-local Clang launchers now select `target.SystemLLVMTriple` as final tool -target while leaving installed-profile validation and production code intact. -Real compilation/linking, cross-filesystem assertions, executable checks, and -cache-hit proof remain enabled. - -Targeted fixture, full executable `x_test`, `internal/target` tests, `go vet -./x_test`, formatting, and diff checks pass locally. Clang dry-runs confirm glibc -interpreter selection for amd64 and arm64. Follow-up CI rerun pending publication. - -## Files changed - -- `.github/workflows/ci.yml` -- `cmd/build.go` -- `cmd/build_test.go` -- `cmd/build_linux_test.go` -- `cmd/build_windows_test.go` -- `x_test/cache_linux_test.go` -- `x_test/managed_object_cache/peeper.toml` -- `x_test/managed_object_cache/src/main.peep` -- This summary and ignored `review-followups.localplan.md`. - -## Rules check - -`compileObject` replaces existing loop/staging logic and owns a real per-object -cleanup lifetime. It has a current production caller. Obsolete `stageCachedObject` -removed. No pass-through wrapper, stale alias, ignored parameter, duplicated -cache decision, or production test hook added. Cache keys, compiler arguments, -diagnostics, error identity, and permissions preserved; publication and staging -lifetime intentionally repaired. diff --git a/cmd/build_test.go b/cmd/build_test.go index ebe08376..fc35f633 100644 --- a/cmd/build_test.go +++ b/cmd/build_test.go @@ -245,6 +245,39 @@ func TestCompileObjectLifecycle(t *testing.T) { } } +func TestCompileObjectRejectsPreexistingDirectory(t *testing.T) { + ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) + cachePath := objectCachePath(ctx, profile, strings.TrimSpace(mod.LLVMIR)) + // Unlike directory-collision, this entry exists before the initial lookup. + if err := os.MkdirAll(cachePath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cachePath, "keep"), []byte("keep"), 0o600); err != nil { + t.Fatal(err) + } + + objectPath, err := compileObject(ctx, profile, mod, base) + if err == nil || objectPath != "" { + t.Fatalf("compileObject() = %q, %v; want empty path and invalid-cache error", objectPath, err) + } + for _, text := range []string{"inspect cached object for " + mod.ID.ImportPath, cachePath, "not a regular file"} { + if !strings.Contains(err.Error(), text) { + t.Errorf("cache error = %v, want %q", err, text) + } + } + if data, err := os.ReadFile(filepath.Join(cachePath, "keep")); err != nil || string(data) != "keep" { + t.Fatalf("cache inspection damaged destination: %q, %v", data, err) + } + entries, err := os.ReadDir(filepath.Dir(cachePath)) + if err != nil || len(entries) != 1 || entries[0].Name() != filepath.Base(cachePath) || !entries[0].IsDir() { + t.Fatalf("cache inspection changed cache entries: %v, %v", entries, err) + } + entries, err = os.ReadDir(filepath.Dir(base)) + if err != nil || len(entries) != 0 { + t.Fatalf("cache inspection wrote compiler artifacts: %v, %v", entries, err) + } +} + func TestCompileObjectConcurrentReaders(t *testing.T) { ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) t.Setenv("PEEPER_TEST_OBJECT_MODE", "wait") diff --git a/docs/end-to-end-code-review.md b/docs/end-to-end-code-review.md index 81351201..3296cc4f 100644 --- a/docs/end-to-end-code-review.md +++ b/docs/end-to-end-code-review.md @@ -1,311 +1,621 @@ -# Compiler Review — Completed Work and Remaining Tasks +# End-to-End Code Review -## Review boundary +## Scope + +Review follows live execution paths from CLI/LSP entry points through dependency handling, compiler scheduling, semantic analysis, THIR/MIR lowering, LLVM emission, and user-facing documentation. + +This document preserves original findings and reproduction snippets as baseline evidence. Remediation status below describes current implementation; snippets inside finding sections are historical unless marked otherwise. + +## Remediation status + +| Finding group | Status | Chosen implementation | +|---|---|---| +| Interface conversion evidence | Resolved | Default substitution deep-clones each occurrence with fresh `NodeID`s and separate declaration/caller provenance maps. Semantic metadata copies from owning module; THIR/MIR consume unique per-occurrence interface evidence. | +| LSP diagnostics | Resolved | Compilation snapshots copy context, component files, generation, and versions under state lock. Publication is serialized, rejects stale generations, and writes without state lock. | +| CLI contract and artifacts | Resolved | One command registry drives dispatch/help/aliases. Compiler APIs are LLVM-only. `check` recursively discovers and groups roots by project. `_gen` publishes completed identity-based staging trees. | +| Dependency lifecycle | Resolved | Lock corruption propagates. `get`/`update` prepare in memory and publish coordinated manifest/lock state only after all work succeeds. Metadata commits before cache deletion. | +| Intrinsic and pipeline ownership | Resolved | One intrinsic definition slice drives compiler-owned free-function symbols, contextual discovery, operations, and signatures. Diagnostic-free scheduler completion requires every scheduled module at backend before runtime-symbol validation. | +| Documentation | Resolved | Interface ownership, target-sized carrier fields, README registry claim, and CLI diagrams now follow live owners. | + +This report groups related defects by owning subsystem. Severity reflects impact, not amount of code needed: + +- **P1:** valid input can crash, fail silently, corrupt state, or race. +- **P2:** architecture has multiple authorities, misleading abstractions, or silent failure paths that make extension unsafe. + +| Group | Highest severity | Main risk | +|---|---:|---| +| CLI and dependency lifecycle | P1 | Incorrect success, ignored input, damaged project state | +| Semantic evidence and intrinsic ownership | P1 | Valid program reaches backend with wrong type evidence | +| Compiler pipeline and backend boundary | P2 | Silent incomplete compilation and fake extensibility | +| LSP state and concurrency | P1 | Data race and stale diagnostics | +| Documentation and architectural truth | P2 | Maintainers implement against contracts that no longer exist | + +## 1. CLI and dependency lifecycle + +These defects belong together because they define command behavior and mutation of user-owned project state. + +### 1.1 `check` and `lint` do not implement their advertised input contract - P1 + +Help text says these commands typecheck a file or recursively check a folder. Actual command handling defaults to `.` and passes one path to the single-entry compiler path: -This document is a fresh-start work list built from the earlier whole-codebase -review and `final-code-review.md`. It is not a new whole-codebase audit. Each -finding below was checked against the current source before being listed as -open, resolved, or documentation-only. - -Current checkout used for this update: - -- branch: `main` -- HEAD: `ee3be84` (`Merge pull request #148 from PeeperLanguage/fix/shared-reference-projections`) -- worktree: clean -- production source/fixture changes, resets, commits, pushes, and GitHub - tracking: none - -The earlier note about uncommitted candidates in -`internal/ir/mir/expr_lower.go` and `internal/backend/llvm/instruction_emit.go` -and the associated rvalue/shift tests and fixtures does not reproduce in this -checkout. No files were removed to establish this baseline. - -Historical runtime results remain historical evidence. This update checked -current source, tests, fixtures, workflows, and documentation paths; it did -not claim to rerun the historical end-to-end reproductions. - -## Completed and revalidated work - -- [x] **Shared-reference and owner mutability:** committed in `1ae82933`. - Owning pointers do not independently grant mutation; immutable owners and - enclosing shared references cannot grant write access. Mutable owners and - `&mut` remain valid. Moving, freeing, and dropping an immutable owner remain - consumption operations and do not require `mut`. -- [x] **Nested/deferred semantic cases:** `1ae82933` covers optional reference - permissions, qualified constant mutable-borrow rejection, deferred - enum-field assignment compatibility, and durable late conversions consumed - by lowering. Go regressions and Peeper fixtures are included in that work. -- [x] **Artifact and AST cleanup:** `58a60595` and `76fbabc2` hardened - symbol-index snapshots and declaration ordering, sealed cloning, removed - unused `ast.Index`, and strengthened clone identity/isolation tests. -- [x] **LSP rename snapshot consistency:** rename uses one captured compiler - context instead of rereading `LastCtx` during background replacement. -- [x] **Transitive lockfile edge selection:** `71bdeef` records the recursive - resolver's selected package ID on parent edges. Regression covers competing - versions and persisted lockfiles. -- [x] **Structural type identity and layout:** `fbdbb52b` makes anonymous - struct field order part of identity and keeps named structs nominal. Distinct - struct types no longer convert; contextual literals still lower in - declaration order. -- [x] **THIR lookup experiment:** reverted. `Module.Node` uses its - module-owned index rather than repeated function scans. -- [x] **Lockfile direct-alias consistency:** current - `pkg/manifest/lockfile.go:SetDirectDependency` publishes the replacement - alias before demoting the old package and checks `isStillDirect` first. - Current tests cover one-alias demotion and preserving a package referenced - by another alias. This item is resolved in the current source; it is not an - open implementation task. -- [x] **CI toolchain classifier coverage:** current - `scripts/detect-changes.sh` includes `cmd/release-profile/*` in the - toolchain classification. The earlier omission is resolved. - -## Remaining compiler correctness work - -### 1. High — Capture rvalues before later operand side effects - -**Status:** open. Current source confirms the defect shape; no candidate is -present in this checkout. - -**Owner:** `internal/ir/mir/expr_lower.go`, with the source-to-IR place/value -boundary in `internal/ir/exprlower/lower.go`. - -`exprlower.LowerIdent` currently produces `ir.Ident`. MIR lowering of an -identifier produces a `RefName`, while LLVM `emitRef` loads that name when it -is emitted. A call argument or binary operand can therefore retain storage -identity until a later nested operand has already mutated the storage. MIR -lowering also emits nested call side effects while constructing the outer -call's arguments. - -Historical reproduction: - -```peep -#[extern("memset")] -fn Memset(p: rawptr, value: i32, size: usize) -> rawptr; -fn Clear(p: rawptr) -> i32 { Memset(p, 0, 4); return 0; } -fn Sum(a: i32, b: i32) -> i32 { return a + b; } - -fn main() { - let mut x: i32 = 7; - println(Sum(x, Clear(@x))); +```go +path := "." +if len(args) > 0 { + path = args[0] } + +return compileEntry(path, options) ``` -Historical result was `0`; expected result under left-to-right evaluation is -`7`. The same shape was reproduced with `println(x + Clear(@x))`. +This has two observable failures: + +- `peeper check .` attempts to read the directory as a source file and fails. +- `peeper check valid.peep invalid.peep` ignores every path after the first and can return success despite invalid requested input. + +Why wrong: + +- Command accepts syntax it does not honor. +- Exit status no longer represents all user-requested work. +- Help text and implementation have different owners for the command contract. + +What should replace it: -Required result: +Resolve input shape once, before compilation. Either support all documented forms or reject unsupported forms explicitly. -- materialize rvalue reads at their evaluation point using existing MIR - mechanisms; -- retain storage identity for assignment, address, and borrow destinations; -- do not blindly snapshot place roots; -- distinguish MIR instruction behavior from language-level ownership - consumption. +```go +type CheckTarget struct { + Files []string +} + +func resolveCheckTarget(args []string) (CheckTarget, error) { + // No args: discover project rooted at current directory. + // One directory: discover project/source files recursively. + // One or more files: validate and retain every file. + // Unsupported mixtures: return a usage error. +} +``` -Regression coverage must include call arguments, binary operands, aggregate -elements, index evaluation, preserved writes and borrows, and existing -move/drop/ownership behavior. The historical candidate paths -`internal/ir/mir/expr_lower_order_test.go`, -`x_test/runtime_mir_rvalue_eval_order_call_capture/`, and -`x_test/runtime_mir_rvalue_eval_order_binary_capture/` are absent here. +Compiler driver should receive resolved files or a project, not reinterpret raw CLI arguments. One owner must define which files form a check operation. -### 2. High — Make narrow shift guards width-safe +Acceptance criteria: -**Status:** open. Current source still contains the lossy guard. +- Directory input checks intended project files. +- Multiple explicit files are all checked, or extra arguments are rejected. +- Any failed requested input produces nonzero exit status. +- Help and argument validation derive from same command definition. -**Owner:** `internal/backend/llvm/instruction_emit.go`. +### 1.2 Corrupt lockfiles are treated as missing lockfiles - P1 -Current shift lowering computes operand width, then compares `right` against -`bits` using `right.Layout`: +`manifest.LoadLockfile` already distinguishes a missing file from other read or parse failures. `get` discards that distinction: ```go -invalid := b.compare("icmp", "uge", right, - b.value(strconv.Itoa(bits), right.Layout)) +lock, err := manifest.LoadLockfile(projectDir) +if err != nil { + lock = manifest.NewLockfile() +} ``` -For a narrow count such as `u5`, the bound `32` is not representable in the -count type. LLVM therefore receives an invalid or wrapped bound instead of a -comparison in a type that can represent both values. The later cast to the -shift operand type happens only after this guard. +A malformed or unreadable existing lockfile therefore becomes an empty lockfile. Later save can replace evidence needed to reproduce the existing dependency graph. -Historical reproduction: +Why wrong: -```peep -fn Shift(count: u5) -> u32 { return 1u32 << count; } -fn main() { println(Shift(1u5)); } +- Recovery policy is duplicated outside the lockfile owner. +- Corruption becomes implicit destructive recovery. +- User receives no diagnosis before state changes. + +What should replace it: + +Trust `LoadLockfile` to own the missing-file case and propagate every remaining error: + +```go +lock, err := manifest.LoadLockfile(projectDir) +if err != nil { + return fmt.Errorf("load dependency lockfile: %w", err) +} +``` + +If explicit repair is desired, it should be a separately named operation with clear user intent-not normal `get` behavior. + +### 1.3 `update` reports success after dependency installation failures - P1 + +Current loop prints an error and continues: + +```go +if err := installUpdate(pkg); err != nil { + fmt.Fprintf(stderr, "failed to update %s: %v\n", pkg.Name, err) + continue +} +``` + +Command can then print an up-to-date message and exit successfully. A partially failed mutation is indistinguishable to scripts from complete success. + +Why wrong: + +- Process status contradicts emitted diagnostics. +- "No updates available" and "all updates failed" collapse into same result. +- CI and automation cannot safely consume command result. + +What should replace it: + +Collect failures while allowing independent work to continue, then return one failure result: + +```go +var failures []error + +for _, pkg := range updates { + if err := installUpdate(pkg); err != nil { + failures = append(failures, fmt.Errorf("%s: %w", pkg.Name, err)) + continue + } + updated++ +} + +if len(failures) != 0 { + return errors.Join(failures...) +} ``` -Required result: validate signed negative and oversized counts in a width that -can represent the count and operand-width bound, then convert to the shift -operand width only after validation. Cover both directions, signed and -unsigned counts, zero and highest valid counts, the exact bound, wide counts -that would truncate to valid values, odd widths, and 32-/64-bit target -layouts. Historical candidate files are absent here. +Summary output must separately represent updated, unchanged, and failed counts. -### 3. Medium — Publish cached objects across filesystems safely +### 1.4 Dependency mutations are not transactional - P1 -**Status:** open. Current source still stages in system temporary storage and -renames across filesystems. +`get`, `remove`, and `update` modify several related resources: -**Owner:** `cmd/build.go`. +- manifest +- lockfile +- package cache/install tree -`buildExecutable` creates `artifactDir` with `os.MkdirTemp("", ...)`, then -publishes each compiled object with `os.Rename(objectPath, cachePath)`. A -managed build whose temporary directory and project cache use different -filesystems can fail with `invalid cross-device link`. +These resources are saved or pruned in different orders. For example, pruning can happen before both metadata files are safely persisted. Individual atomic file writes do not make the whole operation atomic. -Required result: stage the object on the cache destination filesystem and -atomically publish it there. Incomplete objects must remain invisible to cache -readers. Test differing temporary/cache filesystems and cleanup on compiler or -publish failure. +Why wrong: -### 4. Medium — Preserve negative zero in runtime floating negation +- Failure between writes leaves manifest and lockfile describing different graphs. +- Early cache pruning can remove data still referenced by durable metadata. +- Retry behavior depends on where failure occurred. -**Status:** open. Current source still emits `fsub` from positive zero for -floating unary minus. +What should replace it: -**Owner:** `internal/backend/llvm/instruction_emit.go`. +Use one dependency transaction owned by dependency management: -The floating branch of unary `-` currently emits: +```go +type DependencyPlan struct { + Manifest manifest.Manifest + Lockfile manifest.Lockfile + Installs []ResolvedPackage + Prunes []PackagePath +} + +func ApplyDependencyPlan(plan DependencyPlan) error { + // 1. Stage downloads and generated metadata. + // 2. Validate complete staged graph. + // 3. Commit manifest and lockfile as one coordinated operation. + // 4. Prune obsolete cache entries only after durable commit. +} +``` + +Exact disk mechanism can vary, but invariant cannot: before commit, old state remains usable; after commit, both metadata files describe same dependency graph. + +### 1.5 Command metadata has multiple authorities - P2 + +Command names, aliases, handlers, help, and backend policy are spread across separate switches and text blocks. Adding one command requires synchronized edits in several files. Alias and help drift already demonstrate this is not theoretical. + +What should replace it: + +One declarative registry should own command identity and dispatch metadata: ```go -b.arithmetic("fsub", b.value("0.0", arg.Layout), arg) +type CommandDefinition struct { + Name string + Aliases []string + Usage string + Run func(CommandContext, []string) error + BackendRule BackendRule +} + +var commands = []CommandDefinition{ + {Name: "check", Aliases: []string{"lint"}, Usage: "check [path...]", Run: runCheck}, + {Name: "build", Usage: "build [path]", Run: runBuild, BackendRule: LLVMOnly}, +} ``` -This differs from constant folding for signed zero. Historical reproduction -used `1.0f64 / Neg(0.0f64)` and observed `inf` instead of `-inf`. +Lookup, help generation, alias handling, and validation should derive from this registry. Do not add another wrapper around existing switches; remove split authorities when migrating. -Required result: emit LLVM `fneg` and prove constant/runtime parity for signed -zero in both `f32` and `f64`. +### 1.6 Generated IR artifacts can collide or remain stale - P2 -### 5. Medium — Reconcile floating remainder with the specification +`-keep-gen` uses identity-encoded module paths for generated files. MIR and LLVM artifacts are staged and replaced atomically, so old artifacts do not survive a successful publication. -**Status:** open. Current source is inconsistent across specification, -typechecking, constant folding, and backend emission. +What should replace it: -`docs/language-spec.md` says floating division and remainder keep IEEE -behavior. LLVM emission already selects `frem` for floating `%`, but -`internal/semantics/typechecker/check_expr.go` accepts `%` only for integral -types, and `internal/constvalue/value.go` has no floating `%` case. +- Derive artifact path from canonical module/import identity, not basename alone. +- Write into a clean per-build staging directory. +- Publish completed artifacts only after successful generation. -Required result: confirm the intended language rule, then make typing, -constant evaluation, and runtime behavior agree. Do not remove specified -behavior merely to avoid implementation work. Cover floating boundary cases -and constant/runtime parity. +```text +build/generated/ + app/main.mir + app/main.ll + deps/example/main.mir + deps/example/main.ll +``` -### 6. Medium — Discover new files in existing empty directories +## 2. Semantic evidence and intrinsic ownership -**Status:** open. Current source retains the incomplete discovery shortcut. +These issues concern semantic facts that later compiler phases trust. Such facts need one owner and identity matching the operation they describe. -**Owner:** `internal/lsp/workspace.go`. +### 2.1 Interface-conversion evidence is keyed by expression, not conversion occurrence - P1 -The fast path returns cached module paths when `directoriesUnchanged` is true. -`captureDirectoryStamps` records the workspace root, source directory, and -directories containing discovered files. It does not record every directory -traversed during discovery, including pre-existing empty nested directories. -Creating a source file in one of those directories can therefore leave the -directory stamps unchanged and omit the new file when no open-buffer overlay -exists. +Typechecker stores interface implementation evidence using source expression `NodeID`: -Required result: track all relevant traversed directories for invalidation, or -remove the incomplete shortcut. Add an unopened-file regression while -preserving existing discovery and incremental behavior. LSP workspace -concurrency checks are required for any implementation change. +```go +tc.project.InterfaceImplementations[expr.ID()] = implementations +``` -## Broader repository findings to recheck before fixing +Default-argument substitution can reuse one caller expression for multiple parameters. If two expanded defaults convert that expression to different interfaces, both conversions share the same `NodeID`. Second write overwrites first conversion's slot set. -### 7. Lockfile alias/direct-dependency consistency +THIR/MIR lowering later asks for evidence by expression ID: -**Status:** resolved on current source; retained here as revalidated evidence. +```go +implementations := l.project.InterfaceImplementations[expr.ID()] +``` -`SetDirectDependency` now uses `isStillDirect` before clearing the previous -package's `IsDirect` flag. Tests cover replacement, shared aliases, and -dependency-edge `UsedBy` rewiring. Any later lockfile work must still audit -forward edges, reverse `UsedBy` edges, direct flags, pruning, and persistence -without inventing another mirrored representation. +By then target interface context is no longer sufficient to recover overwritten proof. Valid source can reach typed LLVM with wrong aggregate layout and panic, for example: -### 8. Installer release-signature verification +```text +call @use argument 1 is i32, want { i8*, i8* } +``` -**Status:** open. +Why wrong: -The release workflow currently builds an unsigned manifest and publishes -`SHA256SUMS`. Both `scripts/install.sh` and `scripts/install.ps1` authenticate -the manifest and components only by checksums downloaded from the same -release. The repository has signature-related source metadata paths, but the -installers do not use authenticated release signatures. +- Evidence describes a conversion, but key identifies only reused input expression. +- Cache assumes one expression has at most one interface target. +- Later phase receives mutable last-writer state instead of exact semantic result. +- Backend panic is only symptom; ownership error begins in semantic handoff. -Required result: confirm the intended trust model and use authenticated -release metadata consistently on Unix and Windows. Checksums alone do not -establish publisher authenticity. Cover tampered and unsigned metadata under -the selected policy. +What should replace it: -### 9. CI change classification +Best model: make conversion explicit in typed semantic output so evidence travels with its occurrence: -**Status:** resolved in current source. `scripts/detect-changes.sh` includes -`cmd/release-profile/*` in the toolchain case, matching toolchain fingerprint -inputs. Keep a regression if classifier rules are changed later. +```go +type InterfaceConversion struct { + Expr ast.Expr + Target typeinfo.Type + Implementations []project.InterfaceImplementation +} +``` -### 10. Current contributor documentation +If current IR cannot represent explicit conversion nodes yet, use a conversion-site key that cannot collide across expanded arguments: -**Status:** open documentation-only work. +```go +type ConversionSite struct { + ExpressionID ast.NodeID + CallID ast.NodeID + Parameter int +} -Current spot checks still find stale architecture claims: +type InterfaceEvidence map[ConversionSite][]project.InterfaceImplementation +``` + +Key requirement: evidence identity must include conversion occurrence and target context. Adding target type alone may still be insufficient if same expression converts to same interface at separate sites with distinct ownership/lifetime facts later. + +Required regression fixture: + +```peeper +interface ReadA { + read_a(self: &Self) -> i32 +} + +interface ReadB { + read_b(self: &Self) -> i32 +} + +fn use[T](x: T, a: &ReadA = x, b: &ReadB = x) { + // Both defaults reuse x but require distinct interface evidence. +} -- `Code-tour.md` names `internal/project` as the owner of `Module` and lists - `hir` under `internal/ir`, while current ownership is in `internal/module` - and the repository has no HIR artifact. -- `docs/architecture/ir.md` names removed - `internal/ir/thir/build.go`; current THIR materialization is owned by - `internal/semantics/typechecker/thir_build.go`. -- Adjacent phase and infrastructure descriptions must be checked against live - source, including current `HasProvidedContent` and parser/workspace - fingerprint ownership. +use(&c) +``` -Required result: repair inaccurate paths, artifacts, phase descriptions, and -ownership claims only after source verification. Keep `FlowAnalyzer`, -`EffectBuilder`, and existing filenames. Do not repeat the rejected terminology -migration or add a documentation framework. +Fixture must compile and execute through bundled `build/bin/peeper`, not stop at Go unit coverage. -## Architecture and performance follow-ups +### 2.2 Predeclared intrinsic registry still has two authorities - P2 -These are investigations, not proven fixes: +One list says which operations are predeclared: -- **Workspace rebuild cost:** `bdd87ec` shared project context setup and - `f18a320` reused one index pass across diagnostic components. Discovery, - manifest, hash, and graph work remains per refresh. Measure real rebuild work - before adding cache representations. -- **Safe per-function reuse:** identify one independently produced artifact, - every producer and consumer, and all semantic inputs: body/declaration, - scopes/imports, constants/defaults, generic instances, target/configuration, - diagnostics, and topology. FunctionID is identity, not cache validity. - Require demonstrated skipped work, clean-build parity, dependency-change - miss detection, and a full-module fallback when uncertain. Persistent - caching follows a proven in-memory boundary. -- **`TypeTable.Type` backing slices:** shallow returned descriptors still expose - mutable backing slices. No production mutation was found in the earlier - audit, so this remains a latent ownership hazard rather than a demonstrated - active defect. Recheck before changing APIs. - -## Validation and next step - -For each source fix, validate the affected package first, then affected -packages and the full suite. Language behavior changes require focused Go -tests, positive and applicable negative `x_test/` fixtures, bundled executable -validation, affected target widths, and backend output checks. The repository -validation set is: - -```sh -env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache go test -count=1 ./... -env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache go vet ./... -env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache go run ./scripts/bundle.go -env CCACHE_DISABLE=1 GOCACHE=/tmp/peeper-go-cache PEEPER_BIN="$PWD/build/bin/peeper" go test -count=1 ./x_test -git diff --check +```go +var predeclaredOperations = []symbols.CompilerOp{ + // operation identities +} ``` -Next implementation step: reproduce finding 1 or 2 against the current source, -then make one focused fix in its canonical owner. Do not discard or silently -overwrite unrelated work if a later checkout contains candidate changes. +A separate switch says which signatures those operations have: + +```go +switch op { +case symbols.SomeOperation: + return someSignature(target) +// ... +default: + panic("missing intrinsic signature") +} +``` + +Adding an operation requires editing both. Missing one edit either omits symbol exposure or panics during predeclared-scope initialization. + +Why wrong: + +- Registry is unified by API name, not by data ownership. +- Startup correctness depends on synchronized declarations. +- Extension failure occurs far from operation declaration. + +What should replace it: + +One definition must contain identity, exposure, and signature factory: + +```go +type intrinsicDefinition struct { + Operation symbols.CompilerOp + Name string + Predeclared bool + Receiver receiverShape + Signature func(target.Info) *typeinfo.FuncType +} + +var intrinsicDefinitions = []intrinsicDefinition{ + { + Operation: symbols.SomeOperation, + Name: "some_operation", + Predeclared: true, + Signature: someOperationSignature, + }, +} +``` + +`PredeclaredSymbols`, symbol lookup, and operation enumeration should derive from this same collection. A consistency test is useful defense, but it does not replace removing duplicate authority. + +## 3. Compiler pipeline and backend boundary + +These problems concern phase scheduling and ownership after semantic analysis. + +### 3.1 Pipeline scheduler can stop without reporting incomplete modules - P2 + +Scheduler breaks when no module is ready or no phase advances. It can then return without proving every requested module reached backend completion. + +Conceptually, current control flow is: + +```go +for { + ready := findReadyModules() + if len(ready) == 0 { + break + } + + advanced := advance(ready) + if !advanced { + break + } +} + +return nil +``` + +Later build code observes missing/empty LLVM IR, so diagnosis appears after scheduler lost information about blocked phase and prerequisite. + +Why wrong: + +- Loop termination is mistaken for successful compilation. +- Root cause is replaced by downstream empty-artifact failure. +- Adding a phase or dependency edge can silently create an unreported stall. + +What should replace it: + +Scheduler must establish terminal invariant before returning success: + +```go +for _, module := range orderedModules { + if module.Phase != project.PhaseBackend { + return fmt.Errorf( + "pipeline stalled: module %s stopped at %s", + module.Path, + module.Phase, + ) + } +} +``` + +Production diagnostic should include unmet prerequisite or blocking diagnostic when available. Generic invariant error is fallback, not preferred user message. + +### 3.2 Backend abstraction is declared but not owned at one boundary - P2 + +Project state defines LLVM and WASM backend identities, but pipeline imports LLVM directly, module state stores `LLVMIR`, CLI rejects non-LLVM backends, and build consumes LLVM-specific output. + +Current effective flow: + +```text +CLI backend flag + -> project.TargetBackend + -> pipeline ignores abstraction + -> LLVM generator + -> module.LLVMIR + -> clang +``` + +Adding a backend requires edits across CLI, project core, scheduler, module representation, artifact handling, and build toolchain. `TargetBackend` therefore suggests an extension point that does not exist. + +Two valid directions exist: + +1. **Be honestly LLVM-only now.** Remove unsupported backend identities and generic configuration until a second backend is implemented. +2. **Create a real post-MIR backend boundary.** Pipeline selects one backend owner and stores a backend-neutral artifact. + +```go +type Backend interface { + Name() backend.Type + Emit(*mir.Module, target.Info, *diagnostics.DiagnosticBag) (Artifact, error) +} + +type Artifact struct { + Kind backend.Type + Text string +} +``` + +Do not add this interface merely to wrap `GenerateLLVMIR`. It is justified only when pipeline dispatch and artifact ownership actually move behind it. Until then, deleting false generality is simpler and more maintainable. + +### Desired phase ownership + +```text +Parser + produces syntax + | +Typechecker + produces types + conversion evidence + | +THIR/MIR lowering + consumes semantic evidence; does not rediscover it + | +MIR lowering + owns target-independent executable representation + | +Backend boundary + owns target/backend-specific representation and emission + | +CLI build + owns external tool invocation and final artifact placement +``` + +Each arrow is a typed contract. Later phases should not recover facts that an earlier phase already knew, and earlier phases should not store backend-specific output. + +## 4. LSP state and concurrency + +### 4.1 Diagnostic workers read workspace state outside its lock - P1 + +Each changed file can schedule a diagnostic goroutine. Recompile locks shared state only while compilation runs. After unlock, worker computes component files from `state.workspace`. Another worker can rebuild or replace that workspace concurrently. + +Simplified race: + +```text +worker A: lock -> compile A -> unlock +worker B: lock -> rebuild workspace -> unlock +worker A: read workspace component files without lock +``` + +Consequences: + +- Go data race on workspace state. +- Diagnostics from one compiler context can be published using file membership from another workspace generation. +- Older worker can overwrite newer diagnostics. + +What should replace it: + +Create immutable publication snapshot while holding state lock. Release lock before writing protocol output: + +```go +type diagnosticSnapshot struct { + Context *project.CompilerContext + Files []string + Version uint64 +} + +func (s *ServerState) compileDiagnosticSnapshot(path string) (diagnosticSnapshot, error) { + s.mu.Lock() + defer s.mu.Unlock() + + ctx, err := s.recompileLocked(path) + if err != nil { + return diagnosticSnapshot{}, err + } + + return diagnosticSnapshot{ + Context: ctx, + Files: append([]string(nil), s.workspace.componentFiles(path)...), + Version: s.version, + }, nil +} +``` + +Before publishing, compare snapshot version with latest requested version for each document. This prevents slow old workers from replacing current diagnostics. + +Do not hold state mutex while sending LSP messages. Output can block and should not serialize compilation state. + +Acceptance criteria: + +- Workspace reads and writes follow one locking policy. +- Publication operates on copied immutable data. +- Stale worker cannot publish over newer document version. +- Race-enabled tests pass in a Go installation with working race runtime. + +## 5. Documentation and architectural truth + +Documentation defects are grouped because each describes a contract maintainers may use when extending compiler. + +### 5.1 Interface layout documentation is stale - P2 + +Ownership documentation says all interface carriers use `{rawptr, vtable}`. Backend layout for owned interfaces includes allocator/provenance state and is effectively `{data, dispatch, allocator}`. + +Why wrong: + +- ABI work based on two-field model can omit ownership metadata. +- Calls, returns, drops, comparisons, and conversions may use incompatible layouts. + +Correct documentation should distinguish borrowed and owned carriers explicitly: + +```text +borrowed interface: { data pointer, dispatch table } +owned interface: { data pointer, dispatch table, allocator/provenance } +``` + +Field names and exact LLVM types should be copied from canonical type-lowering owner, not maintained as an independent guessed ABI. + +### 5.2 Length/index documentation hardcodes `i64` - P2 + +Allocator documentation describes dynamic array and string lengths as `i64`. Live lowering uses target `IndexType`, so width follows target pointer/index width. + +Correct contract: + +```text +length, capacity, and index fields use target usize/IndexType +``` + +This matters on 32-bit targets. ABI documentation must state target-sized intent and validation must cover both 32-bit and 64-bit layouts. + +### 5.3 Intrinsic registry documentation overstates unification - P2 + +README describes a unified intrinsic registry, while predeclared membership and signature construction remain separate declarations. Documentation should not claim architectural work is complete until all consumers derive from one definition. + +### 5.4 CLI flow diagrams reference removed functions - P2 + +CLI diagrams still reference old parser and test-command functions. This makes code-flow navigation actively misleading. + +What should replace current maintenance model: + +- Update diagrams in same change that moves command ownership. +- Prefer function/module names verified from live source. +- Add a lightweight documentation check for referenced paths or symbols where practical. +- Mark conceptual nodes as conceptual, so they are not mistaken for real functions. + +## Implemented order + +1. Fixed default-expansion identity and semantic evidence ownership. +2. Made LSP diagnostic compilation and publication snapshot-safe. +3. Unified command contract, removed unused backend abstraction, fixed recursive checks, and staged generated artifacts. +4. Made dependency metadata returned-error atomic and cache cleanup post-commit. +5. Unified intrinsic ownership and enforced scheduler terminal state. +6. Aligned documentation with live compiler owners. + +## Validation evidence + +Remediation used live source and executable behavior, not diagrams as authority. + +- `go test -count=1 ./...` passed. +- `go vet ./...` passed. +- `go test -race -count=1 ./internal/lsp` passed. +- `bash scripts/build.sh` rebuilt bundled `build/bin/peeper` successfully. +- Bundled `check` and `run` passed for `x_test/default_interface_evidence`; process exited zero. +- Bundled checks rejected `x_test/negative_default_parameter_effect` and `x_test/negative_default_parameter_move`. +- `TestPipelineLowersEveryRegisteredIntrinsic` passed for 32-bit and 64-bit targets, including clang assembly. +- CLI subprocess tests cover recursive/multiple paths, mixed valid/invalid exit status, aliases/help, and same-basename generated artifacts. +- Dependency tests cover malformed locks, multi-package rollback, coordinated publish rollback, registry-scan failure, graph-only pruning, and post-commit cache cleanup retry. +- D2 renderer was unavailable in validation environment; diagram symbols were checked against live source, but rendered output was not verified. + +## Review boundary -Preserve unrelated local artifacts and review inputs, including -`docs/superpowers/plans/2026-09-29-function-level-incremental-reuse.md`, -`final-code-review.md`, `review.md`, and `lsp.test` when they exist. +Remediation implements all validated findings without commit, push, PR, or GitHub tracking changes. Suggested snippets in original finding sections remain historical design sketches, not current drop-in code. Coordinated dependency persistence guarantees restoration for returned publish errors; it does not claim journaled power-loss recovery or cross-process locking. diff --git a/x_test/cache_linux_test.go b/x_test/cache_linux_test.go index d111fba5..10601a35 100644 --- a/x_test/cache_linux_test.go +++ b/x_test/cache_linux_test.go @@ -77,6 +77,8 @@ func TestManagedObjectCacheFixture(t *testing.T) { // Installed profiles require the canonical musl triple, but this cache-only // fixture uses system Clang/libc. Select its host ABI so output does not depend // on a musl loader being installed on the runner. The final --target wins. + // This tests real compilation, cross-filesystem caching, and execution with + // the system ABI; it does not validate the managed musl ABI/toolchain. driver := "#!/bin/sh\nexec '" + strings.ReplaceAll(clang, "'", "'\"'\"'") + "' \"$@\" --target=" + systemTriple + "\n" for _, tool := range []string{"clang", filepath.Join("link", "clang")} { if err := os.WriteFile(filepath.Join(profileDir, tool), []byte(driver), 0o755); err != nil { @@ -127,4 +129,20 @@ func TestManagedObjectCacheFixture(t *testing.T) { } } } + + // A non-regular entry present before the next build must fail cache inspection, + // rather than reach the compiler/linker or be removed as part of cache repair. + if err := os.Remove(objects[0]); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(objects[0], 0o755); err != nil { + t.Fatal(err) + } + expectation.Mode = "build" + expectation.Outcome = "failure" + expectation.StderrContains = []string{"inspect cached object for", objects[0], "not a regular file"} + runFixture(t, installedBinary, expectation) + if info, err := os.Stat(objects[0]); err != nil || !info.IsDir() { + t.Fatalf("cache inspection changed invalid entry: %v, %v", info, err) + } } From 16041531f3ab7ae58055eaa3e16c505f75958b31 Mon Sep 17 00:00:00 2001 From: itsfuad Date: Sun, 4 Oct 2026 02:48:49 +0600 Subject: [PATCH 4/6] Publish cached objects without replacing winners --- cmd/build.go | 37 ++++++++++++++++++--------- cmd/build_test.go | 54 +++++++++++++++++++++++++++++++++++++++ cmd/build_windows_test.go | 2 +- 3 files changed, 80 insertions(+), 13 deletions(-) diff --git a/cmd/build.go b/cmd/build.go index 1f07c0f7..c2dcec31 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -126,13 +126,10 @@ func compileObject(ctx *project.CompilerContext, profile toolchain.Profile, modu } cachePath := objectCachePath(ctx, profile, ir) if cachePath != "" { - if info, err := os.Stat(cachePath); err == nil { - if !info.Mode().IsRegular() { - return "", fmt.Errorf("inspect cached object for %s: %s is not a regular file", module.ID.ImportPath, cachePath) - } - return cachePath, nil - } else if !os.IsNotExist(err) { + if exists, err := inspectCachedObject(cachePath); err != nil { return "", fmt.Errorf("inspect cached object for %s: %w", module.ID.ImportPath, err) + } else if exists { + return cachePath, nil } } llPath := objectBase + ".ll" @@ -157,12 +154,14 @@ func compileObject(ctx *project.CompilerContext, profile toolchain.Profile, modu return "", err } if cachePath != "" { - // Never move a published object aside: another linker may be opening it. - if err := os.Rename(objectPath, cachePath); err != nil { - // A competing build may have won this key. Reuse its complete object - // when replacement is blocked (e.g. an open file on Windows). - if info, statErr := os.Stat(cachePath); statErr != nil || !info.Mode().IsRegular() { - return "", fmt.Errorf("publish cached object: %w", err) + // A hard link installs the completed object without replacing a winner. + if linkErr := os.Link(objectPath, cachePath); linkErr != nil { + // Any failed link may race with another publisher. Reuse only a + // regular directory entry and preserve the publication error otherwise. + if exists, err := inspectCachedObject(cachePath); err != nil { + return "", fmt.Errorf("publish cached object: %w; inspect cache winner: %w", linkErr, err) + } else if !exists { + return "", fmt.Errorf("publish cached object: %w", linkErr) } } return cachePath, nil @@ -170,6 +169,20 @@ func compileObject(ctx *project.CompilerContext, profile toolchain.Profile, modu return objectPath, nil } +func inspectCachedObject(path string) (bool, error) { + info, err := os.Lstat(path) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, err + } + if !info.Mode().IsRegular() { + return false, fmt.Errorf("%s is not a regular file", path) + } + return true, nil +} + func objectCachePath(ctx *project.CompilerContext, profile toolchain.Profile, ir string) string { if ctx == nil || !profile.IsManaged || ctx.Config.RootDir == "" { return "" diff --git a/cmd/build_test.go b/cmd/build_test.go index fc35f633..a407f42c 100644 --- a/cmd/build_test.go +++ b/cmd/build_test.go @@ -278,6 +278,45 @@ func TestCompileObjectRejectsPreexistingDirectory(t *testing.T) { } } +func TestCompileObjectRejectsPreexistingSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows CI does not guarantee symlink privileges") + } + ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) + cachePath := objectCachePath(ctx, profile, strings.TrimSpace(mod.LLVMIR)) + if err := os.MkdirAll(filepath.Dir(cachePath), 0o755); err != nil { + t.Fatal(err) + } + targetPath := filepath.Join(t.TempDir(), "object.o") + if err := os.WriteFile(targetPath, []byte("not cached"), 0o640); err != nil { + t.Fatal(err) + } + if err := os.Symlink(targetPath, cachePath); err != nil { + t.Fatal(err) + } + + objectPath, err := compileObject(ctx, profile, mod, base) + if err == nil || objectPath != "" { + t.Fatalf("compileObject() = %q, %v; want empty path and invalid-cache error", objectPath, err) + } + for _, text := range []string{"inspect cached object for " + mod.ID.ImportPath, cachePath, "not a regular file"} { + if !strings.Contains(err.Error(), text) { + t.Errorf("cache error = %v, want %q", err, text) + } + } + info, err := os.Lstat(cachePath) + if err != nil || info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("cache inspection changed symlink: %v, %v", info, err) + } + if data, err := os.ReadFile(targetPath); err != nil || string(data) != "not cached" { + t.Fatalf("cache inspection changed symlink target: %q, %v", data, err) + } + entries, err := os.ReadDir(filepath.Dir(base)) + if err != nil || len(entries) != 0 { + t.Fatalf("cache inspection wrote compiler artifacts: %v, %v", entries, err) + } +} + func TestCompileObjectConcurrentReaders(t *testing.T) { ctx, profile, mod, base := objectCompileFixture(t, t.TempDir()) t.Setenv("PEEPER_TEST_OBJECT_MODE", "wait") @@ -312,6 +351,10 @@ func TestCompileObjectConcurrentReaders(t *testing.T) { if err := <-results[0]; err != nil { t.Fatal(err) } + publishedInfo, err := os.Lstat(cachePath) + if err != nil { + t.Fatal(err) + } stopReader := make(chan struct{}) readerDone := make(chan error, 1) go func() { @@ -341,6 +384,17 @@ func TestCompileObjectConcurrentReaders(t *testing.T) { if err := <-results[1]; err != nil { t.Fatal(err) } + currentInfo, err := os.Lstat(cachePath) + if err != nil { + t.Fatal(err) + } + if !os.SameFile(publishedInfo, currentInfo) { + t.Fatal("second publisher replaced the published cache object") + } + entries, err := os.ReadDir(filepath.Dir(cachePath)) + if err != nil || len(entries) != 1 || entries[0].Name() != filepath.Base(cachePath) || entries[0].IsDir() { + t.Fatalf("duplicate publisher left cache entries: %v, %v", entries, err) + } } func TestCompileObjectAcrossFilesystems(t *testing.T) { diff --git a/cmd/build_windows_test.go b/cmd/build_windows_test.go index 289a9096..96763260 100644 --- a/cmd/build_windows_test.go +++ b/cmd/build_windows_test.go @@ -27,7 +27,7 @@ func TestCompileObjectReusesLockedWinner(t *testing.T) { t.Fatal(err) } // Model a linker holding a competing publisher's complete object without - // FILE_SHARE_DELETE. Windows must reject replacing this open destination. + // FILE_SHARE_DELETE. The probe proves the old replacement path is blocked. want := "object:" + strings.TrimSpace(mod.LLVMIR) if err := os.WriteFile(cachePath, []byte(want), 0o640); err != nil { t.Fatal(err) From a74245858dd265a0ddc511c569142f41ee7b376d Mon Sep 17 00:00:00 2001 From: itsfuad Date: Sun, 4 Oct 2026 12:15:40 +0600 Subject: [PATCH 5/6] Run CI for merge queue groups --- .github/workflows/ci.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7d6f3c3f..e6566ec2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,6 +4,9 @@ on: pull_request: branches: - main + merge_group: + types: + - checks_requested push: branches: - main @@ -35,8 +38,8 @@ jobs: env: EVENT_NAME: ${{ github.event_name }} EVENT_BEFORE: ${{ github.event.before }} - EVENT_BASE_SHA: ${{ github.event.pull_request.base.sha }} - EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + EVENT_BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }} + EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha || github.event.merge_group.head_sha }} run: bash scripts/detect-changes.sh quality: From 0d4c230d38d027b480afdcfcd264267a957f152a Mon Sep 17 00:00:00 2001 From: itsfuad Date: Sun, 4 Oct 2026 12:15:48 +0600 Subject: [PATCH 6/6] Handle merge queue change detection --- scripts/detect-changes.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/detect-changes.sh b/scripts/detect-changes.sh index 660f9a66..e60d6aaf 100644 --- a/scripts/detect-changes.sh +++ b/scripts/detect-changes.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash set -euo pipefail -if [ "${EVENT_NAME:-}" = pull_request ]; then +if [ "${EVENT_NAME:-}" = pull_request ] || [ "${EVENT_NAME:-}" = merge_group ]; then base="$EVENT_BASE_SHA" head="$EVENT_HEAD_SHA" elif [ "${EVENT_NAME:-}" = push ] && [ -n "${EVENT_BEFORE:-}" ] && [[ "$EVENT_BEFORE" != 0000000000000000000000000000000000000000 ]]; then