v3.6 ignores "smart-tabs" rule and notifies of indent errors

[IvanSanchez]

I'm a maintainer of Leaflet, and we use an eslint config with smart-tabs, like:

      "no-mixed-spaces-and-tabs": [2, "smart-tabs"],
      "indent": ["error", "tab", {"VariableDeclarator": 0}],

eslint 3.0 through 3.5 work just fine. However, after an upgrade to eslint 3.6, we've got error messages like the following:

/home/ivan/devel/Leaflet/src/control/Control.js
   64:7  error  Expected indentation of 2 tabs but found 4 spaces and 2 tabs  indent

This can be reproduced by checking out the Leaflet repo, then running npm install and then npm test. Note that running npm install eslint@3.5 then npm test will work as expected.

We don't use any transpiler in the Leaflet toolchain; full eslint config is available on the Leaflet repo; and the bug is reproducible on the environments of more than one developer.

Looking at eslint's changelog, I'll guess that this is caused by #7076 (ping @not-an-aardvark)

[IvanSanchez]

Note that I've just clamped the eslint dependency to 3.5. Please keep this in mind when trying to reproduce the bug, you might need to npm install eslint@3.6; npm test.

[platinumazure]

We did recently add functionality to support reporting of mixed spaces and tabs in the indent rule. This was done as a generalization of solving the issue of indent failing to fix tabs to spaces or vice versa (i.e., not a mix, just the wrong character outright).

Given that we have no-mixed-tabs-and-spaces, I think indent should choose not to report mixtures. I think this could also be seen as a regression, but we'll see what the rest of the team thinks.

Marked "evaluating" for now because I can't confirm on my mobile, but I am 95% sure it's reproducible.

@IvanSanchez Could you help us out by providing a code example? You've only listed configuration and an error message. Thanks!

[platinumazure]

Confirmed in online demo with this example:

/* eslint-env es6 */
/* eslint indent:["error", 4], no-mixed-spaces-and-tabs:["error", "smart-tabs"] */

const foo = "bar",
      bar = "baz";   // N.B. One tab, two spaces

Hope it's suitable.

[not-an-aardvark]

@platinumazure I'm a bit confused by the example; shouldn't that report an error if the expected indent is 4 spaces?

[platinumazure]

no-mixed-spaces-and-tabs flags mixture of space and tabs, except in certain circumstances. In "smart-tabs" mode, it allows you to use a mixture if it's for the purpose of lining up a variable name.

One could make an argument that perhaps the mixed tab/space responsibility does belong to indent, but historically they have been separate rules.

I'm still hoping for a better example from @IvanSanchez at some point, since I don't know if my example is very good or not.

[not-an-aardvark]

I think I understand the general idea, but looking at your example:

/* eslint-env es6 */
/* eslint indent:["error", 4], no-mixed-spaces-and-tabs:["error", "smart-tabs"] */

const foo = "bar",
      bar = "baz";   // N.B. One tab, two spaces

The expected indent for that line is 4 spaces, so even ignoring the tabs, indent should throw an error since there are only 2 spaces.

[platinumazure]

Well, as I said, I'm making do and inferring based on insufficient information.

I guess the correct thing to do here is to have eslintbot get involved...

[eslintbot]

Hi @IvanSanchez, thanks for the issue. It looks like there's not enough information for us to know how to help you.

If you're reporting a bug, please be sure to include:

1. The version of ESLint you are using (run eslint -v)
2. What you did (the source code and ESLint configuration)
3. The actual ESLint output complete with numbers
4. What you expected to happen instead

[IvanSanchez]

OK, I'm not used to your workflow so I wasn't fully aware that you expected a minimal example from me. Sorry about that.

I copy-pasted some of our production code and made this small reproducible example:

/* eslint indent:[2, "tab", {"VariableDeclarator": 0}], no-mixed-spaces-and-tabs:[2, "smart-tabs"] */

// Code snipped from https://github.com/Leaflet/Leaflet/blob/master/src/layer/Layer.js

L.Layer = L.Evented.extend({
    _updateZoomLevels: function () {
        var minZoom = Infinity,
            maxZoom = -Infinity,
            oldZoomSpan = this._getZoomSpan();

        for (var i in this._zoomBoundLayers) {
            var options = this._zoomBoundLayers[i].options;
            minZoom = options.minZoom === undefined ? minZoom : Math.min(minZoom, options.minZoom);
            maxZoom = options.maxZoom === undefined ? maxZoom : Math.max(maxZoom, options.maxZoom);
        }

        this._layersMaxZoom = maxZoom === -Infinity ? undefined : maxZoom;
        this._layersMinZoom = minZoom === Infinity ? undefined : minZoom;

        if (oldZoomSpan !== this._getZoomSpan()) {
            this.fire('zoomlevelschange');
        }
    }
});

It seems that tabs/spaces get lost in markdown formatting, so I've put the code in a gist.

Note the spaces and tabs in the variables declaration:

→   →   var·minZoom·=·Infinity,
→   →   ····maxZoom·=·-Infinity,
→   →   ····oldZoomSpan·=·this._getZoomSpan();

Using eslint v3.5.0 (or any previous version), there are no linting errors. This is the expected behaviour.

Using eslint 3.6.1, I get the following output:

/tmp/eslint-smart-tabs-bug.js
   9:7  error  Expected indentation of 2 tabs but found 4 spaces and 2 tabs  indent
  10:7  error  Expected indentation of 2 tabs but found 4 spaces and 2 tabs  indent

✖ 2 problems (2 errors, 0 warnings)

[nzakas]

It looks like the only problem here is that the smart tabs option isn't being respected. If we can fix that, we should be good to go.

[nzakas]

I'll take a look.

[platinumazure]

@nzakas

It looks like the only problem here is that the smart tabs option isn't being respected. If we can fix that, we should be good to go.

I'm not sure it's that simple-- smart-tabs is an option in no-mixed-spaces-and-tabs, not in indent. That option is being respected in no-mixed-spaces-and-tabs, the problem is indent is now warning on mixed spaces and tabs when it shouldn't. (Long-term, I could see an argument for getting rid of no-mixed-spaces-and-tabs and folding that functionality into indent, but right now there's a clash of rule responsibilities.)

[nzakas]

Ah yes, looked at this too quickly, you're right. I'll look at reverting that for now and we can revisit whether or not there should be two rules at a later point.

[nzakas]

@platinumazure it looks like this change came in as a result of an issue you opened: #4274. Can you comment on the effect if we revert this commit? (8b3fc32#diff-f7ae198ea24182d178b8f3376e61b6ed)

[platinumazure]

@nzakas Let me know if this makes sense:

1. The original problem I reported was that if a user used "tab" indent and ran ESLint on a file with spaces, or vice versa, the errors would show up as "Expected 1 tab but found 0" (i.e., no tab characters were found, because there were spaces).
2. Auto-fix with indent was not suitable due to this, because indent would effectively remove all indentation.
3. not-an-aardvark generalized the problem to try to catch any mixture of spaces and tabs, so that auto-fix could fix any such combination into the appropriate number of spaces or tabs (depending on settings).

So if the commit is reverted, the problems described in 1 and 2 above will reappear.

IMO, the correct solution is for indent to only report if there are no mixtures of tabs and spaces (i.e., either only spaces are used when tabs should be, or vice versa, or if the correct indent character but wrong number of characters is used). Obviously, if we are considering a patch release to fix this, a full revert may be simpler and the correct solution can go in the next minor.

Hope this helps, let me know if I can clarify anything.

[not-an-aardvark]

A simple fix would be to have a special case in indent to avoid reporting "some tabs followed by some spaces", while still reporting other cases where there are unexpected tabs/spaces.

edit: Alternatively, we could just have indent not report any cases with mixed tabs and spaces, to avoid conflicts with no-mixed-spaces-and-tabs. However, I don't think fully reverting 8b3fc32#diff-f7ae198ea24182d178b8f3376e61b6ed is a good idea, because that will bring back the bug that @platinumazure mentioned.

[platinumazure]

@not-an-aardvark Agreed, that's the correct solution. The question is whether we want to put that in right before an emergency patch release, rather than just revert the commit so we're at least in a known state.

[nzakas]

We inadvertently introduced a breaking change here, and we need to fix that quickly. If someone can make the correct fix, that's fine, but if no one has the time to do that today, then we should revert the commit to do a patch release. We can always add back the changes later with the correct fix.

This is also a good reminder to make sure each PR solves only one problem so that reverts are clean.

[not-an-aardvark]

If someone can make the correct fix, that's fine, but if no one has the time to do that today, then we should revert the commit to do a patch release.

I made a PR to fix the issue at #7266.

This is also a good reminder to make sure each PR solves only one problem so that reverts are clean.

Fair enough, will keep that in mind for future PRs.