Fix short circuit evaluation of AND / OR in the Turbopack analyzer for string & nullish-related methods

[sampoder]

Here is the original description of the bug:

1. shortcircuit_if_known predicates are swapped for Logical(And) and
Logical(Or) (lines 2541–2548, 2573–2582, 2634–2643)

is_nullish, is_string, and is_empty_string all do this for AND/OR:

LogicalOperator::And => shortcircuit_if_known(list, JsValue::is_truthy, …),
LogicalOperator::Or => shortcircuit_if_known(list, JsValue::is_falsy, …),

shortcircuit_if_known returns item_value(item) for the first item where
use_item == Some(true). But:
- a && b returns a when a is falsy, else b → use_item should be is_falsy.
- a || b returns a when a is truthy, else b → use_item should be is_truthy.

Concretely:
- is_nullish(1 && null): code does is_truthy(1) = Some(true) → return
is_nullish(1) = Some(false). JS answer is true. Wrong.
- is_nullish(null || 2): code does is_falsy(null) = Some(true) → return
is_nullish(null) = Some(true). JS answer is false. Wrong.

As mentioned, in the issue is_string, is_empty_string, and is_nullish's short circuit evaluation of && and || was reversed. This PR reverses them so that for AND expressions, the first false-y value is evaluated (otherwise the last value is). For OR expressions, the first truth-y value is evaluated (otherwise the last value is).

I’ve added unit tests to this PR. To verify these unit tests with JavaScript’s behaviour I used Claude Code to generate this node script:

const PREDICATES = {
  is_string: (v) => typeof v === 'string',
  is_empty_string: (v) => v === '',
  is_nullish: (v) => v == null,
  is_not_nullish: (v) => v != null,
}

const cases = [
  {
    test: 'is_string_constant',
    rust: `assert_eq!(value.is_string(), Some(true));`,
    expr: `'hello'`,
    predicate: 'is_string',
    expected: true,
  },

  {
    test: 'is_string_and_short_circuit',
    rust: `assert_eq!(eval("'hello' && 2").is_string(), Some(false));`,
    expr: `'hello' && 2`,
    predicate: 'is_string',
    expected: false,
  },
  {
    test: 'is_string_and_short_circuit',
    rust: `assert_eq!(eval("2 && 1 && 'hello'").is_string(), Some(true));`,
    expr: `2 && 1 && 'hello'`,
    predicate: 'is_string',
    expected: true,
  },

  {
    test: 'is_string_or_short_circuit',
    rust: `assert_eq!(eval("'hello' || 'bye' || 2").is_string(), Some(true));`,
    expr: `'hello' || 'bye' || 2`,
    predicate: 'is_string',
    expected: true,
  },
  {
    test: 'is_string_or_short_circuit',
    rust: `assert_eq!(eval("'hello' || 2 || 1 || 'bye'").is_string(), Some(true));`,
    expr: `'hello' || 2 || 1 || 'bye'`,
    predicate: 'is_string',
    expected: true,
  },
  {
    test: 'is_string_or_short_circuit',
    rust: `assert_eq!(eval("2 || 1 || 'hello' || 'bye'").is_string(), Some(false));`,
    expr: `2 || 1 || 'hello' || 'bye'`,
    predicate: 'is_string',
    expected: false,
  },

  {
    test: 'is_empty_string_and_short_circuit',
    rust: `assert_eq!(eval("'' && 'string'").is_empty_string(), Some(true));`,
    expr: `'' && 'string'`,
    predicate: 'is_empty_string',
    expected: true,
  },
  {
    test: 'is_empty_string_and_short_circuit',
    rust: `assert_eq!(eval("false && ''").is_empty_string(), Some(false));`,
    expr: `false && ''`,
    predicate: 'is_empty_string',
    expected: false,
  },
  {
    test: 'is_empty_string_and_short_circuit',
    rust: `assert_eq!(eval("0 && false && ''").is_empty_string(), Some(false));`,
    expr: `0 && false && ''`,
    predicate: 'is_empty_string',
    expected: false,
  },

  {
    test: 'is_empty_string_or_short_circuit',
    rust: `assert_eq!(eval("'' || 'string'").is_empty_string(), Some(false));`,
    expr: `'' || 'string'`,
    predicate: 'is_empty_string',
    expected: false,
  },
  {
    test: 'is_empty_string_or_short_circuit',
    rust: `assert_eq!(eval("false || ''").is_empty_string(), Some(true));`,
    expr: `false || ''`,
    predicate: 'is_empty_string',
    expected: true,
  },
  {
    test: 'is_empty_string_or_short_circuit',
    rust: `assert_eq!(eval("0 || false || ''").is_empty_string(), Some(true));`,
    expr: `0 || false || ''`,
    predicate: 'is_empty_string',
    expected: true,
  },

  {
    test: 'is_nullish_and_short_circuit',
    rust: `assert_eq!(eval("'' && null").is_nullish(), Some(false));`,
    expr: `'' && null`,
    predicate: 'is_nullish',
    expected: false,
  },
  {
    test: 'is_nullish_and_short_circuit',
    rust: `assert_eq!(eval("null && ''").is_nullish(), Some(true));`,
    expr: `null && ''`,
    predicate: 'is_nullish',
    expected: true,
  },
  {
    test: 'is_nullish_and_short_circuit',
    rust: `assert_eq!(eval("0 && null && ''").is_nullish(), Some(false));`,
    expr: `0 && null && ''`,
    predicate: 'is_nullish',
    expected: false,
  },
  {
    test: 'is_nullish_and_short_circuit',
    rust: `assert_eq!(eval("null && 1 && 2").is_nullish(), Some(true));`,
    expr: `null && 1 && 2`,
    predicate: 'is_nullish',
    expected: true,
  },

  {
    test: 'is_nullish_or_short_circuit',
    rust: `assert_eq!(eval("'' || null").is_nullish(), Some(true));`,
    expr: `'' || null`,
    predicate: 'is_nullish',
    expected: true,
  },
  {
    test: 'is_nullish_or_short_circuit',
    rust: `assert_eq!(eval("null || ''").is_nullish(), Some(false));`,
    expr: `null || ''`,
    predicate: 'is_nullish',
    expected: false,
  },
  {
    test: 'is_nullish_or_short_circuit',
    rust: `assert_eq!(eval("0 || '' || null").is_nullish(), Some(true));`,
    expr: `0 || '' || null`,
    predicate: 'is_nullish',
    expected: true,
  },
  {
    test: 'is_nullish_or_short_circuit',
    rust: `assert_eq!(eval("null || 1 || 2").is_nullish(), Some(false));`,
    expr: `null || 1 || 2`,
    predicate: 'is_nullish',
    expected: false,
  },

  {
    test: 'is_not_nullish_and_short_circuit',
    rust: `assert_eq!(eval("'' && null").is_not_nullish(), Some(true));`,
    expr: `'' && null`,
    predicate: 'is_not_nullish',
    expected: true,
  },
  {
    test: 'is_not_nullish_and_short_circuit',
    rust: `assert_eq!(eval("null && ''").is_not_nullish(), Some(false));`,
    expr: `null && ''`,
    predicate: 'is_not_nullish',
    expected: false,
  },
  {
    test: 'is_not_nullish_and_short_circuit',
    rust: `assert_eq!(eval("0 && null && ''").is_not_nullish(), Some(true));`,
    expr: `0 && null && ''`,
    predicate: 'is_not_nullish',
    expected: true,
  },
  {
    test: 'is_not_nullish_and_short_circuit',
    rust: `assert_eq!(eval("null && 1 && 2").is_not_nullish(), Some(false));`,
    expr: `null && 1 && 2`,
    predicate: 'is_not_nullish',
    expected: false,
  },

  {
    test: 'is_not_nullish_or_short_circuit',
    rust: `assert_eq!(eval("'' || null").is_not_nullish(), Some(false));`,
    expr: `'' || null`,
    predicate: 'is_not_nullish',
    expected: false,
  },
  {
    test: 'is_not_nullish_or_short_circuit',
    rust: `assert_eq!(eval("null || ''").is_not_nullish(), Some(true));`,
    expr: `null || ''`,
    predicate: 'is_not_nullish',
    expected: true,
  },
  {
    test: 'is_not_nullish_or_short_circuit',
    rust: `assert_eq!(eval("0 || '' || null").is_not_nullish(), Some(false));`,
    expr: `0 || '' || null`,
    predicate: 'is_not_nullish',
    expected: false,
  },
  {
    test: 'is_not_nullish_or_short_circuit',
    rust: `assert_eq!(eval("null || 1 || 2").is_not_nullish(), Some(true));`,
    expr: `null || 1 || 2`,
    predicate: 'is_not_nullish',
    expected: true,
  },
]

let passed = 0
let failed = 0
let currentTest = null

for (const c of cases) {
  if (c.test !== currentTest) {
    console.log(`\n#[test] fn ${c.test}()`)
    currentTest = c.test
  }
  console.log(`  ${c.rust}`)

  const result = (0, eval)(c.expr)
  const actual = PREDICATES[c.predicate](result)
  const ok = actual === c.expected
  const status = ok ? 'PASS' : 'FAIL'
  console.log(
    `    -> JS result = ${JSON.stringify(result)}, ${c.predicate} = ${actual}, expected = ${c.expected}  [${status}]`
  )

  if (ok) passed++
  else failed++
}

console.log(`\n${passed} passed, ${failed} failed`)

After running it on my machine:

28 passed, 0 failed

[github-actions]

Tests Passed

Commit: 4dc9ef9

[github-actions]

Stats skipped

Commit: 4dc9ef9
View workflow run

[bgw]

• We use rstest to create parameterized tests in a few places. It would be good to pull that in and use it here. https://docs.rs/rstest/latest/rstest/#creating-parametrized-tests
• Claude loves generating a bazillion unit tests. We probably don't need to be quite this exhaustive. See if it makes sense to trim down a few of the test cases.

[sampoder]

We use rstest to create parameterized tests in a few places. It would be good to pull that in and use it here. https://docs.rs/rstest/latest/rstest/#creating-parametrized-tests

Neat - switched over to these!

Claude loves generating a bazillion unit tests. We probably don't need to be quite this exhaustive. See if it makes sense to trim down a few of the test cases.

Oops, that was just me copy-pasting and making slight changes to each one. I trimmed it down to four per method (one true for AND, one true for OR, one false for AND, and one false for OR).

[lukesandberg]

add some none results for cases we cannot analyze

this should be as simple as referencing unbound variables in the expression x && 2

also i find the true false parameters don't generate good error messages, so it might be better to split this into multiple test functions

fn is_string_short_circuiting_positive(#[case] input: &str) {
   assert_eq!(EvalContext::eval_single_expr_lit(&input.into())
                .unwrap()
                .is_string(), Some(true), "expected '{}' to be a string", input)
}

ditto for negative and unknown

[sampoder]

Oh cool, didn't think of the none case! Added it + split up the tests.

[lukesandberg]

nice!

[lukesandberg]

this looks good, though vade has a solid comment and i think there are a few more cases to handle for unknown

Also add a little bit of coverage for longer examples?