🐛 Files with fn(() => (aborted = true)); cause - entered unreachable code - when --apply-unsafe

[mrazorvin]

Environment information

Rome: v12.0.0

What happened?

command ./node_modules/.bin/rome check ./packages --apply-unsafe

  res.onAborted(() => (aborted = true));

cause entered unreachable code

Source Location: crates/rome_js_syntax/src/expr_ext.rs:489:18
Thread Name: rome::worker_5
Message: internal error: entered unreachable code

Expected result

Fixed XXXX file(s) in YYYYms

Code of Conduct

• I agree to follow Rome's Code of Conduct

[ematipico]

I believe this issue was fixed. Could you try one of our latest nightlies? We're going to do a new release soon.

[denbezrukov]

I've checked the reason why Rome crashes here.
We have the noAssignInExpressions rule.

tools/crates/rome_js_analyze/src/analyzers/suspicious/no_assign_in_expressions.rs

Lines 107 to 125 in 22ce6c6

	fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
	let assign = ctx.query();
	let op = assign.operator().ok()?;
	if let JsAssignmentOperator::Assign = op {
	let operator_token = assign.operator_token().ok()?;
	let new_operator_token = make::token(JsSyntaxKind::EQ3);
	let mut mutation = ctx.root().begin();
	mutation.replace_token(operator_token, new_operator_token);
	Some(JsRuleAction {
	mutation,
	applicability: Applicability::MaybeIncorrect,
	category: ActionCategory::QuickFix,
	message: markup!("Did you mean '==='?").to_owned(),
	})
	} else {
	None
	}
	}
	}

We replace the token here with ===. However, the node remains a JsAssignmentExpression, and the subsequent noSelfAssign rule tries to retrieve the operator from the JsAssignmentExpression. This causes an issue because the node now contains ===.

I guess that the solution is to convert JsAssignmentExpression into JsBinaryExpression in the noAssignInExpressions rule.

[Conaclos]

You should certainly rewrite your code in the following way:

  res.onAborted(() => { aborted = true; return aborted });

Or if you don't need to return:

  res.onAborted(() => { aborted = true });

[mrazorvin]

You should certainly rewrite your code in the following way:

  res.onAborted(() => { aborted = true; return aborted });

Or if you don't need to return:

  res.onAborted(() => { aborted = true });

This is the point of my issue, I expected this suggestion from linter in console not as comment on github

[Conaclos]

The linter is not able to know if you mean () => { aborted = true } or () => aborted === true. However, the parenthesis around the assignment could suggest that the former suggestion is better than the latter. We decided to opt for the latter one when the rule was designed.

[mrazorvin]

We have big codebase where code written by different people.
I'm okay with any linter notification that I can propagate to other team members, but when liter panic, I get no info what is wrong.

It's obviously that code should be rewritten, but I expected this information from linter

[Conaclos]

@mrazorvin We totally agree. This should be fixed by #4473 in the following release :)

[Conaclos]

For now you can disable the rule that creates the issue:

{
  "linter": {
    "enabled": true,
    "rules": {
      "suspicious": {
        "noAssignInExpressions": "off"
      }
    }
  }
}

[mrazorvin]

@mrazorvin We totally agree. This should be fixed by #4473 in the following release :)

thanks, It just not always easy to change code immediately, especially when there more of those places and you need to find them manually (there not info which file contains wrong code)

[Conaclos]

This is a bug. Rome is not intended to behave in this way. Thanks for the reporting!