Skip to content

Commit d34ffc5

Browse files
committed
feat(analyzer): rewrite match expression analyzer for correctness
This commit introduces a complete rewrite of the `match` expression analyzer to resolve numerous bugs and correctness issues in the previous implementation. The old analyzer struggled to correctly model the control flow and type narrowing within a `match` block, leading to false positives and missed errors related to unreachable arms and exhaustiveness. The new implementation correctly simulates `match` semantics by sequentially analyzing each arm, applying its conditions as assertions to narrow the subject's type, and carrying the remaining (unhandled) subject type forward to subsequent arms. Key capabilities of the new analyzer include: - **Accurate Exhaustiveness Checking**: Correctly determines if all possible types of the subject are handled and reports `MatchNotExhaustive` if not. - **Precise Unreachable Code Detection**: Identifies unreachable arms that are shadowed by previous arms or whose conditions can never be met. - **Redundant Condition Analysis**: Finds arms that are *always* true given the remaining subject type, making subsequent arms unreachable. - **Correct Type Inference**: Reliably infers the final return type by combining the types of all reachable arms. This rewrite provides a robust and correct foundation for analyzing `match` expressions, leading to far more accurate and useful diagnostics. closes #260 Signed-off-by: azjezz <[email protected]>
1 parent 1ba43ac commit d34ffc5

File tree

13 files changed

+914
-1145
lines changed

13 files changed

+914
-1145
lines changed

crates/analyzer/src/assertion.rs

Lines changed: 77 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,20 @@ pub fn scrape_assertions(
4444
}
4545

4646
match unwrap_expression(expression) {
47+
Expression::UnaryPrefix(UnaryPrefix { operator: UnaryPrefixOperator::Not(_), operand }) => {
48+
let assertions = scrape_assertions(operand, artifacts, assertion_context);
49+
let mut negated_assertions = HashMap::default();
50+
for assertion in assertions {
51+
for (var_name, assertion_set) in assertion {
52+
negated_assertions
53+
.entry(var_name)
54+
.or_insert_with(Vec::new)
55+
.extend(negate_assertion_set(assertion_set));
56+
}
57+
}
58+
59+
return if negated_assertions.is_empty() { vec![] } else { vec![negated_assertions] };
60+
}
4761
Expression::Call(call) => {
4862
// Collect `@assert` assertions.
4963
if_types.extend(process_custom_assertions(call.span(), artifacts));
@@ -331,15 +345,15 @@ pub(super) fn scrape_equality_assertions(
331345
}
332346
};
333347

334-
if let Some(null_position) = has_null_variable(left, right) {
348+
if let Some(null_position) = has_null_variable(left, right, artifacts) {
335349
return get_null_equality_assertions(left, right, assertion_context, null_position);
336350
}
337351

338-
if let Some(true_position) = has_true_variable(left, right) {
339-
return get_true_equality_assertions(left, is_identity, right, assertion_context, true_position);
352+
if let Some(true_position) = has_true_variable(left, right, artifacts) {
353+
return get_true_equality_assertions(left, is_identity, right, artifacts, assertion_context, true_position);
340354
}
341355

342-
if let Some(false_position) = has_false_variable(left, right) {
356+
if let Some(false_position) = has_false_variable(left, right, artifacts) {
343357
return get_false_equality_assertions(left, is_identity, right, assertion_context, false_position);
344358
}
345359

@@ -410,15 +424,15 @@ fn scrape_inequality_assertions(
410424
}
411425
};
412426

413-
if let Some(null_position) = has_null_variable(left, right) {
427+
if let Some(null_position) = has_null_variable(left, right, artifacts) {
414428
return get_null_inequality_assertions(left, right, assertion_context, null_position);
415429
}
416430

417-
if let Some(false_position) = has_false_variable(left, right) {
431+
if let Some(false_position) = has_false_variable(left, right, artifacts) {
418432
return get_false_inquality_assertions(left, right, assertion_context, false_position);
419433
}
420434

421-
if let Some(true_position) = has_true_variable(left, right) {
435+
if let Some(true_position) = has_true_variable(left, right, artifacts) {
422436
return get_true_inquality_assertions(left, right, assertion_context, true_position);
423437
}
424438

@@ -1142,6 +1156,7 @@ fn get_true_equality_assertions(
11421156
left: &Expression,
11431157
is_identity: bool,
11441158
right: &Expression,
1159+
artifacts: &mut AnalysisArtifacts,
11451160
assertion_context: AssertionContext<'_>,
11461161
true_position: OtherValuePosition,
11471162
) -> Vec<HashMap<String, AssertionSet>> {
@@ -1166,10 +1181,11 @@ fn get_true_equality_assertions(
11661181
if_types.insert(var_name, vec![vec![Assertion::Truthy]]);
11671182
}
11681183

1169-
return vec![if_types];
1184+
vec![if_types]
1185+
} else {
1186+
// If we can't get an expression ID, we can still assert that the expression is truthy.
1187+
scrape_assertions(base_conditional, artifacts, assertion_context)
11701188
}
1171-
1172-
vec![]
11731189
}
11741190

11751191
pub fn has_typed_value_comparison(
@@ -1473,41 +1489,89 @@ pub fn has_enum_case_comparison(
14731489
}
14741490

14751491
#[inline]
1476-
pub const fn has_null_variable(left: &Expression, right: &Expression) -> Option<OtherValuePosition> {
1492+
pub fn has_null_variable(
1493+
left: &Expression,
1494+
right: &Expression,
1495+
artifacts: &AnalysisArtifacts,
1496+
) -> Option<OtherValuePosition> {
14771497
if let Expression::Literal(Literal::Null(_)) = unwrap_expression(right) {
14781498
return Some(OtherValuePosition::Right);
14791499
}
14801500

1501+
if let Some(right_type) = artifacts.get_expression_type(right)
1502+
&& right_type.is_null()
1503+
{
1504+
return Some(OtherValuePosition::Right);
1505+
}
1506+
14811507
if let Expression::Literal(Literal::Null(_)) = unwrap_expression(left) {
14821508
return Some(OtherValuePosition::Left);
14831509
}
14841510

1511+
if let Some(left_type) = artifacts.get_expression_type(left)
1512+
&& left_type.is_null()
1513+
{
1514+
return Some(OtherValuePosition::Left);
1515+
}
1516+
14851517
None
14861518
}
14871519

14881520
#[inline]
1489-
pub const fn has_false_variable(left: &Expression, right: &Expression) -> Option<OtherValuePosition> {
1521+
pub fn has_false_variable(
1522+
left: &Expression,
1523+
right: &Expression,
1524+
artifacts: &AnalysisArtifacts,
1525+
) -> Option<OtherValuePosition> {
14901526
if let Expression::Literal(Literal::False(_)) = unwrap_expression(right) {
14911527
return Some(OtherValuePosition::Right);
14921528
}
14931529

1530+
if let Some(right_type) = artifacts.get_expression_type(right)
1531+
&& right_type.is_false()
1532+
{
1533+
return Some(OtherValuePosition::Right);
1534+
}
1535+
14941536
if let Expression::Literal(Literal::False(_)) = unwrap_expression(left) {
14951537
return Some(OtherValuePosition::Left);
14961538
}
14971539

1540+
if let Some(left_type) = artifacts.get_expression_type(left)
1541+
&& left_type.is_false()
1542+
{
1543+
return Some(OtherValuePosition::Left);
1544+
}
1545+
14981546
None
14991547
}
15001548

15011549
#[inline]
1502-
pub const fn has_true_variable(left: &Expression, right: &Expression) -> Option<OtherValuePosition> {
1550+
pub fn has_true_variable(
1551+
left: &Expression,
1552+
right: &Expression,
1553+
artifacts: &AnalysisArtifacts,
1554+
) -> Option<OtherValuePosition> {
15031555
if let Expression::Literal(Literal::True(_)) = unwrap_expression(right) {
15041556
return Some(OtherValuePosition::Right);
15051557
}
15061558

1559+
if let Some(right_type) = artifacts.get_expression_type(right)
1560+
&& right_type.is_true()
1561+
{
1562+
return Some(OtherValuePosition::Right);
1563+
}
1564+
15071565
if let Expression::Literal(Literal::True(_)) = unwrap_expression(left) {
15081566
return Some(OtherValuePosition::Left);
15091567
}
15101568

1569+
if let Some(left_type) = artifacts.get_expression_type(left)
1570+
&& left_type.is_true()
1571+
{
1572+
return Some(OtherValuePosition::Left);
1573+
}
1574+
15111575
None
15121576
}
15131577

crates/analyzer/src/code.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,6 @@ impl Code {
114114
pub const MATCH_EXPRESSION_ONLY_DEFAULT_ARM: &'static str = "match-expression-only-default-arm";
115115
pub const EMPTY_MATCH_EXPRESSION: &'static str = "empty-match-expression";
116116
pub const UNKNOWN_MATCH_SUBJECT_TYPE: &'static str = "unknown-match-subject-type";
117-
pub const UNKNOWN_MATCH_CONDITION_TYPE: &'static str = "unknown-match-condition-type";
118-
pub const UNREACHABLE_MATCH_ARM_CONDITION: &'static str = "unreachable-match-arm-condition";
119117
pub const UNREACHABLE_MATCH_ARM: &'static str = "unreachable-match-arm";
120118
pub const UNREACHABLE_MATCH_DEFAULT_ARM: &'static str = "unreachable-match-default-arm";
121119
pub const MATCH_ARM_ALWAYS_TRUE: &'static str = "match-arm-always-true";

crates/analyzer/src/common/synthetic.rs

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,17 @@ pub fn new_synthetic_call(interner: &ThreadedInterner, f: &str, expression: Expr
1010
Expression::Call(Call::Function(FunctionCall {
1111
function: Box::new(Expression::Literal(Literal::String(LiteralString {
1212
kind: Some(LiteralStringKind::SingleQuoted),
13-
span: Span::dummy(0, 1),
13+
span: Span::zero(),
1414
raw: str_id,
1515
value: Some(f.to_string()),
1616
}))),
1717
argument_list: ArgumentList {
18-
left_parenthesis: Span::dummy(0, 1),
18+
left_parenthesis: Span::zero(),
1919
arguments: TokenSeparatedSequence::new(
2020
vec![Argument::Positional(PositionalArgument { ellipsis: None, value: expression })],
2121
vec![],
2222
),
23-
right_parenthesis: Span::dummy(0, 1),
23+
right_parenthesis: Span::zero(),
2424
},
2525
}))
2626
}
@@ -38,6 +38,19 @@ pub fn new_synthetic_disjunctive_equality(
3838
expr
3939
}
4040

41+
pub fn new_synthetic_disjunctive_identity(
42+
subject: &Expression,
43+
left: &Expression,
44+
right: Vec<&Expression>,
45+
) -> Expression {
46+
let mut expr = new_synthetic_identical(subject, left);
47+
for r in right {
48+
expr = new_synthetic_or(&expr, &new_synthetic_identical(subject, r));
49+
}
50+
51+
expr
52+
}
53+
4154
pub fn new_synthetic_negation(expression: &Expression) -> Expression {
4255
if let Expression::Binary(Binary { lhs, operator: BinaryOperator::And(_), rhs }) = expression {
4356
return new_synthetic_or(&new_synthetic_negation(lhs), &new_synthetic_negation(rhs));
@@ -49,16 +62,20 @@ pub fn new_synthetic_negation(expression: &Expression) -> Expression {
4962
})
5063
}
5164

52-
pub fn new_synthetic_variable(interner: &ThreadedInterner, name: &str) -> Expression {
53-
Expression::Variable(Variable::Direct(DirectVariable { span: Span::dummy(0, 1), name: interner.intern(name) }))
65+
pub fn new_synthetic_variable(interner: &ThreadedInterner, name: &str, span: Span) -> Expression {
66+
Expression::Variable(Variable::Direct(DirectVariable { span, name: interner.intern(name) }))
67+
}
68+
69+
pub fn new_synthetic_identical(left: &Expression, right: &Expression) -> Expression {
70+
new_synthetic_binary(left, BinaryOperator::Identical(Span::zero()), right)
5471
}
5572

5673
pub fn new_synthetic_equals(left: &Expression, right: &Expression) -> Expression {
57-
new_synthetic_binary(left, BinaryOperator::Equal(Span::dummy(0, 1)), right)
74+
new_synthetic_binary(left, BinaryOperator::Equal(Span::zero()), right)
5875
}
5976

6077
pub fn new_synthetic_or(left: &Expression, right: &Expression) -> Expression {
61-
new_synthetic_binary(left, BinaryOperator::Or(Span::dummy(0, 1)), right)
78+
new_synthetic_binary(left, BinaryOperator::Or(Span::zero()), right)
6279
}
6380

6481
pub fn new_synthetic_binary(left: &Expression, operator: BinaryOperator, right: &Expression) -> Expression {

crates/analyzer/src/context/block.rs

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,14 @@ use mago_algebra::clause::Clause;
99
use mago_codex::assertion::Assertion;
1010
use mago_codex::context::ScopeContext;
1111
use mago_codex::metadata::CodebaseMetadata;
12+
use mago_codex::ttype::TType;
1213
use mago_codex::ttype::add_optional_union_type;
1314
use mago_codex::ttype::atomic::TAtomic;
1415
use mago_codex::ttype::atomic::scalar::TScalar;
16+
use mago_codex::ttype::comparator::ComparisonResult;
17+
use mago_codex::ttype::comparator::union_comparator;
1518
use mago_codex::ttype::get_mixed;
19+
use mago_codex::ttype::get_never;
1620
use mago_codex::ttype::union::TUnion;
1721
use mago_collector::Collector;
1822
use mago_interner::StringIdentifier;
@@ -24,9 +28,9 @@ use crate::context::Context;
2428
use crate::context::scope::control_action::ControlAction;
2529
use crate::context::scope::finally_scope::FinallyScope;
2630
use crate::context::scope::var_has_root;
27-
use crate::expression::r#match::subtract_union_types;
2831
use crate::reconciler::ReconciliationContext;
2932
use crate::reconciler::assertion_reconciler;
33+
use crate::reconciler::negated_assertion_reconciler;
3034

3135
#[derive(Clone, PartialEq, Eq, Debug)]
3236
pub enum BreakContext {
@@ -666,6 +670,69 @@ fn substitute_types(
666670
add_optional_union_type(updated_type, new_type, context.codebase, context.interner)
667671
}
668672

673+
/// Subtracts the types in `type_to_remove` from `existing_type`.
674+
///
675+
/// This function iterates through each atomic type in `existing_type`. For each of these,
676+
/// it iteratively applies the logic of "is not `atomic_from_remove_set`" for every
677+
/// atomic type in `type_to_remove`. This effectively refines each part of `existing_type`
678+
/// to exclude any possibilities covered by `type_to_remove`.
679+
///
680+
/// This is primarily useful for determining remaining possible types for a match subject
681+
/// after some conditional arms have been considered.
682+
///
683+
/// # Arguments
684+
///
685+
/// * `context` - The reconciliation context, providing access to codebase and interner.
686+
/// * `existing_type` - The initial `TUnion` type (the minuend).
687+
/// * `type_to_remove` - The `TUnion` type whose components should be subtracted from `existing_type`.
688+
///
689+
/// # Returns
690+
///
691+
/// A new `TUnion` representing `existing_type - type_to_remove`.
692+
pub fn subtract_union_types(context: &mut Context<'_>, existing_type: TUnion, type_to_remove: TUnion) -> TUnion {
693+
if existing_type == type_to_remove {
694+
return get_never();
695+
}
696+
697+
if !(existing_type.has_literal_value() && type_to_remove.has_literal_value())
698+
&& union_comparator::is_contained_by(
699+
context.codebase,
700+
context.interner,
701+
&existing_type,
702+
&type_to_remove,
703+
false,
704+
false,
705+
true,
706+
&mut ComparisonResult::new(),
707+
)
708+
{
709+
return existing_type;
710+
}
711+
712+
let mut reconciliation_context = context.get_reconciliation_context();
713+
let mut result = existing_type;
714+
for atomic in type_to_remove.types {
715+
let assertion = Assertion::IsNotType(atomic);
716+
let key = result.get_id(Some(reconciliation_context.interner));
717+
result = negated_assertion_reconciler::reconcile(
718+
&mut reconciliation_context,
719+
&assertion,
720+
&result,
721+
false,
722+
None,
723+
key,
724+
None,
725+
true,
726+
);
727+
728+
if result.is_never() {
729+
break;
730+
}
731+
}
732+
733+
result
734+
}
735+
669736
fn should_keep_clause(clause: &Rc<Clause>, remove_var_id: &str, new_type: Option<&TUnion>) -> bool {
670737
if let Some(possibilities) = clause.possibilities.get(remove_var_id) {
671738
if possibilities.len() == 1

crates/analyzer/src/expression/binary/comparison.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,12 @@ fn report_redundant_comparison(
348348
comparison_description: &str,
349349
result_value_str: &str,
350350
) {
351+
let operator_span = binary.operator.span();
352+
if operator_span.is_zero() {
353+
// this is a synthetic node, do not report it.
354+
return;
355+
}
356+
351357
context.collector.report_with_code(
352358
Code::REDUNDANT_COMPARISON,
353359
Issue::help(format!(

crates/analyzer/src/expression/binary/logical.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ pub fn analyze_logical_and_operation<'a>(
112112
&mut changed_var_ids,
113113
&left_referenced_var_ids,
114114
&binary.rhs.span(),
115-
true,
115+
!binary.operator.span().is_zero(),
116116
!block_context.inside_negation,
117117
);
118118
} else {
@@ -473,7 +473,7 @@ pub fn analyze_logical_or_operation<'a>(
473473
&mut right_changed_var_ids,
474474
&right_referenced_var_ids,
475475
&binary.rhs.span(),
476-
true,
476+
!binary.operator.span().is_zero(),
477477
block_context.inside_negation,
478478
);
479479
}
@@ -642,6 +642,12 @@ fn report_redundant_logical_operation(
642642
rhs_description: &str,
643643
result_value_str: &str,
644644
) {
645+
let operator_span = binary.operator.span();
646+
if operator_span.is_zero() {
647+
// Do not report issues for synthetic nodes.
648+
return;
649+
}
650+
645651
context.collector.report_with_code(
646652
Code::REDUNDANT_LOGICAL_OPERATION,
647653
Issue::help(format!(

0 commit comments

Comments
 (0)