tools icon indicating copy to clipboard operation
tools copied to clipboard

Suggestion: `noAssignInExpressions` fixer is incorrect

Open JP250552 opened this issue 2 years ago • 7 comments

Environment information

CLI:
  Version:                      12.1.3-nightly.f65b0d9
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  ROME_LOG_DIR:                 unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v18.16.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "yarn/3.6.1"

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

Workspace:
  Open Documents:               0

Discovering running Rome servers...

Incompatible Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ Rage discovered this running server using an incompatible version of Rome.

Server:
  Version:                      12.1.3

What happened?

The fixer for noAssignInExpressions is transforming the statement to an equivalency check rather than the intended assignment.

image

Expected result

I'm not sure there should be a fixer on this rule without understanding the intent.

Code of Conduct

  • [X] I agree to follow Rome's Code of Conduct

JP250552 avatar Jul 20 '23 20:07 JP250552

This is more a suggestion than a bug. The fix is marked as unsafe. By the way, I mostly accept with the concern. I think we should remove the suggested fix.

Conaclos avatar Jul 27 '23 14:07 Conaclos

The fix is marked as unsafe.

Maybe this is where the issue is? The vscode plugin is automatically making the fix in the screenshot and I did not opt into anything unsafe.

JP250552 avatar Jul 27 '23 15:07 JP250552

The fix is marked as unsafe.

Maybe this is where the issue is? The vscode plugin is automatically making the fix in the screenshot and I did not opt into anything unsafe.

It's possible you changed your VSCode settings to accept these changes automatically

ematipico avatar Jul 27 '23 15:07 ematipico

This is more a suggestion than a bug. The fix is marked as unsafe. By the way, I mostly accept with the concern. I think we should remove the suggested fix.

Are you sure we should remove the suggested fix?

ematipico avatar Jul 27 '23 15:07 ematipico

The fix is marked as unsafe.

Maybe this is where the issue is? The vscode plugin is automatically making the fix in the screenshot and I did not opt into anything unsafe.

It's possible you changed your VSCode settings to accept these changes automatically

I don't believe I have set anything regarding unsafe changes (aside from organizeImports). How would that be configured?

image

JP250552 avatar Jul 27 '23 16:07 JP250552

Yeah, that's the configuration that applies code changes on save :)

There isn't a distinction between safe and unsafe code changes at the moment.

ematipico avatar Jul 27 '23 16:07 ematipico

Are you sure we should remove the suggested fix?

It seems that some users deliberately use assignments in some expression kinds (conditionals, inline arrow functions). A user can then be very surprised by the suggestion even in unsafe mode. We could so completely remove the action or avoid suggestions in common cases.

Conaclos avatar Jul 27 '23 19:07 Conaclos