New: lines-around-directive rule (fixes #6069)

[kaicataldo]

No description provided.

[mention-bot]

@kaicataldo, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nzakas, @alberto and @mysticatea to be potential reviewers

[eslintbot]

LGTM

[mysticatea]

Thank you for implementation!

My intention was that this rule handles directive prologues (not restrict "use strict").

A Directive Prologue is the longest sequence of ExpressionStatement productions occurring as the initial StatementListItem or ModuleItem productions of a FunctionBody, a ScriptBody, or a ModuleBody and where each ExpressionStatement in the sequence consists entirely of a StringLiteral token followed by a semicolon. The semicolon may appear explicitly or may be inserted by automatic semicolon insertion. A Directive Prologue may be an empty sequence.
http://www.ecma-international.org/ecma-262/7.0/#directive-prologue

A directive prologue might be multiline:

"use asm"
"use strict"

[kaicataldo]

Ah, okay. So to check each directive in the prologue I could maybe do something like this (to borrow this implementation)

function getDirectives(statements) {
    const directives = [];

    for (let i = 0; i < statements.length; i++) {
        const statement = statements[i];

        if (
            statement.type === "ExpressionStatement" &&
            statement.expression.type === "Literal"
        ) {
            directives[i] = statement;
        } else {
            break;
        }
    }

    return directives;
}

Adding do not merge label until I update this.

[mysticatea]

Thank you.
I think good to move the logic to ast-utils.

By the way, directives[i] = statement seems a bug. It should be directives.push(statement) I think.

[kaicataldo]

You're right! Do you mean adding a getDirectives utility to ast-utils? Thanks!

[mysticatea]

Yes!

[kaicataldo]

Got it working for multiple directives and then realized I wasn't sure if we want to check each directive individually or if we want to check around the entire directive prologue. Any thoughts on this @mysticatea?

Now that I know we're checking all directives, I think it makes sense to have an option to enforce blank newlines around the whole prologue (as a group). This wasn't discussed in the accepted issue though, so maybe we should talk about it there.

[mysticatea]

Actually, my first post #6069 says "ignores between directives".
My intention was that the rule checks blank lines around a chunk of directives. 😉

[kaicataldo]

Ah, so it does...Missed that somehow. Okay, thanks! Will update accordingly 👍

[kaicataldo]

Still have to update the docs

[kaicataldo]

Updated with new docs. Please let me know what you think, @mysticatea. Thanks for helping with this!

[eslintbot]

LGTM

[mysticatea]

Wow, there are super many tests!
Looks great to me.

[mysticatea]

Personally, I prefer an empty array instead of null.

(currently, this returns an empty array if the function is empty?)

[kaicataldo]

Would your preference be for it to return an empty array for the early return as well? My goal was to make it consistent for when it doesn't find directives.

[mysticatea]

My preference is that the function always returns an array. We can check the existence of directive prologues by the length of the result (or by iteration of the result).

Also, current implementation seems inconsistent.

// In empty function case, returns `[]`
function foo() {
}

// In non-strict function case, returns `null`
function foo() {
    bar()
}

[kaicataldo]

It should return null in both cases (https://github.com/eslint/eslint/pull/6998/files#diff-6198b73cda8ab9ac49ed872ead7fb268R716), but I can definitely make it always return an array 👍

[eslintbot]

LGTM

[kaicataldo]

Updated once more

[mysticatea]

Great, thank you very much!
LGTM.

[nzakas]

I think we are doing lowercase first word now.

[nzakas]

Lgtm, just one tiny thing on the docs. Really nice job on this -- I especially love the intro on the docs. Very well-written. 💯

[eslintbot]

LGTM

[vitorbal]

Super awesome, @kaicataldo!! Great job! 🎉 🎉