Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eqeqeq: Don't autofix #4578

Closed
nzakas opened this Issue Nov 30, 2015 · 26 comments

Comments

Projects
None yet
9 participants
@nzakas
Copy link
Member

nzakas commented Nov 30, 2015

Right now, the eqeqeq rule will correct this:

if (foo != null) {}

To this:

if (foo !== null) {}

We should never autofix when != is comparing against null or undefined, as the code might be relying on null == undefined.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Nov 30, 2015

Hang on. There's a special option "allow-null" which will avoid those being
reported as errors. Shouldn't we rely on that in determining whether or not
this is even an error in the first place?

Given the existence of that option, I think it makes more sense to
encourage users to use that option (and avoid an error being reported in
the first place, and thus avoid a bad auto fix) and let the fix logic
assume that all reported errors deserve the fix. To suppress a fix in
certain circumstances will just complicate the fix logic and make the rule
less maintainable.
On Nov 30, 2015 12:55 PM, "Nicholas C. Zakas" notifications@github.com
wrote:

Right now, the eqeqeq rule will correct this:

if (foo != null) {}

To this:

if (foo !== null) {}

We should never autofix when != is comparing against null or undefined,
as the code might be relying on null == undefined.


Reply to this email directly or view it on GitHub
#4578.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Nov 30, 2015

Yes, the allow-null option allows you to omit warnings, however, we should never do this replacement regardless of that option. There is no way for us to know that this is a safe replacement because it changes the meaning in a way that could break (this caused a bug in Espree). Whenever we see != null or != undefined, we should report the problem according to the allow-null option but we should never automatically fix it.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Nov 30, 2015

I'd love to hear more about the espree bug.

To me, users who configure ESLint in a particular manner are implicitly making a promise that they know what they're doing (notwithstanding any improvements we can make to our documentation or to follow principle of least surprise). If someone enables eqeqeq without allow-null, does that not imply that the user wants all != changed to !==? If they want to allow comparisons to null/undefined, they should have configured the rule to have allow-null in the first place. I'm okay in general with the idea that not all errors need to be fixable for a rule, but the reasoning just feels flimsy to me. Maybe if I hear more about what happened in espree, I could be persuaded to come around.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Nov 30, 2015

@platinumazure I'll try to explain this once again.

// These do the same thing
if (foo != null) {}
if (foo !== null && foo != undefined) {}

// These do not do the same thing
if (foo != null) {}
if (foo !== null) {}

In Espree, I have foo != null. ESLint changed it to foo !== null. That caused the Espree tests to hang and it took me a while to track it back to this change because foo was coming in as undefined and therefore it wasn't equal to null.

Regardless of anything else ESLint does, it must not make changes that break functionality. For Espree, if it wasn't "fixed", and instead just warned, then I would have saved about an hour of time.

I know you may not agree with this, but this is the right thing to do.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Nov 30, 2015

I concede that it might be best for our users and am willing to drop the issue for that reason. My point remains, though, that this was really a configuration fault on your end and you really should have had allow-null enabled in espree's ESLint config. I'm okay with protecting users from themselves once in a while, and I can see this being a compelling enough case if it caused an hour to be wasted, but at the end of the day, this was a PEBKAC and I think you know that too.

All that said, yes, let's suppress the fix. And you haven't said it yet, but yes, it would sure be nice if there was a way to log the fixes applied so that users might have an easier way of tracking changes that happened as a result of ESLint's autofix.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 1, 2015

If the goal is to never break their code, eslint can almost never replace == with ===. @nzakas why are you only suggesting this for the == null case?

@Slayer95

This comment has been minimized.

Copy link

Slayer95 commented Dec 1, 2015

+1 @michaelficarra

Why do non-style rules even have an autofix implementation?

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 1, 2015

+1 @michaelficarra, @Slayer95

I'm sorry to say it, but this issue honestly feels like a knee-jerk reaction to a frustrating experience that needs to be thought through a little further. I'm fairly sure you would say the same if someone else brought it up, especially if they were venting their frustration at the same time. I'm sorry if there is a better way I can put this that I haven't thought of.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 1, 2015

@michaelficarra that's a fair point. Maybe the question is whether or not this rule should autofix at all.

@platinumazure I'd appreciate it if you could choose your words more considerately. I feel like you've been attacking me this whole thread because you disagreed. Disagreement is fine, but there are more constructive ways to express it (see how simply @michaelficarra both disagreed and expressed the reason for his disagreement).

@eslint/eslint-team thoughts?

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Dec 1, 2015

So far, I think we have limited fixes to mainly style and whitespace issues. Changing the equality operator fundamentally changes how the code works, so I agree with @michaelficarra that this rule should not be fixable.

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Dec 1, 2015

I think we should remove the auto fix ability for this rule. There has been some concerns in my company also for this rule applying auto-fix.
Personally I dont mind this rule applying the auto-fix.

@nzakas nzakas changed the title eqeqeq: Don't fix if compared to null or undefined eqeqeq: Don't autofix Dec 1, 2015

@nzakas nzakas added enhancement breaking and removed bug labels Dec 1, 2015

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 1, 2015

OK, that's a lot of votes for not auto-fixing at all, so let's remove it.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 1, 2015

@platinumazure I'd appreciate it if you could choose your words more considerately. I feel like you've been attacking me this whole thread because you disagreed. Disagreement is fine, but there are more constructive ways to express it (see how simply @michaelficarra both disagreed and expressed the reason for his disagreement).

That was not my intent. My intent was to ensure you were thinking clearly rather than reacting out of frustration. As project lead, you have the ability to make snap decisions and implement them in a hurry, and I was afraid that was about to happen here. If that came off as a personal attack, I apologize. As for expressing things more constructively and simply, I did that early on, and then you repeated yourself rather than actually taking the time to understand my point. So if we're going to talk about how to be more constructive, perhaps you could also interact more constructively with your users by not assuming they don't know basic JavaScript.

EDIT: That said, yes, you and anyone else here deserves constructive interactions and I will try to do better at that in the future.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 1, 2015

Yeah, I had the same concern that @platinumazure had.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 1, 2015

So if we're going to talk about how to be more constructive, perhaps you could also interact more constructively with your users by not assuming they don't know basic JavaScript.

@platinumazure I understand that your intent wasn't to offend and only to help, however, statements like this are pretty insulting. The reality is that all issue comments need to stand up to inspection by people other than those directly involved. I cannot assume that everyone reading the issue will have all the context and all of the knowledge necessary to understand the discussion. It is my job, as you say, to be as transparent and clear as possible so that anyone who happens upon the issue, regardless of their JavaScript knowledge, has a chance at understanding what's going on. If you find that insulting, them I do apologize.

What I read in your statement was a "blame the user" message, that if someone ended up in a bad situation then it was their fault for not paying close enough attention. That's not the way I run this project. I want to protect users so that even if they make a mistake, we don't end up ruining their code. @michaelficarra's comment pointed out that this rule is not protecting users as a whole.

And for what it's worth, the reason I file issues instead of committing directly to master is so I can get people's input on decisions before they're made. You'll note there was no pull request attached to this issue. Assuming that I'm "reacting" and that I'm "frustrated" is fine to think to yourself, but the evidence indicates that I was opening up a discussion and not "reacting" without thought.

@michaelficarra you expressed your concern in a much more constructive way, and I appreciate that.

@sindresorhus

This comment has been minimized.

Copy link
Contributor

sindresorhus commented Dec 2, 2015

Why not just autofix foo != null to foo !== null && foo !== undefined? That's what I would have expected.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 2, 2015

@sindresorhus A few problems with that:

  1. That could cause precedence issues if the implementation is not careful;
  2. undefined can be overwritten;
  3. And most importantly that's a massive fix that could easily collide with many other rules on that logical expression node and that could surprise users.
@sindresorhus

This comment has been minimized.

Copy link
Contributor

sindresorhus commented Dec 2, 2015

  1. Be careful then. Just wrap it in parens (foo !== null && foo !== undefined) if foo !== null is not alone.
  2. If undefined is overridden it's the wild west anyways. Not something we should consider.
  3. See 1. I don't see why users would be surprised if they already asked to error about it.
@Slayer95

This comment has been minimized.

Copy link

Slayer95 commented Dec 2, 2015

What if foo is a getter with side effects? new Set([null, void 0]).has(foo)?

Really, autofixing eqeqeq is out of scope imo. Autofixing should not change the AST ever.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 5, 2015

@sindresorhus we try to make only surgical changes to the code to avoid breaking functionality. Your example is the simplest possible, but what if it's getFoo() != null, would we then change it to getFoo() !== null && getFoo() !== undefined? What if getFoo() has side effects?

In this case, the safest course of action is just not to do the autofixing.

@sindresorhus

This comment has been minimized.

Copy link
Contributor

sindresorhus commented Dec 5, 2015

@nzakas My suggestion was for variables only, but even those can have side-effects, so makes sense :/

ilyavolodin pushed a commit that referenced this issue Dec 5, 2015

@arnaugm

This comment has been minimized.

Copy link

arnaugm commented Dec 7, 2015

I found myself today in a similar situation. In our team we have been linting our code base as the starting point to have something cleaner. I set the "eqeqeq" rule as a warning because we want to encourage the team to use triple equals but I don't want to break existing code. The fix option changes it anyway so I either remove the option or I take the risk of breaking something.
Could it be possibe to manually mark some controversial rules like this as not fixable through the configuration file?

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 7, 2015

Could it be possibe to manually mark some controversial rules like this as not fixable through the configuration file?

No, we're specifically trying to avoid this. If we can't safely fix the rule 100% of the time, then we just won't fix it at all.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 7, 2015

What about allowing all rules set as warnings not to be auto fixed, via a
CLI option?

On Dec 7, 2015 12:17 PM, "Nicholas C. Zakas" notifications@github.com
wrote:

Could it be possibe to manually mark some controversial rules like this
as not fixable through the configuration file?

No, we're specifically trying to avoid this. If we can't safely fix the
rule 100% of the time, then we just won't fix it at all.


Reply to this email directly or view it on GitHub.

@arnaugm

This comment has been minimized.

Copy link

arnaugm commented Dec 7, 2015

I think not auto fixing warnings is a good idea. At least this is what I was expecting until I realized that they were fixed anyway.
If is not an option, then I would definitely set "eqeqeq" as not fixable due to its potential danger.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 7, 2015

@arnaugm I submitted a PR yesterday that removes autofixing from eqeqeq, so that shouldn't be a problem for much longer. I'm not sure about not autofixing warnings, I'll let other decide on that, since I don't really have use case for autofixing in general.

nzakas added a commit that referenced this issue Dec 7, 2015

Merge pull request #4614 from eslint/issue-4578
Breaking: Remove autofix from eqeqeq (fixes #4578)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.