💅 incorrect fix for lint/complexity/useOptionalChain rule.

[BatuhanW]

Environment information

> biome rage

CLI:
  Version:                      1.5.3
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v18.19.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.2.5"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    true
  VCS disabled:                 false

Rule name

useOptionalChain

Playground link

https://biomejs.dev/playground/?importSortingEnabled=false&unsafeParameterDecoratorsEnabled=false&allowComments=false&code=YwBvAG4AcwB0ACAAZgBpAGwAdABlAHIARABlAGEAbABCAGUAZgBvAHIAZQAgAD0AIAAoAGQAZQBhAGwAPwA6ACAARABlAGEAbABBAGcAZwByAGUAZwBhAHQAZQApACAAPQA%2BAAoAIAAgAGQAZQBhAGwAPwAuAGcAcgBvAHUAcABCAHkAIAAmACYAIABkAGUAYQBsAC4AZwByAG8AdQBwAEIAeQA%2FAC4AYwBsAG8AcwBlAEQAYQB0AGUATQBvAG4AdABoACAAJgAmACAAZABlAGEAbAAuAGcAcgBvAHUAcABCAHkAPwAuAGMAbABvAHMAZQBEAGEAdABlAFkAZQBhAHIAOwAKAAoALwAvACAAVABoAGUAIABmAGkAeAAgAGkAcwAgAHMAZQBtAGEAbgB0AGkAYwBhAGwAbAB5ACAAYwBvAHIAcgBlAGMAdAAsACAAaABvAHcAZQB2AGUAcgAsACAAVAB5AHAAZQBzAGMAcgBpAHAAdAAgAGMAbwBtAHAAbABhAGkAbgBzACAAYgBlAGMAYQB1AHMAZQAgAGAAZABlAGEAbABgACAAaQBzACAAcABvAHMAcwBpAGIAbAB5ACAAdQBuAGQAZQBmAGkAbgBlAGQALgAKAC8ALwAgACcAZABlAGEAbAAnACAAaQBzACAAcABvAHMAcwBpAGIAbAB5ACAAJwB1AG4AZABlAGYAaQBuAGUAZAAnAC4ACgBjAG8AbgBzAHQAIABmAGkAbAB0AGUAcgBEAGUAYQBsAEEAcABwAGwAaQBlAGQARgBpAHgAIAA9ACAAKABkAGUAYQBsAD8AOgAgAEQAZQBhAGwAQQBnAGcAcgBlAGcAYQB0AGUAKQAgAD0APgAKACAAIABkAGUAYQBsAC4AZwByAG8AdQBwAEIAeQA%2FAC4AYwBsAG8AcwBlAEQAYQB0AGUATQBvAG4AdABoACAAJgAmACAAZABlAGEAbAAuAGcAcgBvAHUAcABCAHkAPwAuAGMAbABvAHMAZQBEAGEAdABlAFkAZQBhAHIAOwAKAAoALwAvACAASQAgAHQAaABpAG4AawAgAGkAdAAgAHMAaABvAHUAbABkACAAYQBkAGQAIABvAHAAdABpAG8AbgBhAGwAIABjAGgAYQBpAG4AIAB0AG8AIABmAGkAcgBzAHQAIABgAGQAZQBhAGwAYAAgAGMAYQBsAGwALgAKAC8ALwAgAEEAbABzAG8ALAAgAGAAPwBgACAAaQBuACAAcwBlAGMAbwBuAGQAIABlAHgAcAByAGUAcwBzAGkAbwBuACAAYABkAGUAYQBsAC4AZwByAG8AdQBwAEIAeQA%2FAC4AYwBsAG8AcwBlAEQAYQB0AGUAWQBlAGEAcgBgACAAaQBzACAAcgBlAGQAdQBuAGQAYQBuAHQALgAKAC8ALwAgAGkAZgAgAHcAZQAgAGgAYQB2AGUAIAB0AHIAdQB0AGgAeQAgAHYAYQBsAHUAZQAgAGYAbwByACAAYABkAGUAYQBsAD8ALgBnAHIAbwB1AHAAQgB5AD8ALgBjAGwAbwBzAGUARABhAHQAZQBNAG8AbgB0AGgAYAAKAC8ALwAgAFQAaABpAHMAIABtAGUAYQBuAHMAIABgAGQAZQBhAGwALgBnAHIAbwB1AHAAQgB5AGAAIABpAHMAIABuAG8AdAAgAG4AdQBsAGwALgAKAC8ALwAgAFMAbwAgAGkAdAAgAGMAYQBuACAAYgBlACAAYABkAGUAYQBsAC4AZwByAG8AdQBwAEIAeQAuAGMAbABvAHMAZQBEAGEAdABlAFkAZQBhAHIAYAAuAAoAYwBvAG4AcwB0ACAAZgBpAGwAdABlAHIARABlAGEAbABFAHgAcABlAGMAdABlAGQARgBpAHgAIAA9ACAAKABkAGUAYQBsAD8AOgAgAEQAZQBhAGwAQQBnAGcAcgBlAGcAYQB0AGUAKQAgAD0APgAKACAAIABkAGUAYQBsAD8ALgBnAHIAbwB1AHAAQgB5AD8ALgBjAGwAbwBzAGUARABhAHQAZQBNAG8AbgB0AGgAIAAmACYAIABkAGUAYQBsAC4AZwByAG8AdQBwAEIAeQAuAGMAbABvAHMAZQBEAGEAdABlAFkAZQBhAHIAOwA%3D&jsx=false

Expected result

const filterDealExpectedFix = (deal?: DealAggregate) =>
  deal?.groupBy?.closeDateMonth && deal.groupBy.closeDateYear;

I think it should add optional chain to first deal call.
Also, ? in second expression deal.groupBy?.closeDateYear is redundant.
if we have truthy value for deal?.groupBy?.closeDateMonth
This means deal.groupBy is not null.
So it can be deal.groupBy.closeDateYear.

Code of Conduct

• I agree to follow Biome's Code of Conduct

[Sec-ant]

I took some time to look into this problem. And I want to share some of my thoughts... and maybe seek for suggestions and help.

---

The original expression is:

deal?.groupBy && deal.groupBy?.closeDateMonth && deal.groupBy?.closeDateYear;

I simplified it as:

foo?.bar && foo.bar?.baz && foo.bar?.qux;

The expected fix suggested (and also well-explained) by OP is:

foo?.bar?.baz && foo.bar.qux;

However, biome (v1.6.1) suggests this fix:

foo.bar?.baz && foo.bar?.qux;

I also copy-paste it into the typescript-eslint playground and it suggests this fix:

foo?.bar?.baz && foo.bar?.qux;

We can see that neither biome nor eslint can give us the ideal fix. But the suggested fix from eslint will not cause type issues. Because foo can be nullish.

---

That's when I think maybe we should split this issue into two small issues, and we should first make biome suggest the same fix as what eslint does. To focus on this small issue. I further simplified the test case into:

foo?.bar && foo.bar.baz

The correct fix should be foo?.bar?.baz and this is exactly what eslint suggests. And as expected, biome seems to ignore the fact that foo can be nullish and wrongly suggests this fix:

foo.bar?.baz

So I dug up a bit into the implementation details of this rule in the biome's source code. At first glance, the code really looks overwhelming to me. But it's actually very well organized and documented in every detail by @denbezrukov, which I appreciate and admire very much!

After some test and debugging, I think the root cause is this piece of code (you can click the image to jump to its place):

Here in our case, self corresponds to foo.bar.baz and branch corresponds to foo?.bar. We can see that after the cmp_chain returns a good result (self ⊇ branch), we only use the part from the self chain for code transformation, so the optional operator .? in the branch chain is not preserved in the final transformed tree.

To fix this issue, my naive thought is that we should always use the branch part (left expression of &&) in code transformation. To achieve this, my next naive thought is to replace the subnode of part with the head of branch. So I wrote sth like this:

match self.cmp_chain(&branch).ok()? {
    LogicalAndChainOrdering::SubChain => {
        // Here we reduce our main `JsAnyExpression` buffer by splitting the main buffer.
        // Let's say that we have two buffers:
        // The main is `[foo, bar, baz]` and a branch is `[foo]`
        // After splitting the main buffer will be `[foo]` and the tail will be `[bar, baz]`.
        // It means that we need to transform `bar` (first tail expression) into the optional one.
        let mut tail = self.buf.split_off(branch.buf.len());
        if let Some(part) = tail.pop_front() {

            // vvvvvvvvvvvvvvvvvvvv added vvvvvvvvvvvvvvvvvvvv 

            let part = match part {
                AnyJsExpression::JsStaticMemberExpression(ref main) => {
                    match (main.object().ok()?, branch.head) {
                        (
                            AnyJsExpression::JsStaticMemberExpression(m),
                            AnyJsExpression::JsStaticMemberExpression(b),
                        ) => part.replace_node(m, b)?,
                        (
                            AnyJsExpression::JsComputedMemberExpression(m),
                            AnyJsExpression::JsComputedMemberExpression(b),
                        ) => part.replace_node(m, b)?,
                        (
                            AnyJsExpression::JsCallExpression(m),
                            AnyJsExpression::JsCallExpression(b),
                        ) => part.replace_node(m, b)?,
                        _ => part,
                    }
                }
                AnyJsExpression::JsComputedMemberExpression(ref main) => {
                    match (main.object().ok()?, branch.head) {
                        (
                            AnyJsExpression::JsStaticMemberExpression(m),
                            AnyJsExpression::JsStaticMemberExpression(b),
                        ) => part.replace_node(m, b)?,
                        (
                            AnyJsExpression::JsComputedMemberExpression(m),
                            AnyJsExpression::JsComputedMemberExpression(b),
                        ) => part.replace_node(m, b)?,
                        (
                            AnyJsExpression::JsCallExpression(m),
                            AnyJsExpression::JsCallExpression(b),
                        ) => part.replace_node(m, b)?,
                        _ => part,
                    }
                }
                AnyJsExpression::JsCallExpression(ref main) => {
                    match (main.callee().ok()?, branch.head) {
                        (
                            AnyJsExpression::JsStaticMemberExpression(m),
                            AnyJsExpression::JsStaticMemberExpression(b),
                        ) => part.replace_node(m, b)?,
                        (
                            AnyJsExpression::JsComputedMemberExpression(m),
                            AnyJsExpression::JsComputedMemberExpression(b),
                        ) => part.replace_node(m, b)?,
                        (
                            AnyJsExpression::JsCallExpression(m),
                            AnyJsExpression::JsCallExpression(b),
                        ) => part.replace_node(m, b)?,
                        _ => part,
                    }
                }
                _ => part,
            };

            // ^^^^^^^^^^^^^^^^^^^^ added ^^^^^^^^^^^^^^^^^^^^

            optional_chain_expression_nodes.push_front(part)
        };
    }
    LogicalAndChainOrdering::Equal => continue,
    LogicalAndChainOrdering::Different => return None,
}

To my surprise this actually works for our simple test case foo?.bar && foo.bar.baz (it suggests foo?.bar?.baz), but it fails for some other more complicated cases like:

foo && foo?.bar && foo?.bar.baz && foo?.bar.baz[buzz] && foo?.bar.baz[buzz]()

The suggestion it gives is (the correct one should be foo?.bar?.baz?.[buzz]?.())

foo?.bar.baz[buzz]?.()

I also noticed that after calling replace_node, all the range information will get lost. I think this might cause some problems when we're trying to transform chains level by level?

I'm currently stuck on this and would be really appreciate if someone who're more familiar with the codebase can shed some light on this problem. 🙏

@ematipico @Conaclos @denbezrukov (please excuse me for the ping 😝)

[arendjr]

I'm not too familiar with this rule yet, but I'm trying to look into it. I can see that your complex test (foo && foo?.bar && foo?.bar.baz && foo?.bar.baz[buzz] && foo?.bar.baz[buzz]()) is currently passing, so it appears your initial fix is introducing a regression. I'm also still trying to understand the details of what's going, but my first hunch is that it's going wrong because you're replacing nodes in a function that's not yet expected to do so. I think indeed replace_node() should only be done in the action so the code that you modified shouldn't yet do any actual replacement, instead it should transform the suggested replacement expression.

I'll keep looking a bit more to see if this leads me to a solution.

[Sec-ant]

Thanks for looking into this! @arendjr

so it appears your initial fix is introducing a regression.

Yes, I should've mentioned that this is an existing test case. It's indeed a regression.

so the code that you modified shouldn't yet do any actual replacement, instead it should transform the suggested replacement expression.

You're right! I think maybe we should reconsider the structure of the signals that are transferred from the run part to the action part to preserve the lost information from the left part nodes.

To justify that we should always use the left part of && when applying transformation:

a?.b && a.b.c // should be fixed as: `a?.b?.c`, use the left part `a?.b`

a.b && a?.b.c // should be fixed as: `a.b?.c`, use the left part `a.b`
// the dot operator in the left part should be considered safe because the user doesn't use a `.?`