Repository navigation
test(lsp): make didOpen filesystem assertion deterministic - #4626
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe protocol test now uses explicit request, diagnostic, and manual I/O executors. It verifies that ChangesProtocol test control
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR makes the LSP filesystem assertion deterministic without changing production behavior or public interfaces. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
hellovai
changed the base branch from
codex/error-extraneous-throws-unknown
to
canary
August 28, 2026 21:21
hellovai
force-pushed
the
codex/fix-did-open-discovery-race
branch
from
August 28, 2026 21:21
889b9a9 to
007551a
Compare
6 tasks done
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
pull Bot
pushed a commit
to rpatil524/baml
that referenced
this pull request
Aug 29, 2026
…ML#4593) ## Stack context [BoundaryML#4626](BoundaryML#4626), the independent deterministic LSP test fix that formed the lower stack layer, is merged into `canary`. [BoundaryML#4621](BoundaryML#4621), whose pure-BAML compiler-test harness is used here, is also merged. Review this PR relative to `canary`. ## Problem BAML already infers the values that can escape through `throw`. An explicit `throws` clause is a bound on those inferred values, but `unknown` was treated as an automatic escape hatch from unused-contract diagnostics. That allowed declarations such as these to compile without explaining the real behavior: ```baml function DoesNotThrow() -> int throws unknown { 1 } function ThrowsOnlyInt() -> int throws unknown { throw 1 } ``` The first function does not throw. The second throws only `int`. In both cases, `throws unknown` is broader than the implementation and hides useful compiler information. This precision requirement is specific to `unknown`. An ordinary explicit bound may remain conservative: `throws A | B | C` is valid when the function only throws `A | B`. ## After this PR Both imprecise `unknown` declarations are rejected with E0097. For a function that does not throw: ```text `throws unknown` is unnecessary: BAML infers thrown types automatically, and this function does not throw. Remove the declaration; write an explicit `throws` type only to bound what may escape ``` For a function whose inferred thrown type is narrower: ```text `throws unknown` is imprecise: this function only throws `int`. BAML infers thrown types automatically, so remove the declaration; write `throws int` only to explicitly bound what may escape ``` There are three intended remedies. 1. Let BAML infer the thrown type: ```baml function ThrowsOnlyInt() -> int { throw 1 } ``` 2. Keep an explicit, precise bound when the API contract should spell it out: ```baml function ThrowsOnlyInt() -> int throws int { throw 1 } ``` 3. Keep `throws unknown` at a real unknown boundary: ```baml function Rethrow(value: unknown) -> int throws unknown { throw value } ``` ## Behavioral boundaries | Declaration and implementation | Result | | --- | --- | | `throws A \| B \| C`, only `A \| B` escapes | Compiles; ordinary explicit bounds may be wider than inferred throws | | `throws unknown`, no escaping throw | E0097: unnecessary | | `throws unknown`, only `int` escapes | E0097: imprecise | | `throws string \| unknown`, only `string` escapes | E0097: imprecise | | `throws unknown`, an `unknown` value escapes directly | Compiles | | `throws Alias`, where `type Alias = unknown`, and an unknown value escapes | Compiles | | `throws unknown`, unknown escapes through a called function | Compiles | | `throws string \| unknown`, both string and unknown can escape | Compiles | | `throws unknown \| UnusedError`, unknown escapes but `UnusedError` does not | Compiles with the existing E0097 warning for the unused concrete member | | Signature-only interface requirement with `throws unknown` | Remains a valid contract | ## Diagnostic edge cases ### Concrete superset bounds The compiler does not require every concrete member of an explicit `throws` union to appear in the current function body. A bound such as `throws A | B | C` may intentionally reserve `C` for implementations or future behavior even when inference currently finds only `A | B`. `unknown` is different because it admits every value and therefore erases all useful precision when no unknown-typed value can escape. Only that unused open member becomes an error. ### Written union surfaces Finalizing `unknown | SomeError` produces semantic `unknown`, which would normally erase `SomeError`. The coverage check therefore preserves the written surface for open unions so that an unused concrete member still receives the existing warning. Other contracts continue to use the finalized surface. This avoids false unused-member warnings for types that reduce through projections or aliases. ### Aliases and propagated throws A direct `unknown`, an alias that resolves to `unknown`, and an `unknown` value propagated through a call all satisfy the contract. The diagnostic is based on the inferred escaping type, not only on a syntactic `throw` in the current function. ### Compiler acceptance coverage The former `ns_fixtures/ns_throws_unknown` corpus fixture has been replaced with explicit `AssertCompiles` cases. Direct and aliased unknown bounds, mixed unions, signature-only interface bounds, ordinary unknown value flow, and callback error translation now state their expected zero-error behavior at the public compiler boundary. Iterator functions that either do not throw or fully handle their errors no longer declare `throws unknown`. The broader Rust integration-test programs also omit 177 executable `throws unknown` convenience clauses. Signature-only interface requirements, callback and function types, pattern checks, and the dedicated precision tests retain their explicit open contracts. ## Diagnostic assertions The public behavior is covered in pure BAML at: ```text crates/baml_tests/baml_src/ns_compiler/ns_throws/ns_unknown_contracts/precision.baml ``` The tests use the shared `reflect.Package._compile` harness from BoundaryML#4621: - `AssertRejected` and `ExactDiagnostic` lock the exact E0097 code and message for direct, union, aliased, no-throw, and narrower-throw cases. - `AssertCompiles` locks concrete superset bounds, accepted unknown boundaries, transitive propagation, interface signatures, and callback translation. - A focused compiler test verifies that an unused concrete member beside `unknown` remains a warning; successful `_compile` results intentionally do not expose warnings. ## Validation - [x] 9 pure-BAML `reflect.Package._compile` contract tests - [x] Compiler/corpus snapshot coverage updated for the removed fixture - [x] Mechanical audit of 177 executable test-helper migrations - [x] Intentional signature and function-type `throws unknown` surfaces preserved - [x] Diff and whitespace checks - [ ] CI after integration-test migration --- <sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added clearer diagnostics for missing required fields in class constructors and object literals. * Improved guidance when generic functions or methods are used without specialization. * Enhanced validation and messaging for `throws unknown` contracts, including imprecise or unused error types. * **Bug Fixes** * Prevented duplicate diagnostics for generated stream companions. * Preserved warnings for unused concrete error types in open throws contracts. * Improved handling of empty build metadata. * **Tests** * Added coverage for precise `throws unknown` behavior and updated iterator exception scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: hellovai <[email protected]>
This branch was previously deployed
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.
Stack context
This PR targets
canaryand is the bottom layer of the active stack. #4593 is stacked on top of it. The change is independent of the compiler behavior in that PR and can land first.Problem
did_open_never_reads_the_opened_fileuses a real thread pool for filesystem discovery.After
h.open(...)queues discovery, a worker may immediately discover the project and readother.bamlbefore the test checks that the read log is empty:The resulting failure does not mean that the opened
main.bamlbuffer was read from disk. It only means that legitimate asynchronous sibling-file discovery won the scheduling race.After this PR
This test supplies a manual executor for the I/O lane while retaining real thread pools for request and diagnostic work.
The sequence is now deterministic:
didOpen.other.baml.The production LSP implementation is unchanged.
Behavioral boundaries
didOpenuses the editor buffer and performs no synchronous disk readmain.bamlnever appears in the read logother.bamlValidation
Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by CodeRabbit