deno_lint icon indicating copy to clipboard operation
deno_lint copied to clipboard

Ignore directives should affect the entire next AST node

Open nayeemrmn opened this issue 3 years ago • 7 comments

As in, the following should be valid:

// deno-lint-ignore no-explicit-any
{
  let a: any;
  let b: any;
  let c: any;
}

This is taken from Rust, #[rustfmt::skip] and #[allow(warnings)] work this way. // deno-fmt-ignore already works this way also. It's much a much nicer way of handling ignore ranges than // deno-lint-ignore-start etc.

nayeemrmn avatar Oct 29 '20 14:10 nayeemrmn

@nayeemrmn very sensible feature, I'm in favor of supporting it. Implementing it will require some refactor to ignore_directive.rs and filtering diagnostics, but both of them were ripe for a cleanup.

bartlomieju avatar Nov 02 '20 21:11 bartlomieju

@nayeemrmn I gave this a bit more thought and I'm not so sure anymore - ignoring diagnostic in whole AST means that all diagnostics in children of the node would be ignored as well.

Example:

// deno-lint-ignore no-explicit-any
async function foo(): any {
   
  {
    let a: any;
  }

  ...
}

In above example ignore directive above function would also ignore diagnostics inside block. In real world code this might lead to ignoring very deeply placed diagnostics.

Did I get the idea wrong? Could you elaborate a bit more on the use case?

bartlomieju avatar Nov 08 '20 13:11 bartlomieju

I considered that, I presume the user would have to do this if they really only wanted to capture the return type:

async function foo(): /* deno-lint-ignore no-explicit-any */ any {
   
  {
    let a: any; // Fails.
  }

  ...
}

If that's unacceptable, I don't mind making exceptions for functions and similar, or even if this proposal only applied to basic {} blocks.

nayeemrmn avatar Nov 08 '20 13:11 nayeemrmn

I don't like inline block comments, but I think applying to block nodes makes sense.

bartlomieju avatar Nov 08 '20 13:11 bartlomieju

In my opinion this should only be done if the comment is above a statement or module item (this is actually how ignore directives work in rslint 👀). Anything else will give you really weird unpredictable scoping. Then after that you can add deno-lint-ignore-next-line to be more explicit if somebody wants to ignore a whole line. You should reject any directives in that format which are not above a statement

RDambrosio016 avatar Nov 08 '20 13:11 RDambrosio016

@RDambrosio016 deno-lint-ignore currently works only on next line. I agree with you on the scoping problem (hence my above comment) but I see value in @nayeemrmn's proposal and I think it does make sense for blocks but only blocks. I really don't want to add more ignore directives, I find it super confusing to have 4 different ignore directives (like eslint does).

bartlomieju avatar Nov 08 '20 14:11 bartlomieju

I think the intent of the user is important.

I mean, if ecmascript syntax forces the user to place a node on the specific position, like the return type in the code below,

function foo(
    a: string
): string {
    return a;
}

the ignore directive should go before the ast node, like

// Here
function foo(
    a: string
): string {
    return a;
}

At the same time, I think directives for parameter should be placed before the line because user can change order of parameters.

function foo(
    // Here
    a: string
): string {
    return a;
}

The same rule can be used for block statements. Block statement is intentional. I mean, the ecmascript syntax does not force the user to create a block statement in a function body.


function foo() {
    // deno-lint-ignore no-explicit-any
    {
        let a: any;
        let b: any;
        let c: any;
    }
}

The block statement in the function is intentionally created, while the block statement of the function is forced syntax. So I think the directive should work like


// Does not affect function body.
function foo() {
    // Affect the block statement below.
    {
        let a: any;
        let b: any;
        let c: any;
    }
}

But sometimes a user may want to ignore the node. I think another directive like deno-lint-ignore-all can be used to support the case.

Alternatively, I think using deno-lint-ignore-all for intentional block statements is also fine.

kdy1 avatar Mar 04 '21 07:03 kdy1