🐛 Rule: noDelete is not safe to auto-apply

[ysageev]

Environment information

$ npx rome rage
CLI:
  Version:              10.0.1-nightly.00266da
  Color support:        true

Platform:
  CPU Architecture:     x86_64
  OS:                   windows

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...

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

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

Server:
  Version:              <=10.0.0

What happened?

It may be recommended to prefer obj.a = undefined to delete obj.a, but it should not be considered safe to auto-apply this rule.

For example, suppose you have an algorithm that relies on count of keys of an object, and you were previously using delete.

let obj = {a: "foo", b: "bar"}

obj.a = undefined
console.log(obj)
// {a: undefined, b: 'bar'}
console.log(Object.keys(obj)
// ['a', 'b']
delete obj.a
console.log(obj)
// {b: 'bar'}
console.log(Object.keys(obj)
// ['b']

Update: Related thread: https://github.com/rome/tools/issues/3720#issue-1447879014

Expected result

noDelete should not be included in the list of rules that --apply-suggested uses.

Either that, or more clearly specify the meaning of --apply vs. --apply-suggested since many users will think "suggested" rules are safe to apply. If it's not safe, why suggest it?

Code of Conduct

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

[ematipico]

We do have a section https://docs.rome.tools/linter/#code-fixes

Is there something else we should improve?

[MichaReiser]

Would it help to rename apply to apply-safe

[sebmellen]

@MichaReiser I think that would be a fair thing to do, but I would more explicitly name the --apply-suggested feature as something like --apply-danger. In my local package.json script I've renamed it to lint:fix:danger so that is is clear the flag may apply unsafe fixes.

React has a great way of doing this with super long names like DangerouslySetInnerHTML. But I think giving it a name like --apply-danger or --apply-dangerous would be plenty.

You could also have both --apply-safe and --apply-unsafe.

[Conaclos]

--apply-unsafe could imply --apply-safe?

[ysageev]

Would it help to rename apply to apply-safe

Something like that or what @sebmellen suggested to make it more explicit.

Maybe:

--apply-safe-only --apply-safe-and-unsafe

Personally, I prefer not offering the ability to auto-apply unsafe rules. They should just generate alerts. Only safe rules should be available to auto-apply.

In that case, you would only make available --apply-safe.

[github-actions[bot]]

👋 @rome/staff please triage this issue by adding one of the following labels: S-Bug: confirmed, S-Planned , S-Wishlist or umbrella

[seansfkelley]

useOptionalChain is another such rule that makes an unsafe "fix" under the --apply-suggested setting.

Specifically,

foo != null && foo.some(bar);

is changed to

foo?.some(bar);

which is not the same.

(I can file another issue for this if you'd prefer, but since this issue has become more a question of phrasing/documentation and less about the rule itself behaving in an undesirable way, I figure the same resolution would apply to useOptionalChain.)

fwiw, I would rename --apply-suggested to --apply-unsafe, which allows --apply to be a safe default happy path and makes clear where the footgun is and keeps the number of flags down. --apply-unsafe would then imply --apply.