Fix: fix indent errors on multiline destructure

[VictorHom]

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix (template)

Tell us about your environment

• ESLint Version:4.0.0
• Node Version:6.9.5
• npm Version:3.10.10

What parser (default, Babel-ESLint, etc.) are you using?
default
Please show your full configuration:

What did you do? Please include the actual source code causing the issue.
I have a fix for errors that were being reported on multiline destructure as reported in #8729
I Added test cases for a bug where errors were reported on indentation on multiline destructure

Is there anything you'd like reviewers to focus on?
Not sure if my approach is correct. Any feedback is welcome.

[eslintbot]

Thanks for the pull request, @VictorHom! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[eslintbot]

LGTM

[not-an-aardvark]

Thanks for the PR! However, I'm not sure this fix is correct. It makes the rule handle the specific case in the bug report correctly, but I don't think it fixes the underlying issue.

The correct fix might actually be a lot simpler -- see my comments below.

[not-an-aardvark]

I don't think this case should be considered valid. If possible, we should enforce only one valid indentation for any given code. Can you move this to the invalid section?

(edit: this has been fixed, but this comment isn't displaying as outdated for some reason.)

[not-an-aardvark]

It seems like your logic relies a lot on finding matching brackets for destructuring. However, I'm not sure this is a good approach -- I don't think the problem is specific to whether the brackets match. For example, the rule will also incorrectly report an error for:

const [
    foo
] = bar(
    baz
);

[not-an-aardvark]

I think the root cause of the problem is that the = token's indentation is not set correctly in the VariableDeclarator listener. By design, offsets.ignoreToken is called on the token, but this is a no-op if the token is not the first of its line. So the = token ends up with an offset of 1 from the start of the declarator, and then the value on the right of the equals sign is matched against the indentation of the equals sign. As a result, the rule indents value on the right of the equals sign by 1 from the start of the declarator, when it should be indented by 0.

I think this can be fixed by explicitly specifying the indentation of the = token relative to the declarator. Applying the following diff to the original version of the file seems to make everything work correctly and fixes the bug:

diff --git a/lib/rules/indent.js b/lib/rules/indent.js
index 5a8feee8..4b4355b3 100644
--- a/lib/rules/indent.js
+++ b/lib/rules/indent.js
@@ -1242,6 +1242,7 @@ module.exports = {
                     offsets.ignoreToken(equalOperator);
                     offsets.ignoreToken(tokenAfterOperator);
                     offsets.matchIndentOf(equalOperator, tokenAfterOperator);
+                    offsets.matchIndentOf(sourceCode.getFirstToken(node), equalOperator);
                 }
             },
 

[VictorHom]

thanks for the explanation. That makes sense

[eslintbot]

LGTM

[not-an-aardvark]

Thanks! This looks good to me. I have a few suggestions for tests, but they shouldn't block this.

[not-an-aardvark]

Can you also add tests to make sure these are valid?

const
    {
        a
    } = {
        a: 1
    };

const
    foo = {
        bar: 1
    }

[eslintbot]

LGTM

[not-an-aardvark]

Thanks for contributing!