Eslint: Remove fixer from no-debugger rule

[Dema]

What rule do you want to change?
no-debugger

Does this change cause the rule to produce more or fewer warnings?

Fixer for this rule forces me to put eslint-disable-next-line comment before debugger statement rending this rule useless.

With fixer for no-debugger my IDE's (VS Code and vim) fix-on-save feature removes debugger statement the moment I save the file. I have to use

// eslint-disable-next-line
debugger

construction and I already have debugger statement slipped into production code at least twice.

How will the change be implemented? (New option, new default behavior, etc.)?

Please remove the fixer completely or make it optional. I've read the comment https://github.com/eslint/eslint/pull/8660#issuecomment-315806299 and I still think that for no-debugger rule fixer is kind of useless. You'll never have more that one or two accidentally forgotten statements to force IDE creators to implement such mechanism.

[platinumazure]

Although I left the comment referenced in the initial post for this issue, I will note here that we have removed autofixers that were less painful than this in the past. So I could get behind removing the autofixer.

[zeorin]

As I recall, the guideline for autofixers is that they _shouldn't_ change the execution of the code. This autofixer does. Also in favour of removing it.

[platinumazure]

I'll champion this (although I am championing way too many issues right now).

@eslint/eslint-team Can we get more :+1:s on this issue?

[aladdin-add]

added my 👍 , now it's accepted.

[platinumazure]

Thanks everyone! We'll be happy to review any PRs that come in for this :smile:

[ljharb]

Instead of removing the autofixer, could we use an option to control whether the autofix is applied or not?

[not-an-aardvark]

I'd certainly be open to hearing suggestions for an option, but keep in mind that the topic of optionally performing unsafe autofixes has been discussed in the past and has not been adopted.

[ljharb]

This autofix isn't unsafe at all - it's just inconvenient for those who have configured their editors to autofix on save.

[not-an-aardvark]

By the letter of the spec, "perform an implementation-dependent debugging action" could clearly be a distinct behavior from "return NormalCompletion(undefined)" in some implementations, so I think it's reasonable to classify this as an unsafe fix. (In particular, the fix is surprising because it changes the behavior of the code in common scenarios, by preventing the execution from pausing at a breakpoint.)

[ljharb]

I don't agree; the execution won't pause at all unless you have a console open.

[not-an-aardvark]

The execution will change in an arbitrary implementation-specific way based on ECMA262. I think the specifics of how particular implementations (browsers) implement debugger statements is out of scope for ESLint. (I also think that "having the console open" is a still a case where developers might reasonably expect ESLint to not disrupt the behavior of their code.)

[ljharb]

I think this warranted more discussion than 30 minutes from accepted to merged, and I don't think that anything that's implementation-dependent - especially debugger statements - is something that eslint can consider "disrupting behavior".

[aladdin-add]

Terribly sorry for the inconvenience!! Should we revert the change, and reconsider the issue?

[not-an-aardvark]

I think this warranted more discussion than 30 minutes from accepted to merged

Although I agree that the PR should have been open for 2-3 days in accordance with this, the purpose of the delay in merging a PR is typically to allow sufficient review on the implementation of the PR. In general, I think it's more appropriate to discuss whether a change is worth making before it's marked as accepted (since ostensibly once an issue is marked as accepted, it's only a matter of time before it is implemented and merged).

(edited to add): I realize it's hard to keep track of every issue, but I'm also not sure it would be fair for us to block a PR on the basis of an objection raised after an issue was marked as accepted. We try to use the accepted label as an indication when there is consensus that a change is worthwhile, so a contributor can safely implement the change without worrying that their time will have been wasted.

I don't think that anything that's implementation-dependent - especially debugger statements - is something that eslint can consider "disrupting behavior".

I have the opposite view. If a behavior is explicitly marked as implementation-dependent in the spec, then ESLint can't make any guarantees about how the code will behave, so effectively every fix relating to that behavior would be unsafe.

[zeorin]

That also means effectively that the _effect_ of the debugger statement is non-standard, and using it at all in code is inherently unsafe, from a general standards point of view. If a developer is counting on non-standard behaviour, _caveat emptor_ applies for them anyway.

However, in current usage, the debugger statement does nothing unless there is a debugger open, which is not the normal environment of production code anyway.

Anyone that is running eslint with autofixes is thus expecting the debugger statement to be removed and I cannot imagine they would be using it at all. Why add it just to have it be removed?

Even if someone _were doing exactly that_, there would only be a noticable change if a debugging environment were active. I consider that fairly safe (to remove ESLint's autofix for this rule).

[not-an-aardvark]

That also means effectively that the effect of the debugger statement is non-standard, and using it at all in code is inherently unsafe, from a general standards point of view. If a developer is counting on non-standard behaviour, caveat emptor applies for them anyway.

I don't think that's necessarily the case. A debugger statement might have well-documented behavior in a particular environment; for example, when developing for a particular browser, a user might reasonably have an expectation that a debugger statement will create a breakpoint and pause execution. However, ESLint is behaving independently of the target environment here, so it has less information about the expected behavior of a debugger statement. As a result, ESLint can't make assumptions about the effect of a debugger statement, even if the developer can.

Anyone that is running eslint with autofixes is thus expecting the debugger statement to be removed and I cannot imagine they would be using it at all. Why add it just to have it be removed?

Even if someone were doing exactly that, there would only be a noticable change if a debugging environment were active. I consider that fairly safe.

I think the no-debugger rule is typically used to prevent debugger statements from appearing in production code. It's not intended to prevent developers from using debugger statements at all throughout the development process.

Quite a few users have reported (e.g. in https://github.com/eslint/eslint/issues/7549#issuecomment-313631728) that automatically removing debugger statements is disruptive to their development process, because they expect the browser to create a breakpoint when they insert a debugger statement, and the browser does not do this when the debugger statement is removed. This is precisely what we want to avoid when doing autofixes -- the autofixer is materially changing what happens when the code runs, and it's causing confusion as a result.

[ljharb]

It would only be disruptive if they automatically run autofix on save - so could perhaps an alternative be to those users to "not do that"?

[not-an-aardvark]

This fix is disruptive regardless of whether it's triggered by running autofix on save, or by using the --fix flag on the command line.

In general, we want developers to be able to use --fix without worrying that it will change the behavior of their code. Even if a developer doesn't want debugger statements to appear in production, "remove all debugger statements right now" might not necessarily the desired fix for the presence of debugger statements at any given point. For example, a developer could run the linter while still actively debugging, in which case might want to acknowledge that there are still debugger statements in the code without removing them yet.

[Dema]

@ljharb I wonder, why would anyone wanted to have auto-fix on save turned off? That's the whole point of auto-fixing.
As for debugger statement slipping into production code, this is almost impossible if someone uses any kind of CI. because when building under CI it is common to set all warnings to become errors. And as I said I did end up committing "debugger" statements despite eslint in pre-commit hook because I had to insert // eslint-ignore-next-line comment because there's no way to disable it globally

[mysticatea]

I think reasonable that removing the autofix of no-debugger rule.

ESLint should not fix code automatically if the fix will change that behavior. The debugger statements have behaviors for debugging tools, and the autofix changes the behavior.

[ljharb]

@Dema i don’t agree that’s the point of autofixing; specifically, I’d recommend doing it as part of a precommit or prepush hook and not in your editor.

debugger statements have no behavior in the language unless an implementation injects it. Since eslint can’t assume what an implementation will do, it must act as if it’s the minimum required thing: a noop. Thus, it’s safe to remove.

[zeorin]

Debugger statements have an implementation-specific behaviour. But this autofix is changing the behaviour of my code as a whole.

I know of no other ESLint rule, built-in or provided by a plugin, that does this when autofixing.

It goes against ESLint's own guideline for autofixing, and as evidenced by so many comments and complaints, debugger statements are not safe to remove on autofixing.

ESLint is a great tool, and part of its utility is that it is useful in different contexts.

This autofix makes ESLint less versatile as an on-save fixing tool. ESLint is _opinionated about my workflow_ because of this autofix.

If I want something to change the behaviour of my code on save/build/run/commit, there are tools better suited to the job. I don't merely think this autofix is annoying, I think it's _out of scope_ for any linting tool.

[Dema]

There are three main reasons I want autofix-on-save

1) I use prettier and have it format-on-save, I'd love to have eslint's fix-on-save run after that

2) I don't like to see all those irrelevant warnings in editor like wrong spaces, order of imports and other things that can be fixed. If I can't fix them on save, I have to manually fix those issues which makes autofix feature useless.

2) Small number of relevant warnings can be overseen in a huge list of fixable warnings.

And for me this fixer is not just annoying, it's insanely annoying. Yesterday I added debugger statement to my other project, switched to a browser, spent couple of minutes walking through the app and was wondering why it didn't stop. And then I realized that I forgot to turn this rule off for this project.

[not-an-aardvark]

debugger statements have no behavior in the language unless an implementation injects it. Since eslint can’t assume what an implementation will do, it must act as if it’s the minimum required thing: a noop. Thus, it’s safe to remove.

This doesn't make sense to me.

N.B.: I'm using the term "safe" to mean that a given assumption will hold under every implementation behavior allowed by ECMA262 (since ESLint should be able to work on code that runs under any ECMA262-compliant engine).

Since ESLint doesn't know what an implementation will do, it's not safe to assume that the engine will treat the debugger statement as a noop. This assumption would hold for some allowed implementation behaviors, but it would not hold for all allowed implementation behaviors (specifically, the assumption would be incorrect for any implementations that do something else with debugger statements, such as running a custom debugging hook or dumping the heap to a file).

In other words, removing debugger statements would change the behavior of code on a non-empty subset of ECMA262-compliant engines, so I don't think we should do it in an autofixer.

[ljharb]

Is there a reason we couldn't use an option - in either direction - to enable or disable the autofixer, rather than removing it?

[zeorin]

That's better than nothing, but there has been resistance to the idea of allowing autofix behaviour to be configured on a per-rule level. On mobile rn so can't find the link easily.

[platinumazure]

Is there a reason we couldn't use an option - in either direction - to enable or disable the autofixer, rather than removing it?

Unfortunately, there's just no core support for it right now. I suppose we could add an option which would specifically emit or not emit a fix function, but that's something we haven't done in any core rule before and I don't know if there would be team support for it.

Everything you've said about text editor integration (and how they should not autofix everything on save) is exactly correct, but we have to develop for our users' actual reality rather than the ideal world. Unfortunately: There are editor integrations which autofix indiscriminately on save; there are users who use said integrations and are inconvenienced by the autofixer; and some of these users use inline disable comments, and then risk their debugger statements getting into production code.

On the other hand, for your use case, it seems to me that massive numbers of debugger statements (needing autofix) would only really occur when adding ESLint to existing projects or otherwise needing to fix lots of code at once; for incremental development, I would assume developers would normally add few debugger statements per changeset, and then could fix those manually and not be hurt too badly by lack of autofix.

So I will admit, I championed this autofix removal because honestly the impacts to use cases seemed fairly lopsided to me. If I am missing something about how your users tend to operate, then I apologize for pushing for this without knowing about those use cases.

I'll got ahead and note that you are more than welcome to develop a custom rule that (for example) uses eslint-rule-composer to run the core no-debugger rule and then modify the lint message to include a fixer. This might be the best way forward for you given the current lack of ability to autofix per rule.

[Dema]

Could you explain a bit why do you think that autofix on save is a bad thing? I genuinely would like to know.

[ljharb]

@Dema sure! basically, autofixing risks paving over the developer's intent. As such, the developer who wrote the code needs to very carefully review the autofixes, to ensure their intent is preserved. Autofix-on-save doesn't give them the opportunity to review this (ie, as a diff) - whereas committing, and then autofixing, and then amending as needed, does.

[zeorin]

When few things were being auto-fixed, this made sense. However, I think that's no longer necessarily the case: the mindset around auto-fixing is changing for some developers, and it's because of tools like Prettier.

Both Prettier and (ostensibly) ESLint auto-fix don't change the execution logic of your code, so it's natural that Prettier auto-formatting and ESLint auto-fixing are thought of in the same way and executed at the same times in certain developers' workflows.

Prettier changes a lot more code than most ESLint configs and reviewing all of that would lead to a lot of noise in the signal, and also many changes made at once is very jarring compared to small changes made over time (as you save).

Your own code could look unfamiliar if you auto-format and auto-fix e.g. only once you're ready to commit or have finished creating and editing a file. You'd have to re-orient and re-familiarize yourself with the new code.

I certainly find auto-formatting and auto-fixing less disruptive when they happen on save.

[Dema]

Yep, If I use autofix-on-save, those fixes are small and focused on the code I just wrote because I save very frequently. When I stop typing, I hit save and prettier and eslint fixes everything and those fixes are only in the 1-2 lines of code I just wrote. It feels seamless.

[michaelficarra]

There seems to be two conflicting goals people have when using eslint's auto-fixing.

1. Those running it on save want only stylistic rules fixed. In my opinion, this is the domain of prettier.
2. Those using it so that they don't have to manually fix the things that they've told eslint are undesirable.

For the first group, changing anything that we could fix automatically but which changes the structure/behaviour of the program is unwanted. For the second group, not changing something that we could fix automatically adds burden on them.

It looks to me like the way to resolve this is to have auto-fixing run in one of two modes: fix only stylistic things, and fix anything that we possibly can.

[Dema]

Could you give some examples of fixes that change structure/behaviour except no-debugger? I've looked through eslint rules bundled with eslint itself and didn't really find anything I don't want to be fixed on-save.

[zeorin]

@michaelficarra

anything that we could fix automatically but which changes the structure/behaviour of the program

Literally only the no-debugger rule does this, even amongst most ESLint plugins (certainly 100% of the ones I've used). This is no accident. It is because fixers _shouldn't_ change the behaviour of the program.
Anything that doesn't fall into the category of "change the behaviour of the program" _is_ a stylistic change, whether it's a change coming from ESLint or Prettier or another tool.

That means that all ESlints autofixers ought to be merely stylistic. And they all are, apart from no-debugger, which is why it's a problem.

[not-an-aardvark]

This idea was also discussed in https://github.com/eslint/eslint/issues/7873 (and has come up in a few other places, e.g. https://github.com/eslint/eslint/issues/7714). We've generally been following the policy that autofixers should never change the behavior of user code. I'm not convinced we could ever be sure enough of the user's intentions that it would be a good idea to change the behavior of their code without user input.

[mysticatea]

Anyway, our current policy is that autofix must not change the behavior of user code, so this issue is a bug. We have to remove the autofix of the no-debugger rule.

If anyone wants to change the policy, it's another issue. We can re-add the feature after the policy was discussed and changed.

[platinumazure]

Re-labeling as a bug based on ESLint policy.

@not-an-aardvark @mysticatea @Aladdin-ADD What are our next steps here? Is the fixer removed? Or do we need to remove it again?

[aladdin-add]

I had reverted the change in #10426 , seems we have to remove it again.

[ljharb]

I don't have an issue with the policy; but I disagree that this changes behavior - the debugger is not runtime behavior.

[platinumazure]

@ljharb As I've already noted, you are more than welcome to create a custom rule that adds its own fix logic. Using eslint-rule-composer would also save on maintenance by basing the logic on the original no-debugger rule as well.

But at this point, I don't think we can justify having an autofix in the core rule itself while there is any possibility that runtime behavior could be changed (on any platform). On top of that concern, it's also a terrible experience for the common fix-on-save use case.

If you like, we could discuss it in the TSC and get a final decision there. But at this point, the team seems to believe that we need to remove the autofix.

Thanks for a robust discussion so far and for keeping the discussion respectful (even and especially when we didn't follow our policy on PR review time by accident). I really appreciate it.

[ljharb]

Please realize, though, that "you can create a custom rule" just isn't a viable option, since I maintain a shared config that can't, in a practical sense, be migrated to use a plugin, and adding new peer deps is a massive burden.

[Dema]

Yep, it's the same process for us who don't want this exact rule. I have to install separate package to be able to create my own rule inside my every project just to have no-debugger rule without the fixer. It's so much more work than to delete just one or two debugger statements I left after debugging session looking at eslint warnings.

[JanisE]

I agree with michaelficarra's suggestion about having the auto-fix in two modes.

I, for example, do not understand why would you add no-debugger rule in the error mode, and not want it to be removed by the auto-fixer. I'm running the auto-fixer only upon the final production version of my code, and the more it can do, the better. Fixing the no-debugger by just removing it (or commenting it out) seems to be the only expected behaviour of fixing the problem, so it should be implemented like that.

[Dema]

It fixes not only in error mode but in warning mode too. https://github.com/Microsoft/vscode-eslint/issues/208. I understand that in vscode you can disable this rule, but I use both vscode and vim.

[ljharb]

You should be able to configure any editor integration to only fix errors; if not, that’s a bug in the editor integration that shouldn’t force a change in eslint.

[zeorin]

Autofixing this changes the execution, and is thus contrary to ESLint's design principles.

If people need an autofix version of this it's simple enough to create a plugin for it themselves.