BREAKING: always enable ban-unknown-rule-code and ban-unused-ignore & add these docs (#769)

Author: magurotuna

examples/dlint/main.rs

@@ -189,9 +189,7 @@ fn run_linter(
 
       let mut linter_builder = LinterBuilder::default()
         .rules(rules)
-        .syntax(determine_syntax(file_path))
-        .lint_unknown_rules(true)
-        .lint_unused_ignore_directives(true);
+        .syntax(determine_syntax(file_path));
 
       for plugin_path in &plugin_paths {
         let js_runner = js::JsRuleRunner::new(plugin_path);

src/context.rs

@@ -150,6 +150,9 @@ impl<'view> Context<'view> {
     filtered
   }
 
+  /// Lint rule implementation for `ban-unused-ignore`.
+  /// This should be run after all normal rules have been finished because this
+  /// works for diagnostics reported by other rules.
   pub(crate) fn ban_unused_ignore(
     &self,
     specified_rules: &[Box<dyn LintRule>],
@@ -211,6 +214,10 @@ impl<'view> Context<'view> {
     diagnostics
   }
 
+  /// Lint rule implementation for `ban-unknown-rule-code`.
+  /// This should be run after all normal rules have been finished because
+  /// currently we collect the rule codes of plugins as they are run and thus
+  /// there's no way of knowing what are the "known" rule codes beforehand.
   pub(crate) fn ban_unknown_rule_code(&self) -> Vec<LintDiagnostic> {
     let builtin_all_rule_codes: HashSet<&'static str> =
       get_all_rules().into_iter().map(|r| r.code()).collect();

src/lib.rs

@@ -36,17 +36,8 @@ mod lint_tests {
   use swc_common::SourceMap;
   use swc_ecmascript::ast::Program;
 
-  fn lint(
-    source: &str,
-    unknown_rules: bool,
-    unused_dir: bool,
-    rules: Vec<Box<dyn LintRule>>,
-  ) -> Vec<LintDiagnostic> {
-    let linter = LinterBuilder::default()
-      .lint_unknown_rules(unknown_rules)
-      .lint_unused_ignore_directives(unused_dir)
-      .rules(rules)
-      .build();
+  fn lint(source: &str, rules: Vec<Box<dyn LintRule>>) -> Vec<LintDiagnostic> {
+    let linter = LinterBuilder::default().rules(rules).build();
 
     let (_, diagnostics) = linter
       .lint("lint_test.ts".to_string(), source.to_string())
@@ -59,15 +50,9 @@ mod lint_tests {
     comments: SingleThreadedComments,
     source_map: Rc<SourceMap>,
     tokens: Vec<TokenAndSpan>,
-    unknown_rules: bool,
-    unused_dir: bool,
     rules: Vec<Box<dyn LintRule>>,
   ) -> Vec<LintDiagnostic> {
-    let linter = LinterBuilder::default()
-      .lint_unknown_rules(unknown_rules)
-      .lint_unused_ignore_directives(unused_dir)
-      .rules(rules)
-      .build();
+    let linter = LinterBuilder::default().rules(rules).build();
 
     let (_, diagnostics) = linter
       .lint_with_ast(
@@ -81,78 +66,46 @@ mod lint_tests {
     diagnostics
   }
 
-  fn lint_recommended_rules(
-    source: &str,
-    unknown_rules: bool,
-    unused_dir: bool,
-  ) -> Vec<LintDiagnostic> {
-    lint(source, unknown_rules, unused_dir, get_recommended_rules())
+  fn lint_recommended_rules(source: &str) -> Vec<LintDiagnostic> {
+    lint(source, get_recommended_rules())
   }
 
   fn lint_recommended_rules_with_ast(
     ast: Program,
     comments: SingleThreadedComments,
     source_map: Rc<SourceMap>,
     tokens: Vec<TokenAndSpan>,
-    unknown_rules: bool,
-    unused_dir: bool,
   ) -> Vec<LintDiagnostic> {
-    lint_with_ast(
-      ast,
-      comments,
-      source_map,
-      tokens,
-      unknown_rules,
-      unused_dir,
-      get_recommended_rules(),
-    )
+    lint_with_ast(ast, comments, source_map, tokens, get_recommended_rules())
   }
 
   fn lint_specified_rule<T: LintRule + 'static>(
     source: &str,
-    unknown_rules: bool,
-    unused_dir: bool,
   ) -> Vec<LintDiagnostic> {
-    lint(source, unknown_rules, unused_dir, vec![T::new()])
+    lint(source, vec![T::new()])
   }
 
   #[test]
   fn empty_file() {
-    let diagnostics = lint_recommended_rules("", true, false);
+    let diagnostics = lint_recommended_rules("");
     assert!(diagnostics.is_empty());
   }
 
   #[test]
-  fn warn_unknown_rules() {
+  fn ban_unknown_rule_code() {
     let src = r#"
  // deno-lint-ignore some-rule
  function _foo() {
    // deno-lint-ignore some-rule-2 some-rule-3
    let _bar_foo = true
  }
       "#;
-    let diagnostics = lint_recommended_rules(src, true, false);
+    let diagnostics = lint_recommended_rules(src);
 
     assert_diagnostic(&diagnostics[0], "ban-unknown-rule-code", 2, 1, src);
     assert_diagnostic(&diagnostics[1], "ban-unknown-rule-code", 4, 3, src);
   }
 
-  #[test]
-  fn ignore_unknown_rules() {
-    let diagnostics = lint_recommended_rules(
-      r#"
- // deno-lint-ignore some-rule
- function _foo() {
-   // pass
- }
-      "#,
-      false,
-      false,
-    );
-
-    assert_eq!(diagnostics.len(), 0);
-  }
-
   #[test]
   fn unknown_rules_always_know_available_rules() {
     use crate::rules::camelcase::Camelcase;
@@ -161,15 +114,13 @@ mod lint_tests {
 // deno-lint-ignore no-explicit-any
 const fooBar: any = 42;
       "#,
-      true,
-      false,
     );
 
     assert!(diagnostics.is_empty());
   }
 
   #[test]
-  fn ban_unused_ignore_enabled() {
+  fn ban_unused_ignore() {
     let src = r#"
  // deno-lint-ignore no-explicit-any
  function _bar(_p: boolean) {
@@ -178,31 +129,13 @@ const fooBar: any = 42;
  }
       "#;
 
-    let diagnostics = lint_recommended_rules(
-      src, false, true, // enables `ban-unused-ignore`
-    );
+    let diagnostics = lint_recommended_rules(src);
 
     assert_eq!(diagnostics.len(), 2);
     assert_diagnostic(&diagnostics[0], "ban-unused-ignore", 2, 1, src);
     assert_diagnostic(&diagnostics[1], "ban-unused-ignore", 4, 3, src);
   }
 
-  #[test]
-  fn ban_unused_ignore_disabled() {
-    let diagnostics = lint_recommended_rules(
-      r#"
- // deno-lint-ignore no-explicit-any
- function _bar(_p: boolean) {
-   // pass
- }
-      "#,
-      false,
-      false, // disables `ban-unused-ignore`
-    );
-
-    assert_eq!(diagnostics.len(), 0);
-  }
-
   #[test]
   fn ban_unused_ignore_not_report_unexecuted_rule() {
     use crate::rules::camelcase::Camelcase;
@@ -211,8 +144,6 @@ const fooBar: any = 42;
 // deno-lint-ignore no-explicit-any
 const _fooBar = 42;
       "#,
-      false,
-      true, // enables `ban-unused-ignore`
     );
 
     assert!(diagnostics.is_empty());
@@ -227,8 +158,6 @@ const _fooBar = 42;
 // deno-lint-ignore no-explicit-any
 const _foo = 42;
       "#,
-      false,
-      true, // enables `ban-unused-ignore`
     );
 
     assert!(diagnostics.is_empty());
@@ -241,12 +170,15 @@ const _foo = 42;
 // deno-lint-ignore no-explicit-any ban-unused-ignore
 const _foo = 42;
       "#;
-    let diagnostics = lint_recommended_rules(
-      src, false, true, // enables `ban-unused-ignore`
-    );
+    let diagnostics = lint_recommended_rules(src);
 
-    assert_eq!(diagnostics.len(), 1);
+    assert_eq!(diagnostics.len(), 2);
+
+    // Both `no-explicit-any` and `ban-unused-ignore` are considered "unused"
+    // ignore directives in this case. Remember that `ban-unused-ignore`, if
+    // it's ignored at a line level, doesn't have any effect.
     assert_diagnostic(&diagnostics[0], "ban-unused-ignore", 2, 0, src);
+    assert_diagnostic(&diagnostics[1], "ban-unused-ignore", 2, 0, src);
   }
 
   #[test]
@@ -259,8 +191,6 @@ const _foo = 42;
    // pass
  }
       "#,
-      false,
-      false,
     );
 
     assert_eq!(diagnostics.len(), 0);
@@ -275,9 +205,7 @@ const _foo = 42;
    // pass
  }
       "#;
-    let diagnostics = lint_recommended_rules(
-      src, false, true, // enables `ban-unused-ignore`
-    );
+    let diagnostics = lint_recommended_rules(src);
 
     assert_eq!(diagnostics.len(), 1);
     assert_diagnostic(&diagnostics[0], "ban-unused-ignore", 2, 1, src);
@@ -293,7 +221,7 @@ const _foo = 42;
    // pass
  }
       "#;
-    let diagnostics = lint_recommended_rules(src, false, true);
+    let diagnostics = lint_recommended_rules(src);
 
     assert_eq!(diagnostics.len(), 1);
     assert_diagnostic(&diagnostics[0], "ban-unused-ignore", 4, 1, src);
@@ -302,9 +230,8 @@ const _foo = 42;
   #[test]
   fn empty_file_with_ast() {
     let (ast, comments, source_map, tokens) = parse("");
-    let diagnostics = lint_recommended_rules_with_ast(
-      ast, comments, source_map, tokens, true, false,
-    );
+    let diagnostics =
+      lint_recommended_rules_with_ast(ast, comments, source_map, tokens);
     assert!(diagnostics.is_empty());
   }
 }

src/linter.rs

@@ -25,8 +25,6 @@ pub use swc_common::SourceFile;
 pub struct LinterBuilder {
   ignore_file_directive: String,
   ignore_diagnostic_directive: String,
-  lint_unused_ignore_directives: bool,
-  lint_unknown_rules: bool,
   syntax: swc_ecmascript::parser::Syntax,
   rules: Vec<Box<dyn LintRule>>,
   plugins: Vec<Box<dyn Plugin>>,
@@ -37,8 +35,6 @@ impl LinterBuilder {
     Self {
       ignore_file_directive: "deno-lint-ignore-file".to_string(),
       ignore_diagnostic_directive: "deno-lint-ignore".to_string(),
-      lint_unused_ignore_directives: true,
-      lint_unknown_rules: true,
       syntax: get_default_ts_config(),
       rules: vec![],
       plugins: vec![],
@@ -49,8 +45,6 @@ impl LinterBuilder {
     Linter::new(
       self.ignore_file_directive,
       self.ignore_diagnostic_directive,
-      self.lint_unused_ignore_directives,
-      self.lint_unknown_rules,
       self.syntax,
       self.rules,
       self.plugins,
@@ -67,19 +61,6 @@ impl LinterBuilder {
     self
   }
 
-  pub fn lint_unused_ignore_directives(
-    mut self,
-    lint_unused_ignore_directives: bool,
-  ) -> Self {
-    self.lint_unused_ignore_directives = lint_unused_ignore_directives;
-    self
-  }
-
-  pub fn lint_unknown_rules(mut self, lint_unknown_rules: bool) -> Self {
-    self.lint_unknown_rules = lint_unknown_rules;
-    self
-  }
-
   pub fn syntax(mut self, syntax: Syntax) -> Self {
     self.syntax = syntax;
     self
@@ -100,8 +81,6 @@ pub struct Linter {
   ast_parser: AstParser,
   ignore_file_directive: String,
   ignore_diagnostic_directive: String,
-  lint_unused_ignore_directives: bool,
-  lint_unknown_rules: bool,
   syntax: Syntax,
   rules: Vec<Box<dyn LintRule>>,
   plugins: Vec<Box<dyn Plugin>>,
@@ -111,8 +90,6 @@ impl Linter {
   fn new(
     ignore_file_directive: String,
     ignore_diagnostic_directive: String,
-    lint_unused_ignore_directives: bool,
-    lint_unknown_rules: bool,
     syntax: Syntax,
     rules: Vec<Box<dyn LintRule>>,
     plugins: Vec<Box<dyn Plugin>>,
@@ -121,8 +98,6 @@ impl Linter {
       ast_parser: AstParser::new(),
       ignore_file_directive,
       ignore_diagnostic_directive,
-      lint_unused_ignore_directives,
-      lint_unknown_rules,
       syntax,
       rules,
       plugins,
@@ -200,12 +175,10 @@ impl Linter {
     let start = Instant::now();
 
     let mut filtered_diagnostics = context.check_ignore_directive_usage();
-    if self.lint_unused_ignore_directives {
-      filtered_diagnostics.extend(context.ban_unused_ignore(&self.rules));
-    }
-    if self.lint_unknown_rules {
-      filtered_diagnostics.extend(context.ban_unknown_rule_code());
-    }
+    // Run `ban-unused-ignore`
+    filtered_diagnostics.extend(context.ban_unused_ignore(&self.rules));
+    // Run `ban-unknown-rule-code`
+    filtered_diagnostics.extend(context.ban_unknown_rule_code());
     filtered_diagnostics.sort_by_key(|d| d.range.start.line);
 
     let end = Instant::now();

src/rules.rs

@@ -5,8 +5,10 @@ use dprint_swc_ecma_ast_view::Program as ProgramView;
 pub mod adjacent_overload_signatures;
 pub mod ban_ts_comment;
 pub mod ban_types;
+pub mod ban_unknown_rule_code;
 pub mod ban_untagged_ignore;
 pub mod ban_untagged_todo;
+pub mod ban_unused_ignore;
 pub mod camelcase;
 pub mod constructor_super;
 pub mod default_param_last;
@@ -144,8 +146,10 @@ pub fn get_all_rules() -> Vec<Box<dyn LintRule>> {
     adjacent_overload_signatures::AdjacentOverloadSignatures::new(),
     ban_ts_comment::BanTsComment::new(),
     ban_types::BanTypes::new(),
+    ban_unknown_rule_code::BanUnknownRuleCode::new(),
     ban_untagged_ignore::BanUntaggedIgnore::new(),
     ban_untagged_todo::BanUntaggedTodo::new(),
+    ban_unused_ignore::BanUnusedIgnore::new(),
     camelcase::Camelcase::new(),
     constructor_super::ConstructorSuper::new(),
     default_param_last::DefaultParamLast::new(),