diff --git a/core/translate/expr.rs b/core/translate/expr.rs index a011b68ca..89469f4d1 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -16,7 +16,6 @@ pub struct ConditionMetadata { pub jump_if_condition_is_true: bool, pub jump_target_when_true: BranchOffset, pub jump_target_when_false: BranchOffset, - pub parent_op: Option, } fn emit_cond_jump(program: &mut ProgramBuilder, cond_meta: ConditionMetadata, reg: usize) { @@ -137,87 +136,53 @@ pub fn translate_condition_expr( match expr { ast::Expr::Between { .. } => todo!(), ast::Expr::Binary(lhs, ast::Operator::And, rhs) => { - // In a binary AND, never jump to the 'jump_target_when_true' label on the first condition, because - // the second condition must also be true. - let _ = translate_condition_expr( + // In a binary AND, never jump to the parent 'jump_target_when_true' label on the first condition, because + // the second condition MUST also be true. Instead we instruct the child expression to jump to a local + // true label. + let jump_target_when_true = program.allocate_label(); + translate_condition_expr( program, referenced_tables, lhs, ConditionMetadata { - jump_if_condition_is_true: false, - // Mark that the parent op for sub-expressions is AND - parent_op: Some(ast::Operator::And), + jump_target_when_true, ..condition_metadata }, resolver, - ); - let _ = translate_condition_expr( + )?; + program.resolve_label(jump_target_when_true, program.offset()); + translate_condition_expr( program, referenced_tables, rhs, + condition_metadata, + resolver, + )?; + } + ast::Expr::Binary(lhs, ast::Operator::Or, rhs) => { + // In a binary OR, never jump to the parent 'jump_target_when_false' label on the first condition, because + // the second condition CAN also be true. Instead we instruct the child expression to jump to a local + // false label. + let jump_target_when_false = program.allocate_label(); + translate_condition_expr( + program, + referenced_tables, + lhs, ConditionMetadata { - parent_op: Some(ast::Operator::And), + jump_if_condition_is_true: true, + jump_target_when_false, ..condition_metadata }, resolver, - ); - } - ast::Expr::Binary(lhs, ast::Operator::Or, rhs) => { - if matches!(condition_metadata.parent_op, Some(ast::Operator::And)) { - // we are inside a bigger AND expression, so we do NOT jump to parent's 'true' if LHS or RHS is true. - // we only short-circuit the parent's false label if LHS and RHS are both false. - let local_true_label = program.allocate_label(); - let local_false_label = program.allocate_label(); - - // evaluate LHS in normal OR fashion, short-circuit local if true - let lhs_metadata = ConditionMetadata { - jump_if_condition_is_true: true, - jump_target_when_true: local_true_label, - jump_target_when_false: local_false_label, - parent_op: Some(ast::Operator::Or), - }; - translate_condition_expr(program, referenced_tables, lhs, lhs_metadata, resolver)?; - - // if lhs was false, we land here: - program.resolve_label(local_false_label, program.offset()); - - // evaluate rhs with normal OR: short-circuit if true, go to local_true - let rhs_metadata = ConditionMetadata { - jump_if_condition_is_true: true, - jump_target_when_true: local_true_label, - jump_target_when_false: condition_metadata.jump_target_when_false, - // if rhs is also false => parent's false - parent_op: Some(ast::Operator::Or), - }; - translate_condition_expr(program, referenced_tables, rhs, rhs_metadata, resolver)?; - - // if we get here, both lhs+rhs are false: explicit jump to parent's false - program.emit_insn(Insn::Goto { - target_pc: condition_metadata.jump_target_when_false, - }); - // local_true: we do not jump to parent's "true" label because the parent is AND, - // so we want to keep evaluating the rest - program.resolve_label(local_true_label, program.offset()); - } else { - let jump_target_when_false = program.allocate_label(); - - let lhs_metadata = ConditionMetadata { - jump_if_condition_is_true: true, - jump_target_when_false, - parent_op: Some(ast::Operator::Or), - ..condition_metadata - }; - - translate_condition_expr(program, referenced_tables, lhs, lhs_metadata, resolver)?; - - // if LHS was false, we land here: - program.resolve_label(jump_target_when_false, program.offset()); - let rhs_metadata = ConditionMetadata { - parent_op: Some(ast::Operator::Or), - ..condition_metadata - }; - translate_condition_expr(program, referenced_tables, rhs, rhs_metadata, resolver)?; - } + )?; + program.resolve_label(jump_target_when_false, program.offset()); + translate_condition_expr( + program, + referenced_tables, + rhs, + condition_metadata, + resolver, + )?; } ast::Expr::Binary(lhs, op, rhs) => { let lhs_reg = translate_and_mark(program, Some(referenced_tables), lhs, resolver)?; diff --git a/core/translate/group_by.rs b/core/translate/group_by.rs index 2b6e7afb3..5855caa06 100644 --- a/core/translate/group_by.rs +++ b/core/translate/group_by.rs @@ -386,7 +386,6 @@ pub fn emit_group_by<'a>( jump_if_condition_is_true: false, jump_target_when_false: group_by_end_without_emitting_row_label, jump_target_when_true: BranchOffset::Placeholder, // not used. FIXME: this is a bug. HAVING can have e.g. HAVING a OR b. - parent_op: None, }, &t_ctx.resolver, )?; diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index 33f76a84e..a19fe66f7 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -230,7 +230,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false: next, - parent_op: None, }; translate_condition_expr( program, @@ -279,7 +278,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false, - parent_op: None, }; for predicate in predicates.iter() { translate_condition_expr( @@ -352,7 +350,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false: next, - parent_op: None, }; translate_condition_expr( program, @@ -537,7 +534,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false: next, - parent_op: None, }; translate_condition_expr( program, diff --git a/testing/where.test b/testing/where.test index c613f784b..b3154cd7c 100755 --- a/testing/where.test +++ b/testing/where.test @@ -365,3 +365,21 @@ do_execsql_test nested-parens-conditionals-and-double-or { 8171|Andrea|Lee|dgarrison@example.com|001-594-430-0646|452 Anthony Stravenue|Sandraville|CA|28572|12 9110|Anthony|Barrett|steven05@example.net|(562)928-9177x8454|86166 Foster Inlet Apt. 284|North Jeffreyburgh|CA|80147|97 9279|Annette|Lynn|joanne37@example.com|(272)700-7181|2676 Laura Points Apt. 683|Tristanville|NY|48646|91}} + +# Regression test for nested parens + OR + AND. This returned 0 rows before the fix. +# It should always return 1 row because it is true for id = 6. +do_execsql_test nested-parens-and-inside-or-regression-test { + SELECT count(1) FROM users + WHERE ( + ( + ( + (id != 5) + AND + (id = 5 OR TRUE) + ) + OR FALSE + ) + AND + (id = 6 OR FALSE) + ); +} {1} \ No newline at end of file