Conversation
- Add TypeInfoKind::Error variant to represent unresolved or invalid types - Update TypeChecker to gracefully handle TypeInfoKind::Error by suppressing cascading errors - Update expression inference (Identifier, Struct, MemberAccess, FunctionCall, etc.) to return Error type on failure instead of None/cascading - Prevent spurious type mismatch errors when one of the types is Error
…soning) - Add tests for TypeInfoKind::Error variant behavior (is_error, Display, substitute, has_unresolved_params) - Add tests for cascading error suppression with undeclared variables, undefined structs/functions - Add tests for error propagation through assignments, member access, method calls, function arguments - Add tests for error types in complex expressions (binary ops, arrays, conditionals) - Register error_type_poisoning_tests module in mod.rs
|
As pointed out in the contributor guide
|
|
@SurfingBowser updated. |
|
@SurfingBowser kindly review |
There was a problem hiding this comment.
Pull request overview
This PR implements Error Type Poisoning to prevent cascading errors in the type checker, following rustc's TyKind::Error model. When a type lookup fails, the type checker now uses TypeInfoKind::Error to suppress subsequent type mismatch errors that would otherwise cascade from the initial failure.
Changes:
- Added
TypeInfoKind::Errorvariant with supporting methods (is_error(), display as{unknown}) - Updated type checker to return
Errortypes on lookup failures and suppress cascading type mismatch errors - Added comprehensive test suite covering error poisoning behavior across various scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/src/type_checker/mod.rs | Registered new error type poisoning test module |
| tests/src/type_checker/error_type_poisoning_tests.rs | Comprehensive test suite verifying error type behavior and cascading error suppression |
| core/type-checker/src/type_info.rs | Added Error variant to TypeInfoKind enum with display and helper methods |
| core/type-checker/src/type_checker.rs | Updated type checking logic to use error types for poisoning and suppress cascading errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@0xGeorgii done, I Implemented Error variant now takes a String message. |
There was a problem hiding this comment.
I think this is the only case where None is returned from this function. Is there a way to handle this better and make the function not return Option?
|
@0xGeorgii kindly review |
|
@CMI-James thank for your contribution, here are several comments: Critical Issues
In type_checker.rs, the let cond_type = self.infer_expression(condition, ctx);
if !cond_type.is_error() {
if !cond_type.is_bool() {
self.errors.push(TypeCheckError::TypeMismatch { ... });
}
} else {
// BUG: Still pushes a TypeMismatch error when cond_type is Error!
self.errors.push(TypeCheckError::TypeMismatch {
expected: TypeInfo::boolean(),
found: TypeInfo::default(), // Also wrong: uses Unit instead of the actual Error type
context: TypeMismatchContext::Condition,
location: ...,
});
}The else branch fires when cond_type.is_error() is true. Error poisoning should suppress the cascading error, not emit one. The entire
Fix: Remove the else block entirely. When cond_type.is_error(), do nothing.
OperatorKind::And | OperatorKind::Or => {
if left_type.is_bool() && right_type.is_bool() {
TypeInfo::boolean()
} else {
self.errors.push(TypeCheckError::InvalidBinaryOperand { ... }); // CASCADE!
TypeInfo::error("Invalid binary operand")
}
}When one operand is Error, is_bool() returns false, so this falls into the error branch and emits InvalidBinaryOperand -- a cascading error. Should
OperatorKind::Add | OperatorKind::Sub | ... => {
if !left_type.is_number() || !right_type.is_number() {
self.errors.push(TypeCheckError::InvalidBinaryOperand { ... }); // CASCADE!
}
// ...
}Error types are not numbers, so this fires when one operand is Error. Same cascade issue.
A type-mismatch check before the operator-specific match: if !left_type.is_error() && !right_type.is_error() && left_type != right_type {
self.errors.push(TypeCheckError::BinaryOperandTypeMismatch { ... });
}But the arithmetic arm ALSO checks: if left_type != right_type {
self.errors.push(TypeCheckError::BinaryOperandTypeMismatch { ... });
}For non-Error mismatched arithmetic types, BOTH checks fire, producing duplicate errors. The original code only had the second check (inside the if
Old code for undefined functions: if let Some(arguments) = &function_call_expression.arguments {
for arg in arguments {
self.infer_expression(&arg.1.borrow(), ctx);
}
}
return None;New code: return TypeInfo::error("Poisoned type from error");The new code skips argument inference entirely. This means errors within arguments of an undefined function call won't be detected. This is a Significant Issues
The error messages are ad-hoc strings like "Poisoned type from error", "Expected array type", "Undefined identifier", "Method not found", etc. These strings are not user-facing (they go into TypeInfoKind::Error(String) for internal tracking), but they're inconsistent in style. Some describe what happened, some describe what was expected. Consider using a more structured approach or at minimum a consistent naming convention.
Tests primarily check that error messages contain certain keywords: But they don't verify:
Stronger tests should count errors or check for absence of specific TypeCheckError variants. Minor Issues
The test file error_type_poisoning_tests.rs wraps its module in #[cfg(test)]. Since it's already under tests/src/, the #[cfg(test)] is redundant.
|
- Fixed cascading errors in Loop, If, and Assert statements - Fixed cascading errors in binary logical and arithmetic operators - Deduplicated BinaryOperandTypeMismatch errors - Restored argument inference for undefined function calls - Fixed empty array literal returning Error type - Removed redundant #[cfg(test)] from test file
|
@0xGeorgii review. |
|
@CMI-James thanks for addressing the previous round. The critical bugs from round 1 (Loop/If/Assert cascading, binary operator guards, duplicate BinaryOperandTypeMismatch, argument inference for undefined functions) are fixed. there are remaining issues: Critical Issues
OperatorKind::Add | OperatorKind::Sub | ... => {
if !left_type.is_error() && !right_type.is_error() {
if !left_type.is_number() || !right_type.is_number() {
self.errors.push(TypeCheckError::InvalidBinaryOperand { ... });
}
}
left_type.clone() // <-- always returns left_type
}When right_type is Error but left_type is valid (e.g. 5 + unknown_var), this returns i32 -- a valid type -- even though the expression is unresolved. Downstream code will treat this as well-typed. Fix: Return TypeInfo::error(...) when either operand is Error, similar to the And/Or arm: if left_type.is_error() || right_type.is_error() {
return if left_type.is_error() { left_type.clone() } else { right_type.clone() };
}
When receiver_type.is_error(), the code returns early without inferring function call arguments. The old code explicitly inferred all arguments even when the receiver type was None (for error Fix: Infer arguments before returning the error type, matching the old None branch behavior.
TypeInfoKind derives PartialEq, so Error("foo") != Error("bar"). If two different error-producing expressions are compared (e.g. in assignment), the is_error() guards prevent issues today, but this Industry standard (rustc, rust-analyzer, chalk) uses either a unit variant Error with no payload, or Error(ErrorGuaranteed) with a zero-sized proof token. The String payload creates:
Recommendation: Use TypeInfoKind::Error (unit variant, no payload). If you want the error message for debugging, it's already captured in self.errors. Significant Issues
Eq/Ne/Lt/Le/Gt/Ge always return TypeInfo::boolean(), even when one or both operands are Error. This diverges from the pattern used by And/Or and arithmetic. The practical impact is low (comparison
The test file checks string contents (error_msg.contains("keyword")) but never verifies the total number of errors. For a feature specifically about suppressing cascading errors, tests should split let errors: Vec<&str> = error_msg.split("; ").collect();
Expression::Uzumaki returns TypeInfo::error("Uzumaki type not inferred") but pushes no TypeCheckError. The user gets no diagnostic. If this path is unreachable (uzumaki type is always pre-set), use
return_type.is_error() in Statement::Return and target_type.is_error() in Statement::VariableDefinition can never be true -- these types come from TypeInfo::new() / TypeInfo::new_with_type_params() Style / Convention Issues
|
0xGeorgii
left a comment
0xGeorgii
left a comment
There was a problem hiding this comment.
Please address issues from my comment
Critical fixes:
- Arithmetic ops now propagate Error symmetrically (5+unknown == unknown+5)
- Method call on error receiver now infers arguments before returning
- Changed Error(String) to unit variant Error for proper equality semantics
Significant fixes:
- Comparison operators now propagate Error for consistency
- Uzumaki expression now emits diagnostic when type not inferred
- Added comments explaining defensive guards on declared types
- Tests now verify error counts using split('; ')
Style fixes:
- Fixed double space in Statement::If
- Added #[must_use] reason strings to error() and is_error()
- Added #[cfg(test)] attribute to test module
- Added doc comments to error() and is_error() methods
|
@0xGeorgii review |
3 participants
Description
Closes #72
Changes proposed
What were you told to do?
I was tasked with implementing Error Type Poisoning to prevent cascading errors in the type checker. This involved adding a
TypeInfoKind::Errorvariant similar torustc'sTyKind::Errorand updating the type checker to gracefully handle these error types.What did I do?
Implemented
TypeInfoKind::ErrorErrorvariant to theTypeInfoKindenum incore/type-checker/src/type_info.rs.Displayimplementation to show{unknown}for error types.is_error()helper method toTypeInfo.substituteandhas_unresolved_paramsto handleErrorvariants neutrally.Updated Type Checker Logic
core/type-checker/src/type_checker.rsto useTypeInfoKind::Errorfor type poisoning.infer_expressionforIdentifier,Structinitialization,MemberAccess,TypeMemberAccess, andFunctionCallto returnTypeInfoKind::Errorupon lookup failure instead ofNoneor continuing with invalid types.Statement::Assign,Statement::Return,Statement::If,Statement::Loop, and others to suppress errors if either the expected or found type isError.Expression::ArrayIndexAccess,Expression::PrefixUnary, andExpression::Binaryto propagateErrortypes and suppress secondary errors (e.g., "expected array type" when the array expression itself failed).Added Tests
tests/src/type_checker/error_type_poisoning_tests.rswith comprehensive tests covering:TypeInfoKind::Errorvariant behavior (is_error(),Display,substitute,has_unresolved_params)Benefits
rustc's error handling model.AI Disclosure
AI tools (Claude) were used to assist with the following parts of this implementation:
AI-generated code (reviewed and tested before submission):
core/type-checker/src/type_info.rs: AddedTypeInfoKind::Errorvariant,is_error()method,Displaymatch arm forError, and handling insubstitute/has_unresolved_paramscore/type-checker/src/type_checker.rs: Updated type mismatch checks to skip errors when either type isError, modifiedinfer_expressionbranches to returnTypeInfoKind::Erroron lookup failurestests/src/type_checker/error_type_poisoning_tests.rs: AI assisted with test structure and test case implementationHuman contributions:
Check List (Check all the applicable boxes)
🚨Please review the contribution guideline for this repository.
Screenshots/Videos