useOptionalChain

[ematipico]

No description provided.

[denbezrukov]

Would you mind assigning me this task?🙏🏽

[ematipico]

@denbezrukov Done!

[denbezrukov]

There is a conditional which is used to check isLeftSideLowerPrecedence.

• https://github.com/typescript-eslint/typescript-eslint/blob/943523d9c345f01a7e285eb4440be2998614186d/packages/eslint-plugin/src/rules/prefer-optional-chain.ts#L73-L83

We need it for the case then it's necessary to add parentheses around left part of LogicalExpression after fixing the rule:

(await a || {}).b -> (await a)?.b

I found OperatorPrecedence enum.

tools/crates/rome_js_parser/src/syntax/util.rs

Lines 8 to 30 in 282bda4

	pub(crate) enum OperatorPrecedence {
	Comma = 0,
	Yield = 1,
	Assignment = 2,
	Conditional = 3,
	Coalesce = 4,
	LogicalOr = 5,
	LogicalAnd = 6,
	BitwiseOr = 7,
	BitwiseXor = 8,
	BitwiseAnd = 9,
	Equality = 10,
	Relational = 11,
	Shift = 12,
	Additive = 13,
	Multiplicative = 14,
	Exponential = 15,
	Unary = 16,
	Update = 17,
	LeftHandSide = 18,
	Member = 19,
	Primary = 20,
	}

I was wondering if I could make it public and port getOperatorPrecedence function from typescript-eslint:

• https://github.com/typescript-eslint/typescript-eslint/blob/943523d9c345f01a7e285eb4440be2998614186d/packages/eslint-plugin/src/util/getOperatorPrecedence.ts#L195

What do you think?🙏🏽

[ematipico]

I was wondering if I could make it public and port getOperatorPrecedence function from typescript-eslint:

@denbzrukov yes, it makes total sense! Ideally, the precedence should be built using Rust's trait system (Ord and PartialOrd).

Inside the formatter we created an enum called FormatPrecedence, which implements methods where the precedence is created based on the context. For your solution, it seems we want to create a new method for the optional chain operator.

What we could do, is to move FormatPrecedence inside the crate rome_js_syntax, in a file called utils.rs, and rename it NodePrecedence. And then make it available to the formatter AND the analyzer. At that point, we can create a new method called with_precedence_for_optional_operator. That should work?

Let me know what you think.

[denbezrukov]

@ematipico
Could you please explain how FormatPrecedence::with_precedence_for_parenthesis works now? Does It return FormatPrecedence::High for expressions which have more precedence over parentheses? And FormatPrecedence::Low for expressions which have less precedence over parentheses?

I guess that with_precedence_for_optional_operator should work. I see only one drawback that next time when we need to implement, for example, with_precedence_for_async_await we'll again struggle with Operator Precedence Table.

It seems that getOperatorPrecedence function from typescript-eslint can handle all cases. Because it returns OperatorPrecedence which already derives Ord and PartialOrd.

tools/crates/rome_js_parser/src/syntax/util.rs

Lines 7 to 8 in 282bda4

	#[derive(Debug, Eq, Ord, PartialOrd, PartialEq, Copy, Clone)]
	pub(crate) enum OperatorPrecedence {

We can implement this function and use it in with_precedence_for_optional_operator and with_precedence_for_parenthesis.

E.g. with_precedence_for_optional_operator:

1. pass JsSyntaxNode as argument.
2. get_operator_precedence returns OperatorPrecedence for the node.
3. compare OperatorPrecedence with OperatorPrecedence::LeftHandSide. Because optional chain has that precedence.

[ematipico]

Could you please explain how FormatPrecedence::with_precedence_for_parenthesis works now? Does It return FormatPrecedence::High for expressions which have more precedence over parentheses? And FormatPrecedence::Low for expressions which have less precedence over parentheses?

Yes, but there's also the fact that this precedence has a context.

The idea is that you have two nodes, the current node and its parent, then you pass kind of the nodes to the function. The function will emit a precedence priority. Then you use these two priorities for doing different things. If the parent has more priority compared to the child, we can't remove the parenthesis.

An example:

a[(a + b) * 3]

Here the the parent of the JsParenthesizedExpression is a JsBinaryExpression. We assign a high priority to JsBinaryExpression because we can't remove the parenthesis: doing so will cause a different in the logic, breaking the code.

Instead:

const a = (3 + 4);

Here it's safe to remove the parenthesis. The parent here is a JsInitializerClause, where we assign Precedence::None, which is the lowest. Then, the parenthesis are removed.

[denbezrukov]

@ematipico
Thank you for your explanation!
I have concerns that there are cases then we can omit parenthesis for your first example but current implementation keeps them.
E.g.
a[(a * b) * 3] -> a[a * b * 3]
Prettier actually does it. Playground

What do you think about:
This function is port of typescript-eslint function

• https://github.com/typescript-eslint/typescript-eslint/blob/943523d9c345f01a7e285eb4440be2998614186d/packages/eslint-plugin/src/util/getOperatorPrecedence.ts#L195-L298

#[derive(Debug, Eq, Ord, PartialOrd, PartialEq, Copy, Clone)]
pub enum OperatorPrecedence {
    Comma = 0,
    Yield = 1,
    Assignment = 2,
    Conditional = 3,
    Coalesce = 4,
    LogicalOr = 5,
    LogicalAnd = 6,
    BitwiseOr = 7,
    BitwiseXor = 8,
    BitwiseAnd = 9,
    Equality = 10,
    Relational = 11,
    Shift = 12,
    Additive = 13,
    Multiplicative = 14,
    Exponential = 15,
    Unary = 16,
    Update = 17,
    LeftHandSide = 18,
    Member = 19,
    Primary = 20,
    Group = 21,
}

pub fn expression_precedence(expression: &JsAnyExpression) -> SyntaxResult<OperatorPrecedence> {
    let precedence = match expression {
        JsAnyExpression::JsSequenceExpression(_) => OperatorPrecedence::Comma,
        JsAnyExpression::JsYieldExpression(_) => OperatorPrecedence::Yield,
        JsAnyExpression::JsConditionalExpression(_) => OperatorPrecedence::Conditional,
        JsAnyExpression::JsAssignmentExpression(_) => OperatorPrecedence::Assignment,
        JsAnyExpression::JsInExpression(_)
        | JsAnyExpression::JsInstanceofExpression(_)
        | JsAnyExpression::TsAsExpression(_) => OperatorPrecedence::Relational,
        JsAnyExpression::JsLogicalExpression(expression) => match expression.operator()? {
            JsLogicalOperator::LogicalAnd => OperatorPrecedence::LogicalAnd,
            JsLogicalOperator::LogicalOr => OperatorPrecedence::LogicalOr,
            JsLogicalOperator::NullishCoalescing => OperatorPrecedence::Coalesce,
        },
        JsAnyExpression::JsBinaryExpression(expression) => match expression.operator()? {
            JsBinaryOperator::LessThan
            | JsBinaryOperator::GreaterThan
            | JsBinaryOperator::GreaterThanOrEqual
            | JsBinaryOperator::LessThanOrEqual => OperatorPrecedence::Relational,
            JsBinaryOperator::Equality
            | JsBinaryOperator::StrictEquality
            | JsBinaryOperator::Inequality
            | JsBinaryOperator::StrictInequality => OperatorPrecedence::Equality,
            JsBinaryOperator::Minus | JsBinaryOperator::Plus => OperatorPrecedence::Additive,
            JsBinaryOperator::Remainder | JsBinaryOperator::Divide | JsBinaryOperator::Times => {
                OperatorPrecedence::Multiplicative
            }
            JsBinaryOperator::Exponent => OperatorPrecedence::Exponential,
            JsBinaryOperator::UnsignedRightShift
            | JsBinaryOperator::RightShift
            | JsBinaryOperator::LeftShift => OperatorPrecedence::Shift,
            JsBinaryOperator::BitwiseAnd => OperatorPrecedence::BitwiseAnd,
            JsBinaryOperator::BitwiseOr => OperatorPrecedence::BitwiseOr,
            JsBinaryOperator::BitwiseXor => OperatorPrecedence::BitwiseXor,
        },
        JsAnyExpression::TsTypeAssertionExpression(_)
        | JsAnyExpression::TsNonNullAssertionExpression(_)
        | JsAnyExpression::JsUnaryExpression(_)
        | JsAnyExpression::JsAwaitExpression(_) => OperatorPrecedence::Unary,
        JsAnyExpression::JsPostUpdateExpression(_) | JsAnyExpression::JsPreUpdateExpression(_) => {
            OperatorPrecedence::Update
        }
        JsAnyExpression::JsCallExpression(_)
        | JsAnyExpression::JsNewExpression(_)
        | JsAnyExpression::JsImportCallExpression(_)
        | JsAnyExpression::JsSuperExpression(_) => OperatorPrecedence::LeftHandSide,

        JsAnyExpression::JsComputedMemberExpression(_)
        | JsAnyExpression::JsStaticMemberExpression(_)
        | JsAnyExpression::ImportMeta(_)
        | JsAnyExpression::NewTarget(_) => OperatorPrecedence::Member,

        JsAnyExpression::JsThisExpression(_)
        | JsAnyExpression::JsAnyLiteralExpression(_)
        | JsAnyExpression::JsArrayExpression(_)
        | JsAnyExpression::JsArrowFunctionExpression(_)
        | JsAnyExpression::JsClassExpression(_)
        | JsAnyExpression::JsFunctionExpression(_)
        | JsAnyExpression::JsIdentifierExpression(_)
        | JsAnyExpression::JsObjectExpression(_)
        | JsAnyExpression::JsxTagExpression(_) => OperatorPrecedence::Primary,

        JsAnyExpression::JsTemplate(template) => {
            if template.tag().is_some() {
                OperatorPrecedence::Member
            } else {
                OperatorPrecedence::Primary
            }
        }

        JsAnyExpression::JsUnknownExpression(_) => OperatorPrecedence::lowest(),
        JsAnyExpression::JsParenthesizedExpression(_) => OperatorPrecedence::highest(),
    };

    Ok(precedence)
}

We can use the function for JsParenthesizedExpression context too:

1. Get a precedence for JsParenthesizedExpression expression.
2. Try to cast parent of JsParenthesizedExpression to JsAnyExpression and get precedence.
3. Compare precedence and decide to keep or remove parenthesis.

[ematipico]

@denbezrukov I think there's some misunderstanding, or I don't really understand some point. The parenthesis logic that we have in the formatter doesn't have any correlation with what we want to do inside the linter, I was just suggesting an alternative solution of how we created a precedence ordering based on the context.

If I understood correctly, you wanted to use what we already have in the formatter for this rule? I strongly suggest to not use it inside the linter. Let's just use the port from eslint and create the parenthesis expression using the mutation API.

Of course we can use the OperatorPrecedence, which already has baked in the correct precedence.

[denbezrukov]

@ematipico Sorry😅
I hope that we can use the function which returns precedence in formatter and lint crate. Because it can handle cases which are inconsistent with prettier now. E.g. a[(1 * 4) * 1]

Maybe it's better to port the function in another PR?

[ematipico]

Maybe it's better to port the function in another PR?

Yes, I suppose so. We can start by implementing the rule by itself, without "dependencies". We can check afterwards what we can do with the formatter. The formatter has still an open issue around parenthesis, so it's not fixed/ready yet.

[denbezrukov]

Hi,

I've handle cases with nullish coalescing and logical or:

https://github.com/denbezrukov/tools/blob/feature/use-optional-chaining/crates/rome_js_analyze/tests/specs/js/useOptionalChain/nullishAndLogicalOr.ts.snap

Now I'm working on:
foo && foo.bar && foo.bar.zoo

[denbezrukov]

Hi,

I've handle cases with logical and.

https://github.com/denbezrukov/tools/blob/feature/use-optional-chaining/crates/rome_js_analyze/tests/specs/js/useOptionalChain/logicalAndCases.js.snap

I'm going to double check test cases, clean code and make MR🥳

[IWANABETHATGUY]

Hi,

I've handle cases with logical and.

https://github.com/denbezrukov/tools/blob/feature/use-optional-chaining/crates/rome_js_analyze/tests/specs/js/useOptionalChain/logicalAndCases.js.snap.new

I'm going to double check test cases, clean code and make MR🥳

Glad to see that!