🐛 noUnusedVariables --write applies the underscore fix instead of removal for imports

[peter-goodfill]

Environment information

CLI:
  Version:              10.0.1
  Color support:        true

Platform:
  CPU Architecture:     x86_64
  OS:                   linux

Environment:
  ROME_LOG_DIR:         unset
  NO_COLOR:             unset
  TERM:                 "xterm-256color"

Rome Configuration:
  Status:               loaded
  Formatter disabled:   false
  Linter disabled:      false

Workspace:
  Open Documents:       0

Discovering running Rome servers...

Running Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ The client isn't connected to any server but rage discovered this running Rome server.

Server:
  Version:              10.0.1
  Name:                 rome_lsp
  CPU Architecture:     x86_64
  OS:                   linux

Workspace:
  Open Documents:       0

Other Active Server Workspaces:

Workspace:
  Open Documents:       0
  Client Name:          Visual Studio Code
  Client Version:       1.73.1

Workspace:
  Open Documents:       10
  Client Name:          Visual Studio Code
  Client Version:       1.73.1

Rome Server Log:

⚠ Please review the content of the log file before sharing it publicly as it may contain sensitive information:
  * Path names that may reveal your name, a project name, or the name of your employer.
  * Source code

├─22140085ms INFO rome_lsp::server Starting Rome Language Server...

What happened?

1. By setting the "noUnusedVariables": "error" and running rome with --write
2. The unused imports are not removed, but prefixed with _

Expected result

The unused imports are simply removed in autofix (--write) mode

Code of Conduct

• I agree to follow Rome's Code of Conduct

[ematipico]

The rule works as intended, for example:

app.on((ctx, res)) => {
	res.send(200)
})

Here, ctx is marked as unused, which is a correct case for this rule. Removing ctx will lead to a syntax error. That's why the team decided to add an _ to the binding name, which is a wide unwritten custom to mark unused variables.

@MichaReiser what kind of enhancements you had in mind? It's not clear unfortunately

[MichaReiser]

It's about imports:

import { a } from "b";

// a not used

After running npx rome check . --apply-suggested you get

import { _a } from "b";

Rome should remove the import instead.

[MichaReiser]

@xunilrj what are your thoughts on this suggestion?

[stefanoverna]

Rome should remove the import instead.

This would be really useful!

[xunilrj]

Ideally yes, we should remove. Actually, the first version of the rule default suggestion was to remove unused bindings. The problem is that I am not entirely sure this is the best suggestion anymore...

First, when variables are unused because of typos, removing them is not what the user wants. Maybe the best suggestion would be to search for unresolved references with similar names.

Second, if the variable has a complex initializer, all the code is lost. Same thing for functions, classes etc... We can argue if imports are on the same level of complexity or not. Maybe not, and import makes more sense.

I think what we want (at least for now) is code actions to remove unused bindings.

Nonetheless, we don't suggest renaming imports anymore: https://github.com/rome/tools/pull/3860/files#diff-faa3dc18f8fdd68c8c5bb127968eea846ef84f5d3c21d642f9ee82ad515b7e73