Multi-error compilation: default-on with --strict-errors opt-out#970
Open
ankushdesai wants to merge 1 commit into
Open
Multi-error compilation: default-on with --strict-errors opt-out#970ankushdesai wants to merge 1 commit into
ankushdesai wants to merge 1 commit into
Conversation
2 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes multi-error type checking from opt-in to default behavior for p compile, adding a CLI opt-out for legacy strict error handling and updating related docs/tests/tool descriptions.
Changes:
- Defaults
CompilerConfigurationto collecting mode and adds--strict-errors/-seto restore abort-on-first behavior. - Replaces the env-var unit test with a default-mode configuration test.
- Updates user, contributor, changelog, fixture, and PeasyAI MCP descriptions for default multi-error compilation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
Src/PCompiler/CompilerCore/CompilerConfiguration.cs |
Defaults compiler configs to collecting mode. |
Src/PCompiler/CompilerCore/ICompilerConfiguration.cs |
Updates API documentation for default collecting mode and strict opt-out. |
Src/PCompiler/PCommandLine/Options/PCompilerOptions.cs |
Adds --strict-errors / -se handling. |
Tst/UnitTests/TypeChecker/DiagnosticCollectorTest.cs |
Replaces env-var behavior test with default collecting-mode test. |
Tst/RegressionTests/Feature3Exprs/StaticError/MultipleErrors/MultipleErrors.p |
Updates fixture comments for default collecting mode. |
README.md |
Adds user-facing multi-error compilation documentation. |
ChangeList.md |
Adds P 3.0 changelog entry for multi-error type checking. |
CLAUDE.md |
Adds contributor guidance for multi-error type checking. |
Src/PeasyAI/src/ui/mcp/tools/compilation.py |
Updates compile tool description for multi-error output. |
Src/PeasyAI/src/ui/mcp/tools/fixing.py |
Updates fix-all tool description for default multi-error compilation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @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." | ||
| 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." |
Comment on lines
+176
to
+185
| 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); |
Comment on lines
+64
to
+66
| `p compile` reports ALL type errors in one pass by default, sorted by source | ||
| location. Cascade-suppression keeps root causes surfacing without downstream | ||
| noise: |
Comment on lines
+4
to
+5
| - `p compile` now reports ALL type errors in one pass by default, sorted by | ||
| source location, instead of aborting on the first error. |
Comment on lines
+106
to
+108
| `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. |
Comment on lines
15
to
17
| // - 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. |
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) <noreply@anthropic.com>
ankushdesai
force-pushed
the
ankush/multi-error-default-on
branch
from
2d56ad1 to
aac588f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Behavioral change:
p compilenow reports all type errors in one pass by default. Replaces the previous opt-inP_COMPILER_COLLECT_ERRORSenv var with a proper CLI flag--strict-errors(-se) for opting back into legacy abort-on-first behavior.Supersedes:
What changed
Compiler
CompilerConfiguration— both constructors defaultContinueOnErrortotrue. RemovedReadContinueOnErrorEnvVarhelper.PCompilerOptions— new--strict-errors/-seboolean flag. When passed, parser setsContinueOnError = falseAND reconstructsDiagnostics+Handlerso the strict-mode collector throws on first Report (preserves legacy abort-on-first).Tests
DiagnosticCollectorTest.ContinueOnError_ReadsEnvVar→ replaced withCompilerConfiguration_DefaultsToCollectingModewhich verifies the new default + theconfig.Diagnostics === config.Handler.Diagnosticsshared-instance invariant.Phase1DormancyTestandMultiErrorAcceptanceTestare unaffected — both explicitly overrideconfig.ContinueOnErrorafter construction.StaticErrorValidatoronly checksexitCode == 1, so all existingStaticError/*regression tests pass unchanged.Docs
README.md— new "Multi-Error Compilation (P 3.0+)" section in "What's New" with worked exampleChangeList.md— entry under "P 3.0 Changes"CLAUDE.md— new top-level "Multi-Error Type Checking (Compiler)" contributor sectionWhy default-on?
The motivating use case (PeasyAI's
peasy-ai-fix-allloop) needs collecting mode to converge in O(N/k) LLM round trips instead of O(N). Env var propagation across subprocess boundaries was fragile (PR #969 was opened specifically to handle this — now unnecessary) and discoverability was poor.Compatibility
--strict-errors.Test plan
Tst/UnitTests/**pass with the new defaultTst/RegressionTests/**/StaticError/**and**/Correct/**pass (validated only on exit code, which is preserved)🤖 Generated with Claude Code