Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linter): eslint-plugin-unicorn/no-abusive-eslint-disable #1125

Merged
merged 5 commits into from
Nov 12, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/oxc_linter/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ impl<'a> LintContext<'a> {
&self.semantic
}

pub fn disable_directives(&self) -> &DisableDirectives<'a> {
&self.disable_directives
}

pub fn source_text(&self) -> &'a str {
self.semantic().source_text()
}
Expand Down
44 changes: 43 additions & 1 deletion crates/oxc_linter/src/disable_directives.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,21 @@ enum DisabledRule<'a> {
Single(&'a str),
}

/// A comment which disables one or more specific rules
pub struct DisableRuleComment<'a> {
/// Span of the comment
pub span: Span,
/// Rules disabled by the comment
pub rules: Vec<&'a str>,
}

pub struct DisableDirectives<'a> {
/// All the disabled rules with their corresponding covering spans
intervals: Lapper<u32, DisabledRule<'a>>,
/// Spans of comments that disable all rules
disable_all_comments: Vec<Span>,
/// All comments that disable one or more specific rules
disable_rule_comments: Vec<DisableRuleComment<'a>>,
}

impl<'a> DisableDirectives<'a> {
Expand All @@ -24,6 +36,14 @@ impl<'a> DisableDirectives<'a> {
|| matches!(interval.val, DisabledRule::Single(name) if name.contains(rule_name))
})
}

pub fn disable_all_comments(&self) -> &Vec<Span> {
&self.disable_all_comments
}

pub fn disable_rule_comments(&self) -> &Vec<DisableRuleComment<'a>> {
&self.disable_rule_comments
}
}

pub struct DisableDirectivesBuilder<'a, 'b> {
Expand All @@ -35,6 +55,10 @@ pub struct DisableDirectivesBuilder<'a, 'b> {
disable_all_start: Option<u32>,
/// Start of `eslint-disable rule_name`
disable_start_map: FxHashMap<&'a str, u32>,
/// Spans of comments that disable all rules
disable_all_comments: Vec<Span>,
/// All comments that disable one or more specific rules
disable_rule_comments: Vec<DisableRuleComment<'a>>,
}

impl<'a, 'b> DisableDirectivesBuilder<'a, 'b> {
Expand All @@ -45,12 +69,18 @@ impl<'a, 'b> DisableDirectivesBuilder<'a, 'b> {
intervals: Lapper::new(vec![]),
disable_all_start: None,
disable_start_map: FxHashMap::default(),
disable_all_comments: vec![],
disable_rule_comments: vec![],
}
}

pub fn build(mut self) -> DisableDirectives<'a> {
self.build_impl();
DisableDirectives { intervals: self.intervals }
DisableDirectives {
intervals: self.intervals,
disable_all_comments: self.disable_all_comments,
disable_rule_comments: self.disable_rule_comments,
}
}

fn add_interval(&mut self, start: u32, stop: u32, val: DisabledRule<'a>) {
Expand All @@ -74,6 +104,7 @@ impl<'a, 'b> DisableDirectivesBuilder<'a, 'b> {
if self.disable_all_start.is_none() {
self.disable_all_start = Some(span.end);
}
self.disable_all_comments.push(span);
continue;
}

Expand All @@ -86,11 +117,15 @@ impl<'a, 'b> DisableDirectivesBuilder<'a, 'b> {
.fold(span.end, |acc, line| acc + line.len() as u32);
if text.trim().is_empty() {
self.add_interval(span.end, stop, DisabledRule::All);
self.disable_all_comments.push(span);
} else {
// `eslint-disable-next-line rule_name1, rule_name2`
let mut rules = vec![];
Self::get_rule_names(text, |rule_name| {
self.add_interval(span.end, stop, DisabledRule::Single(rule_name));
rules.push(rule_name);
});
self.disable_rule_comments.push(DisableRuleComment { span, rules });
}
continue;
}
Expand All @@ -107,19 +142,26 @@ impl<'a, 'b> DisableDirectivesBuilder<'a, 'b> {
// `eslint-disable-line`
if text.trim().is_empty() {
self.add_interval(start, stop, DisabledRule::All);
self.disable_all_comments.push(span);
} else {
// `eslint-disable-line rule-name1, rule-name2`
let mut rules = vec![];
Self::get_rule_names(text, |rule_name| {
self.add_interval(start, stop, DisabledRule::Single(rule_name));
rules.push(rule_name);
});
self.disable_rule_comments.push(DisableRuleComment { span, rules });
}
continue;
}

// `eslint-disable rule-name1, rule-name2`
let mut rules = vec![];
Self::get_rule_names(text, |rule_name| {
self.disable_start_map.entry(rule_name).or_insert(span.end);
rules.push(rule_name);
});
self.disable_rule_comments.push(DisableRuleComment { span, rules });

continue;
}
Expand Down
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ mod unicorn {
pub mod error_message;
pub mod filename_case;
pub mod new_for_builtins;
pub mod no_abusive_eslint_disable;
pub mod no_console_spaces;
pub mod no_empty_file;
pub mod no_instanceof_array;
Expand Down Expand Up @@ -286,6 +287,7 @@ oxc_macros::declare_all_lint_rules! {
unicorn::error_message,
unicorn::filename_case,
unicorn::new_for_builtins,
unicorn::no_abusive_eslint_disable,
unicorn::no_console_spaces,
unicorn::no_empty_file,
unicorn::no_instanceof_array,
Expand Down
139 changes: 139 additions & 0 deletions crates/oxc_linter/src/rules/unicorn/no_abusive_eslint_disable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
use lazy_static::lazy_static;
use oxc_diagnostics::{
miette::{self, Diagnostic},
thiserror::Error,
};
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use regex::Regex;

use crate::{context::LintContext, disable_directives::DisableRuleComment, rule::Rule};

#[derive(Debug, Error, Diagnostic)]
#[error("eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.")]
#[diagnostic(severity(warning), help("Specify the rules you want to disable."))]
struct NoAbusiveEslintDisableDiagnostic(#[label] pub Span);

#[derive(Debug, Default, Clone)]
pub struct NoAbusiveEslintDisable;

declare_oxc_lint!(
/// ### What it does
/// This rule disallows `eslint-disable` comments that do not specify any rules to disable.
///
/// ### Why is this bad?
/// When only one rule should be disabled but the `eslint-disable` comment does not specify any rules, other useful errors will also be silently ignored.
///
/// ### Example
/// ```javascript
/// // Fail
/// /* eslint-disable */
/// console.log(message);
///
/// console.log(message); // eslint-disable-line
///
/// // eslint-disable-next-line
/// console.log(message);
///
/// // Pass
/// /* eslint-disable no-console */
/// console.log(message);
///
/// console.log(message); // eslint-disable-line no-console
///
/// // eslint-disable-next-line no-console
/// console.log(message);
/// ```
NoAbusiveEslintDisable,
restriction
);

impl Rule for NoAbusiveEslintDisable {
fn run_once(&self, ctx: &LintContext) {
lazy_static! {
static ref RULE_PATTERN: Regex =
Regex::new("^(?:@[0-9A-Za-z_-]+/)?(?:[0-9A-Za-z_-]+/)?[0-9A-Za-z_-]+$").unwrap();
Boshen marked this conversation as resolved.
Show resolved Hide resolved
}

for span in ctx.disable_directives().disable_all_comments() {
ctx.diagnostic(NoAbusiveEslintDisableDiagnostic(*span));
}

for DisableRuleComment { span, rules } in ctx.disable_directives().disable_rule_comments() {
if rules.is_empty() || !RULE_PATTERN.is_match(rules[0]) {
ctx.diagnostic(NoAbusiveEslintDisableDiagnostic(*span));
}
}
}
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
"eval();",
"eval(); // eslint-disable-line no-eval",
"eval(); // eslint-disable-line no-eval, no-console",
"eval(); //eslint-disable-line no-eval",
"eval(); // eslint-disable-line no-eval",
"eval(); // eslint-disable-line no-eval",
"eval(); /* eslint-disable-line no-eval */",
"eval(); // eslint-disable-line plugin/rule",
"eval(); // eslint-disable-line @scope/plugin/rule-name",
"eval(); // eslint-disable-line no-eval, @scope/plugin/rule-name",
"eval(); // eslint-disable-line @scope/rule-name",
"eval(); // eslint-disable-line no-eval, @scope/rule-name",
"eval(); // eslint-line-disable",
"eval(); // some comment",
"/* eslint-disable no-eval */",
r#"
/* eslint-disable no-abusive-eslint-disable */
eval(); // eslint-disable-line
"#,
r#"
foo();
// eslint-disable-line no-eval
eval();
"#,
r#"
foo();
/* eslint-disable no-eval */
eval();
"#,
r#"
foo();
/* eslint-disable-next-line no-eval */
eval();
"#,
];

let fail = vec![
r#"
// eslint-disable-next-line @scopewithoutplugin
eval();
"#,
"eval(); // eslint-disable-line",
r#"
foo();
eval(); // eslint-disable-line
"#,
"/* eslint-disable */",
r#"
foo();
/* eslint-disable */
eval();
"#,
r#"
foo();
/* eslint-disable-next-line */
eval();
"#,
r#"
// eslint-disable-next-line
eval();
"#,
];

Tester::new_without_config(NoAbusiveEslintDisable::NAME, pass, fail).test_and_snapshot();
}
67 changes: 67 additions & 0 deletions crates/oxc_linter/src/snapshots/no_abusive_eslint_disable.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
---
source: crates/oxc_linter/src/tester.rs
assertion_line: 105
expression: no_abusive_eslint_disable
---
⚠ eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.
╭─[no_abusive_eslint_disable.tsx:1:1]
1 │
2 │ // eslint-disable-next-line @scopewithoutplugin
· ──────────────────────────────────────────────
3 │ eval();
4 │
╰────
help: Specify the rules you want to disable.

⚠ eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.
╭─[no_abusive_eslint_disable.tsx:1:1]
1 │ eval(); // eslint-disable-line
· ────────────────────
╰────
help: Specify the rules you want to disable.

⚠ eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.
╭─[no_abusive_eslint_disable.tsx:2:1]
2 │ foo();
3 │ eval(); // eslint-disable-line
· ─────────────────────
4 │
╰────
help: Specify the rules you want to disable.

⚠ eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.
╭─[no_abusive_eslint_disable.tsx:1:1]
1 │ /* eslint-disable */
· ────────────────
╰────
help: Specify the rules you want to disable.

⚠ eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.
╭─[no_abusive_eslint_disable.tsx:2:1]
2 │ foo();
3 │ /* eslint-disable */
· ────────────────
4 │ eval();
╰────
help: Specify the rules you want to disable.

⚠ eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.
╭─[no_abusive_eslint_disable.tsx:2:1]
2 │ foo();
3 │ /* eslint-disable-next-line */
· ──────────────────────────
4 │ eval();
╰────
help: Specify the rules you want to disable.

⚠ eslint-plugin-unicorn(no-abusive-eslint-disable): Unexpected `eslint-disable` comment that does not specify any rules to disable.
╭─[no_abusive_eslint_disable.tsx:1:1]
1 │
2 │ // eslint-disable-next-line
· ──────────────────────────
3 │ eval();
4 │
╰────
help: Specify the rules you want to disable.


Loading