From aac588f4814e9acbbfd04af84f424172d5544fdd Mon Sep 17 00:00:00 2001 From: Ankush Desai Date: Wed, 27 May 2026 21:55:35 -0700 Subject: [PATCH] Multi-error compilation: default-on with --strict-errors opt-out flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavioral change: `p compile` now defaults to multi-error mode (reports all type errors in one pass). Replaces the previous opt-in P_COMPILER_COLLECT_ERRORS environment variable with a proper CLI flag `--strict-errors` (`-se`) for opting back into legacy abort-on-first behavior. Supersedes #968 (docs about the env var) and #969 (PeasyAI subprocess env-var setup) — both now redundant. - `CompilerConfiguration` — both constructors default `ContinueOnError` to true. Removed the `ReadContinueOnErrorEnvVar` helper. - `ICompilerConfiguration.ContinueOnError` XML doc updated to describe the new default and the `--strict-errors` opt-out. - `PCompilerOptions` — new `--strict-errors` / `-se` boolean flag. When passed, the parser sets `ContinueOnError = false` AND reconstructs the `Diagnostics` collector + `Handler` so the strict-mode collector throws on first Report. Order matters: collector first, then handler (which holds a reference to it). The motivating use case (PeasyAI's `peasy-ai-fix-all` loop) needs collecting mode to converge in O(N/k) LLM round trips instead of O(N). Env var propagation across subprocess boundaries is fragile and discoverability is poor (users have to know to set it). Making it the default delivers the value to every user automatically. Risk of breakage: zero for valid programs (no behavior change), exit code unchanged for invalid programs (still 1). Only user-visible difference is the NUMBER of diagnostics emitted per failed compile — users now see all errors at once. CI scripts that grep for an exact error count may need updating; they can opt out via `--strict-errors`. - `DiagnosticCollectorTest` — removed `ContinueOnError_ReadsEnvVar` (env var is gone). Replaced with `CompilerConfiguration_DefaultsToCollectingMode` which verifies `ContinueOnError == true`, `Diagnostics.ContinueOnError == true`, and the `config.Diagnostics === config.Handler.Diagnostics` shared- instance invariant. - `Phase1DormancyTest` and `MultiErrorAcceptanceTest` are unaffected — both explicitly override `config.ContinueOnError` after construction to run each mode. - `StaticErrorValidator` only checks `exitCode == 1`, so all existing StaticError/* regression tests pass without modification. - `README.md` — new "Multi-Error Compilation (P 3.0+)" section in "What's New" with worked example showing default + opt-out - `ChangeList.md` — entry under "P 3.0 Changes" - `CLAUDE.md` — top-level "Multi-Error Type Checking (Compiler)" section for contributors; also added the `--strict-errors` example in the "Working with P Programs" snippet - MCP tool descriptions (`peasy-ai-compile`, `peasy-ai-fix-all`) — updated to reflect that multi-error mode is now default - Stale env-var comment in `MultipleErrors.p` updated to say "Default (collecting)" - Close PR #968 (env var docs, now obsolete) - Close PR #969 (PeasyAI env var propagation, now obsolete) - Phase 3 (#967) may need a small rebase since it modifies the env-var test in DiagnosticCollectorTest. The test is renamed in this PR, resolving cleanly: Phase 3's `[NonParallelizable]` attribute change becomes a no-op (the method it was attached to no longer exists). Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 34 ++++++------ ChangeList.md | 26 +++++---- README.md | 26 ++++++--- .../CompilerCore/CompilerConfiguration.cs | 27 ++++----- .../CompilerCore/ICompilerConfiguration.cs | 13 ++--- .../PCommandLine/Options/PCompilerOptions.cs | 16 ++++++ Src/PeasyAI/src/ui/mcp/tools/compilation.py | 2 +- Src/PeasyAI/src/ui/mcp/tools/fixing.py | 2 +- .../MultipleErrors/MultipleErrors.p | 2 +- .../TypeChecker/DiagnosticCollectorTest.cs | 55 +++++-------------- 10 files changed, 97 insertions(+), 106 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 555d2c4dc..347eea8ed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -94,31 +94,28 @@ p compile --mode pex # Run model checking with PEx backend p check --mode pex -# Multi-error compilation — report ALL type errors in one pass -# instead of aborting on the first. Default is strict (one error, -# exit 1). Errors are sorted by source location. -P_COMPILER_COLLECT_ERRORS=1 p compile +# By default, `p compile` reports ALL type errors in one pass. +# Use --strict-errors / -se to restore the legacy abort-on-first behavior. +p compile --strict-errors ``` ## Multi-Error Type Checking (Compiler) -The C# compiler under `Src/PCompiler/CompilerCore/` supports a **collecting -mode** that gathers diagnostics through `IDiagnosticCollector` instead of -throwing on the first error. Opt in via the `P_COMPILER_COLLECT_ERRORS=1` -environment variable. Default (strict) mode is bit-for-bit identical to -pre-3.0 behavior for valid programs and same exit code on invalid programs. +The C# compiler under `Src/PCompiler/CompilerCore/` reports all type errors +in one pass by default (P 3.0+). Diagnostics flow through +`IDiagnosticCollector` and are flushed (sorted by source location) at end of +compilation. Users opt out via `--strict-errors` (`-se`), which restores the +legacy abort-on-first behavior. ### Architecture - **`IDiagnosticCollector` / `DefaultDiagnosticCollector`** — strict mode - rethrows immediately; collecting mode appends to a list, returns, and the - collector is flushed at end of compilation via - `Compiler.FlushCollectedDiagnostics`. + rethrows immediately; collecting mode (default) appends to a list. The + collector is flushed via `Compiler.FlushCollectedDiagnostics` at end of + compilation. - **`ErrorType` (singleton sentinel) / `ErrorExpr`** — substituted for - failed-to-type-check expressions in collecting mode. `ErrorType` claims - compatibility with every other type so downstream compatibility checks - cascade-suppress (one root-cause error doesn't generate a chain of - "incompatible operand" diagnostics). + failed-to-type-check expressions. `ErrorType` claims compatibility with + every other type so downstream compatibility checks cascade-suppress. - **`PLanguageType.IsSameTypeAs`** has a symmetric short-circuit when either side is `ErrorType`. - **`TypeCheckingUtils.CheckAssignable`** is the cascade-aware compatibility @@ -134,7 +131,8 @@ Follow the convention in `ExprVisitor.cs`'s class doc: 1. **Visit children first** — so their internal errors surface even if the parent lookup fails. -2. **Short-circuit on `ErrorType`** — `if (subExpr.Type is ErrorType) return new ErrorExpr(context);` +2. **Short-circuit on `ErrorType`** — + `if (subExpr.Type is ErrorType) return new ErrorExpr(context);` at the top of each method after visiting children. 3. **Convert each `throw handler.X(...)` to**: ```csharp @@ -161,7 +159,7 @@ This way, one bad item doesn't abort the pass for siblings in collecting mode. T ### Test fixtures -- **`Tst/UnitTests/TypeChecker/DiagnosticCollectorTest.cs`** — collector contract + env-var parsing +- **`Tst/UnitTests/TypeChecker/DiagnosticCollectorTest.cs`** — collector contract + default mode verification - **`Tst/UnitTests/TypeChecker/Phase1DormancyTest.cs`** — iterates every Correct/StaticError dir; asserts both modes succeed on Correct/ and that collecting count ≥ strict count on StaticError/ - **`Tst/UnitTests/TypeChecker/MultiErrorAcceptanceTest.cs`** — `[TestCaseSource]` with pinned strict/collecting counts on curated multi-error files under `Tst/RegressionTests/Feature3Exprs/StaticError/` diff --git a/ChangeList.md b/ChangeList.md index b475eaea7..6f91bd40d 100644 --- a/ChangeList.md +++ b/ChangeList.md @@ -1,18 +1,20 @@ P 3.0 Changes -=== Multi-Error Type Checker (#957, #963, #965) === -- Opt-in via `P_COMPILER_COLLECT_ERRORS=1` environment variable. -- When enabled, `p compile` reports all type errors in one pass instead of - aborting on the first error. Errors are sorted by source location. -- Cascade-suppression rules (ErrorType/ErrorExpr sentinels + - `TypeCheckingUtils.CheckAssignable`) prevent one root-cause error from +=== Multi-Error Type Checker (#957, #963, #965, #967, #970) === +- `p compile` now reports ALL type errors in one pass by default, sorted by + source location, instead of aborting on the first error. +- Cascade-suppression (ErrorType/ErrorExpr sentinels + + `TypeCheckingUtils.CheckAssignable`) prevents one root-cause error from generating downstream "incompatible operand" noise. -- Cross-machine `` lookup (in `x is ` test expressions) now - resolves against the instance's specific machine; same-named states in - unrelated machines no longer bind silently (#963). -- Default behavior (env var unset) is unchanged — strict mode still - aborts on the first error, bit-for-bit identical to pre-3.0 behavior - for valid programs and same exit code on invalid programs. +- Pass-level tolerance in `Analyzer.cs` (`TolerantStep`) so one bad + machine/function doesn't clobber diagnostics from its siblings (#967). +- Cross-machine `` lookup in `x is ` test expressions now + resolves against the instance's specific machine (#963). +- New CLI flag `--strict-errors` / `-se` opts back into legacy abort-on- + first behavior for users / CI scripts that depend on it (#970). +- Exit codes unchanged: 0 on success, 1 on any error. The change is + user-visible only in the number of diagnostics emitted per failed + compile. === First PL === - branch: `dev_p3.0/cleanup_targets` diff --git a/README.md b/README.md index 0a1f9b05a..b93e1d41f 100644 --- a/README.md +++ b/README.md @@ -59,22 +59,30 @@ Validate that production systems conform to their formal P specifications. 👉 [Learn about PObserve](https://p-org.github.io/P/advanced/pobserve/pobserve/) -### Multi-Error Compilation +### Multi-Error Compilation (P 3.0+) -Set `P_COMPILER_COLLECT_ERRORS=1` to make `p compile` report ALL type errors -in one pass instead of aborting on the first. Errors are sorted by source -location with cascade-suppression so root causes surface without downstream -noise. +`p compile` reports ALL type errors in one pass by default, sorted by source +location. Cascade-suppression keeps root causes surfacing without downstream +noise: -```bash -$ P_COMPILER_COLLECT_ERRORS=1 p compile +``` +$ p compile [Error:] [bad.p:6:4] got type: bool, expected: int [Error:] [bad.p:8:13] could not find variable 'undeclaredVar' [Error:] [bad.p:9:16] incompatible binary operands int and string ``` -Particularly useful with AI fix loops (PeasyAI, Cursor) and large refactors — -fix N errors per LLM round-trip instead of N round-trips per N errors. +Use `--strict-errors` (or `-se`) to restore the legacy abort-on-first +behavior: + +``` +$ p compile --strict-errors +[Error:] [bad.p:6:4] got type: bool, expected: int +``` + +The new default is particularly useful with AI fix loops (PeasyAI, Cursor) +and large refactors — fix N errors per LLM round-trip instead of N +round-trips per N errors. ## The P Framework diff --git a/Src/PCompiler/CompilerCore/CompilerConfiguration.cs b/Src/PCompiler/CompilerCore/CompilerConfiguration.cs index d1e432d40..0a34aaf33 100644 --- a/Src/PCompiler/CompilerCore/CompilerConfiguration.cs +++ b/Src/PCompiler/CompilerCore/CompilerConfiguration.cs @@ -37,7 +37,11 @@ public CompilerConfiguration() PObservePackageName = $"{ProjectName}.pobserve"; ProjectRootPath = new DirectoryInfo(Directory.GetCurrentDirectory()); LocationResolver = new DefaultLocationResolver(); - ContinueOnError = ReadContinueOnErrorEnvVar(); + // Collecting mode is the default — report all type errors in one + // pass instead of aborting on the first. Users opt OUT via the + // `--strict-errors` (`-se`) CLI flag, which the PCompilerOptions + // parser flips to false. + ContinueOnError = true; Diagnostics = new DefaultDiagnosticCollector(ContinueOnError); Handler = new DefaultTranslationErrorHandler(LocationResolver, Diagnostics); OutputLanguages = new List{CompilerOutput.PChecker}; @@ -49,19 +53,6 @@ public CompilerConfiguration() TargetProofBlocks = new List(); Parallelism = Math.Max(Environment.ProcessorCount / 2, 1); } - - /// - /// Reads the P_COMPILER_COLLECT_ERRORS environment variable. - /// Any non-empty value other than "0"/"false" (case-insensitive) - /// enables collecting mode. Defaults to false (strict, throw-on-first). - /// - private static bool ReadContinueOnErrorEnvVar() - { - var raw = Environment.GetEnvironmentVariable("P_COMPILER_COLLECT_ERRORS"); - if (string.IsNullOrWhiteSpace(raw)) return false; - return !raw.Equals("0", StringComparison.OrdinalIgnoreCase) - && !raw.Equals("false", StringComparison.OrdinalIgnoreCase); - } /// /// Initializes a new instance of the class with specific settings. /// @@ -102,7 +93,8 @@ public CompilerConfiguration(ICompilerOutput output, DirectoryInfo outputDir, IL PObservePackageName = pObservePackageName ?? $"{ProjectName}.pobserve"; ProjectRootPath = projectRoot; LocationResolver = new DefaultLocationResolver(); - ContinueOnError = ReadContinueOnErrorEnvVar(); + // See parameterless ctor for collecting-mode rationale. + ContinueOnError = true; Diagnostics = new DefaultDiagnosticCollector(ContinueOnError); Handler = new DefaultTranslationErrorHandler(LocationResolver, Diagnostics); OutputLanguages = outputLanguages; @@ -112,8 +104,9 @@ public CompilerConfiguration(ICompilerOutput output, DirectoryInfo outputDir, IL } /// - /// See . Wired from - /// P_COMPILER_COLLECT_ERRORS at construction. Phase 1 wiring only. + /// See . Defaults + /// to true (collecting mode). Flipped to false by the CLI flag + /// --strict-errors / -se in PCompilerOptions. /// public bool ContinueOnError { get; set; } diff --git a/Src/PCompiler/CompilerCore/ICompilerConfiguration.cs b/Src/PCompiler/CompilerCore/ICompilerConfiguration.cs index 396178fa0..4c8f75287 100644 --- a/Src/PCompiler/CompilerCore/ICompilerConfiguration.cs +++ b/Src/PCompiler/CompilerCore/ICompilerConfiguration.cs @@ -26,13 +26,12 @@ public interface ICompilerConfiguration int Parallelism { get; } /// - /// When true, the type checker collects diagnostics and continues - /// instead of throwing on the first error. See - /// for the contract. Driven by env var P_COMPILER_COLLECT_ERRORS - /// (any non-empty / non-"0" value enables it). - /// - /// Phase 1: scaffolding only. No visitor currently reports through the - /// collector, so flipping this flag has no observable effect yet. + /// When true (the default), the type checker collects diagnostics and + /// continues instead of throwing on the first error. See + /// for the contract. + /// Users opt OUT of collecting via the CLI flag + /// --strict-errors / -se, which restores the legacy + /// abort-on-first behavior. /// bool ContinueOnError { get; } diff --git a/Src/PCompiler/PCommandLine/Options/PCompilerOptions.cs b/Src/PCompiler/PCommandLine/Options/PCompilerOptions.cs index 5fc51c400..0af92260d 100644 --- a/Src/PCompiler/PCommandLine/Options/PCompilerOptions.cs +++ b/Src/PCompiler/PCommandLine/Options/PCompilerOptions.cs @@ -75,6 +75,11 @@ internal PCompilerOptions() Parser.AddArgument("pobserve-package", "po", "PObserve package name").IsHidden = true; Parser.AddArgument("debug", "d", "Enable debug logs", typeof(bool)).IsHidden = true; + + Parser.AddArgument("strict-errors", "se", + "Abort on the first type error (legacy P 2.x behavior). " + + "By default, the compiler reports all type errors in one pass.", + typeof(bool)); var pvGroup = Parser.GetOrCreateGroup("pverifier", "PVerifier options"); pvGroup.AddArgument("timeout", "t", "Set SMT solver timeout in seconds", typeof(int)).IsHidden = true; @@ -168,6 +173,17 @@ private static void UpdateConfigurationWithParsedArgument(CompilerConfiguration case "debug": compilerConfiguration.Debug = true; break; + case "strict-errors": + // Opt out of collecting mode. Replaces the parameterless + // ctor's default (true) — and reconstructs the collector + + // handler so the strict-mode collector throws on first + // Report rather than appending. Order matters: collector + // first, then handler (which holds a reference to it). + compilerConfiguration.ContinueOnError = false; + compilerConfiguration.Diagnostics = new DefaultDiagnosticCollector(continueOnError: false); + compilerConfiguration.Handler = new DefaultTranslationErrorHandler( + compilerConfiguration.LocationResolver, compilerConfiguration.Diagnostics); + break; case "timeout": compilerConfiguration.Timeout = (int)option.Value; break; diff --git a/Src/PeasyAI/src/ui/mcp/tools/compilation.py b/Src/PeasyAI/src/ui/mcp/tools/compilation.py index 579d58033..8d757f267 100644 --- a/Src/PeasyAI/src/ui/mcp/tools/compilation.py +++ b/Src/PeasyAI/src/ui/mcp/tools/compilation.py @@ -36,7 +36,7 @@ def register_compilation_tools(mcp, get_services, with_metadata): @mcp.tool( name="peasy-ai-compile", - description="Compile a P project and return compilation results. The project directory must contain a .pproj file. On failure, the response includes parsed errors with file, line, and message details. Use peasy-ai-fix-compile-error or peasy-ai-fix-all to resolve compilation errors. Tip: set P_COMPILER_COLLECT_ERRORS=1 in the project environment to receive ALL type errors in a single response (compiler 3.0+) — lets fix-all converge in fewer iterations." + description="Compile a P project and return compilation results. The project directory must contain a .pproj file. On failure, the response includes ALL parsed errors with file, line, and message details (P compiler 3.0+ defaults to multi-error mode). Use peasy-ai-fix-compile-error or peasy-ai-fix-all to resolve compilation errors." ) def p_compile(params: PCompileParams) -> Dict[str, Any]: logger.info(f"[TOOL] peasy-ai-compile: {params.path}") diff --git a/Src/PeasyAI/src/ui/mcp/tools/fixing.py b/Src/PeasyAI/src/ui/mcp/tools/fixing.py index 67337ade3..5fa7ad701 100644 --- a/Src/PeasyAI/src/ui/mcp/tools/fixing.py +++ b/Src/PeasyAI/src/ui/mcp/tools/fixing.py @@ -410,7 +410,7 @@ def fix_checker_error(params: FixCheckerErrorParams) -> Dict[str, Any]: @mcp.tool( name="peasy-ai-fix-all", - description="Iteratively compile, detect errors, and fix them in a loop until the project compiles successfully or max_iterations is reached. This is the recommended way to fix multiple compilation errors at once — it automatically re-compiles after each fix to catch cascading issues. Use this instead of calling peasy-ai-fix-compile-error repeatedly. Tip: with P compiler 3.0+, set P_COMPILER_COLLECT_ERRORS=1 to get all type errors per compile in one batch — the loop converges in N/k iterations instead of N, where k is the average errors-per-iteration." + description="Iteratively compile, detect errors, and fix them in a loop until the project compiles successfully or max_iterations is reached. This is the recommended way to fix multiple compilation errors at once — it automatically re-compiles after each fix to catch cascading issues. With P compiler 3.0+ (multi-error mode is default), each compile reports all current errors so the loop converges in O(N/k) iterations instead of O(N). Use this instead of calling peasy-ai-fix-compile-error repeatedly." ) def fix_iteratively(params: FixIterativelyParams) -> Dict[str, Any]: logger.info(f"[TOOL] peasy-ai-fix-all: {params.project_path}") diff --git a/Tst/RegressionTests/Feature3Exprs/StaticError/MultipleErrors/MultipleErrors.p b/Tst/RegressionTests/Feature3Exprs/StaticError/MultipleErrors/MultipleErrors.p index 75664e4c3..0365adb0b 100644 --- a/Tst/RegressionTests/Feature3Exprs/StaticError/MultipleErrors/MultipleErrors.p +++ b/Tst/RegressionTests/Feature3Exprs/StaticError/MultipleErrors/MultipleErrors.p @@ -22,7 +22,7 @@ // - Strict mode (default): exit 1 after the first error is reported. // Identical to the historical behavior — this file is no different // from any other single-error StaticError test in this mode. -// - Collecting mode (P_COMPILER_COLLECT_ERRORS=1): exit 1 after +// - Default (collecting): exit 1 after // reporting all 4 independent errors. The Phase1DormancyTest // fixture asserts `collecting_count >= strict_count`, which holds // trivially with strict=1; the stronger acceptance check lives in diff --git a/Tst/UnitTests/TypeChecker/DiagnosticCollectorTest.cs b/Tst/UnitTests/TypeChecker/DiagnosticCollectorTest.cs index 885defc5a..17f973127 100644 --- a/Tst/UnitTests/TypeChecker/DiagnosticCollectorTest.cs +++ b/Tst/UnitTests/TypeChecker/DiagnosticCollectorTest.cs @@ -170,46 +170,21 @@ public void Diagnostics_IsLiveView_NotSnapshot() Assert.AreEqual("late arrival", liveView[0].Message); } - // Env var parsing — see CompilerConfiguration.ReadContinueOnErrorEnvVar. - // Tested via the public ContinueOnError property after construction so we - // don't have to mark the private helper internal. Each case saves and - // restores the var to keep test isolation. - // [NonParallelizable] because this test mutates a process-global env var - // (Environment.SetEnvironmentVariable). Any parallel test that constructs - // a CompilerConfiguration would read the temporarily-set value and run - // in the wrong mode — silently masking real strict-mode regressions. - // NUnit runs tests in different fixtures in parallel by default; this - // attribute forces sequential execution for THIS test. - [TestCase("1", true)] - [TestCase("true", true)] - [TestCase("True", true)] - [TestCase("TRUE", true)] - [TestCase("yes", true)] // any non-"0"/"false" non-empty value wins - [TestCase("anything-else", true)] - [TestCase("0", false)] - [TestCase("false", false)] - [TestCase("False", false)] - [TestCase("FALSE", false)] - [TestCase("", false)] // empty string treated as unset - [TestCase(" ", false)] // whitespace-only treated as unset - [TestCase(null, false)] // unset - [NonParallelizable] - public void ContinueOnError_ReadsEnvVar(string envValue, bool expected) + [Test] + public void CompilerConfiguration_DefaultsToCollectingMode() { - const string envName = "P_COMPILER_COLLECT_ERRORS"; - var saved = Environment.GetEnvironmentVariable(envName); - try - { - Environment.SetEnvironmentVariable(envName, envValue); - var config = new CompilerConfiguration(); - Assert.AreEqual(expected, config.ContinueOnError, - $"P_COMPILER_COLLECT_ERRORS={envValue ?? "(unset)"}"); - // The collector's mode must agree with the config's flag. - Assert.AreEqual(expected, config.Diagnostics.ContinueOnError); - } - finally - { - Environment.SetEnvironmentVariable(envName, saved); - } + // P 3.0+ contract: collecting mode is the default. Users opt out via + // the `--strict-errors` CLI flag, which PCompilerOptions flips after + // construction. If this test fails, the default has regressed — check + // the parameterless CompilerConfiguration constructor. + var config = new CompilerConfiguration(); + Assert.IsTrue(config.ContinueOnError, + "CompilerConfiguration() must default ContinueOnError to true (collecting mode)"); + Assert.IsNotNull(config.Diagnostics); + Assert.IsTrue(config.Diagnostics.ContinueOnError, + "The collector's mode must agree with the config's flag at construction"); + // Handler.Diagnostics must be the same instance as config.Diagnostics + // (invariant guarded by DefaultTranslationErrorHandler's null-throw). + Assert.AreSame(config.Diagnostics, config.Handler.Diagnostics); } }