fix(lint/useOptionalChain): incorrect fix for optional chaining #2127
Conversation
github-actions
bot
added
A-Linter
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| // If the previous branch had other expressions that already | ||
| // included the optional chaining operators, we need to | ||
| // include them as well. | ||
| if let Some(mut prev_branch) = prev_branch { |
There was a problem hiding this comment.
This block provides the real fix, together with a similar looking one a little bit below.
I'm not entirely happy with the repetition between these blocks, but I'm also afraid it's going to introduce even more boilerplate and hard-to-follow indirection if I try to share that (a closure won't help here because of borrowing rules).
arendjr
changed the title
CodSpeed Performance ReportMerging #2127 will not alter performanceComparing Summary
|
| /// `LogicalAndChainOrdering` is the result of a comparison between two logical and chain. | ||
| /// `LogicalAndChainOrdering` is the result of a comparison between two logical | ||
| /// AND chains. | ||
| enum LogicalAndChainOrdering { | ||
| /// An ordering where a chain is a sub-chain of another. | ||
| /// | ||
| /// ```js | ||
| /// (foo && foo.bar) /* is sub-chain of */ (foo && foo.bar && foo.bar.baz) | ||
| /// ``` | ||
| /// | ||
| /// Chains can be considered subchains even when they have unequal | ||
| /// optional chaining internally. For instance: | ||
| /// | ||
| /// ```js | ||
| /// (foo?.bar) /* is also a sub-chain of */ (foo?.bar && foo.bar.baz) | ||
| /// ``` | ||
| SubChain, | ||
| /// An ordering where a chain is equal to another. | ||
| /// ```js |
There was a problem hiding this comment.
I may be wrong, but the comments here are confusing. I think the LogicalAndChainOrdering is the result of comparing two dot-notation chains appearing on each side of a && operator, e.g. a.b && a.b.c or a?.b && a.b.c, where a.b or a?.b is considered a subchain of a.b.c. That's also how LogicalAndChain::cmp_chain is implemented.
The names are really confusing though.
There was a problem hiding this comment.
Yes, your understanding is correct. What do feel is the confusing part? The fact it’s called a subchain at all, or the comment I added? I know it took me quite a while to understand as well, so I would be happy if we can improve the wording or naming.
There was a problem hiding this comment.
What do feel is the confusing part?
I think we should change the particular examples here to:
/// An ordering where a chain is a sub-chain of another.
///
/// ```js
-/// (foo && foo.bar) /* is sub-chain of */ (foo && foo.bar && foo.bar.baz)
+/// (foo.bar) /* is a sub-chain of */ (foo.bar.baz)
/// ```
///
/// Chains can be considered subchains even when they have unequal
/// optional chaining internally. For instance:
///
/// ```js
-/// (foo?.bar) /* is also a sub-chain of */ (foo?.bar && foo.bar.baz)
+/// (foo?.bar) /* is also a sub-chain of */ (foo.bar.baz)
/// ```And maybe we should use other terms (instead of "sub-chain") for a && a.b vs a && a.b && a.b.c to discriminate && chains and . chains? 🤔
There was a problem hiding this comment.
Oh yes, good suggestions.
There was a problem hiding this comment.
Not sure about what’s a better word for subchain yet though… Maybe best to leave it for now.
There was a problem hiding this comment.
Yeah naming is hard. I think just updating the examples would be enough for now.
| i Unsafe fix: Change to an optional chain. | ||
|
|
||
| 7 │ foo?.bar·&&·foo.bar?.baz·&&·foo.bar?.qux; | ||
| │ ----------- |
There was a problem hiding this comment.
I've been wondering this since my previous attempt to fix this rule: Why sometimes the suggested fix doesn't provide a diff (like what it does in the following diagnostics)?
There was a problem hiding this comment.
Oh yes, that tripped me in the beginning too. The trick is, when it only removes characters it is indicated by minuses below. Once you know it is a bit clearer, but it is not intuitive at first, I agree.
There was a problem hiding this comment.
Oh wow, thanks for the explanation, this is so much clearer now! I originally misunderstood those minus signs as underlines to indicate which part of the expression should be fixed.🤦
There was a problem hiding this comment.
I know, I had the exact same experience 😅
Summary
This was a pretty rough rule to get into. I even considered whether it wasn't easier to rewrite the logic, but thankfully that wasn't necessary. I do still wonder whether it wouldn’t be easier to just match directly against previous arms of a logical expression, instead of putting everything into queues. The ideal fix for the expression
foo && foo.bar?.baz && foo.bar?.quxisfoo?.bar?.baz && foo.bar.qux. However, I didn’t get further than the same fix offered by ESLint:foo?.bar?.baz && foo.bar?.quxBut with the way the queues and the sorting enum work it isn’t really feasible to match against previous arms that are not strictly decreasing in length. Anyway, that’s a lot of rambling for saying I have a fix, but I didn’t manage to get the ideal solution yet :)
Sorry the diff is a bit larger, since I updated the documentation and some variable names in my process of trying to understand the rule. I'll leave a comment for the part that's really relevant.
Fixes #1925 (cc @Sec-ant)
Test Plan
Added test cases.