Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Suggestion: noAssignInExpressions fixer is incorrect #4717

Open
1 task done
JP250552 opened this issue Jul 20, 2023 · 7 comments
Open
1 task done

Suggestion: noAssignInExpressions fixer is incorrect #4717

JP250552 opened this issue Jul 20, 2023 · 7 comments
Labels
A-Linter Area: linter L-JavaScript Langauge: JavaScript

Comments

@JP250552
Copy link

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

  • I agree to follow Rome's Code of Conduct
@JP250552 JP250552 added the S-To triage Status: user report of a possible bug that needs to be triaged label Jul 20, 2023
@Conaclos Conaclos added A-Linter Area: linter L-JavaScript Langauge: JavaScript and removed S-To triage Status: user report of a possible bug that needs to be triaged labels Jul 27, 2023
@Conaclos Conaclos changed the title 🐛 noAssignInExpressions fixer is incorrect Suggestion: noAssignInExpressions fixer is incorrect Jul 27, 2023
@Conaclos
Copy link
Contributor

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.

@JP250552
Copy link
Author

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.

@ematipico
Copy link
Contributor

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
Copy link
Contributor

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?

@JP250552
Copy link
Author

JP250552 commented Jul 27, 2023

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

@ematipico
Copy link
Contributor

ematipico commented Jul 27, 2023

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.

@Conaclos
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter L-JavaScript Langauge: JavaScript
Projects
None yet
Development

No branches or pull requests

3 participants