Skip to content

feat(ignore-directives): support block-scoped deno-lint-ignore - #1518

Open
bartlomieju wants to merge 1 commit into
mainfrom
block-scoped-ignore-directives
Open

feat(ignore-directives): support block-scoped deno-lint-ignore#1518
bartlomieju wants to merge 1 commit into
mainfrom
block-scoped-ignore-directives

Conversation

@bartlomieju

Copy link
Copy Markdown
Member

A // deno-lint-ignore directive only suppressed diagnostics on the
single line that follows it, so silencing a rule across a multi-line
span meant repeating the directive on every line or reaching for the
much coarser deno-lint-ignore-file.

A directive placed directly above a { ... } block now suppresses
diagnostics anywhere inside that block. Coverage is intentionally
limited to bare, explicit blocks: the directive has to be a leading
comment of a BlockStmt. A directive placed before a function, class
or other statement attaches to that node rather than to a block, so it
keeps the ordinary next-line behavior and does not silently swallow
deeply nested diagnostics, which was the main concern raised when this
was discussed.

Closes #476

A `// deno-lint-ignore` directive only suppressed diagnostics on the
single following line, so there was no way to silence a rule across a
multi-line span without repeating the directive or reaching for the
coarse `deno-lint-ignore-file`.

A directive placed directly above a `{ ... }` block now suppresses
diagnostics anywhere inside that block. Coverage is intentionally
limited to bare/explicit blocks: the directive must be a leading comment
of a `BlockStmt`. A directive before a function, class or other
statement attaches to that node rather than to a block, so it keeps the
ordinary next-line behavior and does not silently swallow deeply nested
diagnostics.

Closes #476
@bartlomieju

Copy link
Copy Markdown
Member Author

Verdict: LGTM — correct and well-tested, including the key safety guard. Recommend merge (ignore-directive behavior change → maintainer sign-off; needs green CI / rebase).

Correctness — verified the main risk

  • The over-suppression concern (a directive before if (x) {/function f() { swallowing the whole body) is handled correctly and tested: because comments attach to the nearest following token, leading_comments(block_range.start) only returns a directive that sits directly above a bare {. The test function foo(): any { let a: any } yields block_end_line() == None, and the behavioral test confirms debugger inside a function body is still reported. So block scope only applies to explicit bare blocks — exactly the intent.
  • The containment check (block_start_line <= diagnostic_line <= block_end_line + check_used) is right, and it runs only after the ordinary next-line path, so existing behavior is unchanged when no bare block is involved.
  • extend_block_ignore_directives is correctly gated on !directives.is_empty(), so files without directives pay nothing.

Performance — minor

  • The block-directive check in the diagnostic filter is an O(block-directives) linear scan per diagnostic, i.e. O(diagnostics x block-directives) worst case. Negligible for normal files (few directives), but if you expect files with many ignore directives + many diagnostics, consider indexing block directives by line range. Not a blocker.
  • extend_block_ignore_directives walks the full AST once per file-with-directives — acceptable as a one-time per-file cost.

Suggestion

  • Add an if/for/while body case to the "blocks only" tests (function is covered; the attachment mechanism is the same, but explicit coverage would lock in that if (x) { bodies aren't block-scoped).

Housekeeping: mergeable: UNKNOWN → rebase. Behavior change to ignore-directives → your call to sign off (your own PR).

No changes requested. Well-scoped feature with the right guardrails and tests; good to merge once CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ignore directives should affect the entire next AST node

1 participant