Type checker: wire up @expect directive suppression for type/dnu warnings#1015
Type checker: wire up @expect directive suppression for type/dnu warnings#1015
Conversation
…nings `species` returns the receiver's metaclass at runtime, which is inherently dynamic. Declaring `-> Object` caused spurious "Object does not understand 'withAll:'" warnings at every callsite using the species pattern (e.g. Collection#collect:, Collection#select:). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes statement-sequence type inference into a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
`@expect dnu` and `@expect type` were already parsed into AST nodes and handled by `apply_expect_directives` in diagnostic_provider, but the type checker had no knowledge of them — `ExpectDirective` nodes reset the inferred body type to Dynamic, breaking return-type inference. Changes: - Add `infer_stmts` helper that skips `ExpectDirective` nodes so they don't clobber the inferred body type used for return-type checking - Replace all three method-body loops and the block-body loop with `infer_stmts` - Tag all previously-uncategorised type checker diagnostics with `DiagnosticCategory::Type` so `@expect type` can suppress them - Remove the `species` special-case (hack) added in the previous commit — the right fix is `@expect dnu` at the two callsites in Collection.bt - Add `@expect dnu` before `self species withAll: result` in `collect:` and `select:` — suppression is validated at build time (stale @expect is a compile error) - Add integration tests that go through the full pipeline (type checker + `apply_expect_directives`) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
stdlib/src/Collection.bt (1)
140-141: Consider adding a brief comment for consistency.The
@expect dnuincollect:(line 124) has an explanatory comment while this one doesn't. A one-line comment like// species withAll: — see collect: for rationalewould help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stdlib/src/Collection.bt` around lines 140 - 141, Add a one-line explanatory comment above the "@expect dnu" in the species-withAll code path to match collect:'s style; for example, add "// species withAll: — see collect: for rationale" immediately before the "@expect dnu" that precedes "self species withAll: result" so future readers know the rationale and can find the related explanation in collect:.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@stdlib/src/Collection.bt`:
- Around line 140-141: Add a one-line explanatory comment above the "@expect
dnu" in the species-withAll code path to match collect:'s style; for example,
add "// species withAll: — see collect: for rationale" immediately before the
"@expect dnu" that precedes "self species withAll: result" so future readers
know the rationale and can find the related explanation in collect:.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
crates/beamtalk-core/src/semantic_analysis/type_checker.rsstdlib/src/Collection.bt
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
@expect dnuand@expect typesuppression in the type checker so@expectdirectives properly suppress type-checker diagnosticsDiagnosticCategory::Typeso@expect typecan match theminfer_stmtshelper that skipsExpectDirectiveAST nodes during body iteration (prevents them from clobbering return-type inference)@expect dnuannotations toCollection.btcollect:andselect:methods to suppress false "Object does not understand 'withAll:'" warnings from the species patternContext
self species withAll: resultreturns the receiver's metaclass at runtime — inherently dynamic, cannot be type-checked statically. Thespeciesmethod returns-> Object, but the actual class at runtime will understandwithAll:. The@expect dnudirective suppresses this known-safe warning.Test plan
just test— all Rust tests pass (including 3 new@expectintegration tests)just build— stdlib builds cleanly, no morewithAll:warningsjust test-stdlib— all tests passjust clippy— clean🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
Tests