From 0d707093f4df26fc4f6179cb7ac821dca180bdb9 Mon Sep 17 00:00:00 2001 From: Dmitry Zakharov Date: Sat, 16 Nov 2024 20:48:47 +0300 Subject: [PATCH] feat(linter): add `typescript/no-require-imports` rule --- crates/oxc_linter/src/rules.rs | 2 + .../rules/typescript/no_require_imports.rs | 393 ++++++++++++++++++ .../src/snapshots/no_require_imports.snap | 217 ++++++++++ 3 files changed, 612 insertions(+) create mode 100644 crates/oxc_linter/src/rules/typescript/no_require_imports.rs create mode 100644 crates/oxc_linter/src/snapshots/no_require_imports.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 8714025070a8a2..5b6be90dcac68c 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -170,6 +170,7 @@ mod typescript { pub mod no_non_null_asserted_nullish_coalescing; pub mod no_non_null_asserted_optional_chain; pub mod no_non_null_assertion; + pub mod no_require_imports; pub mod no_this_alias; pub mod no_unnecessary_type_constraint; pub mod no_unsafe_declaration_merging; @@ -852,6 +853,7 @@ oxc_macros::declare_all_lint_rules! { typescript::no_non_null_asserted_nullish_coalescing, typescript::no_non_null_asserted_optional_chain, typescript::no_non_null_assertion, + typescript::no_require_imports, typescript::no_this_alias, typescript::no_unnecessary_type_constraint, typescript::no_unsafe_declaration_merging, diff --git a/crates/oxc_linter/src/rules/typescript/no_require_imports.rs b/crates/oxc_linter/src/rules/typescript/no_require_imports.rs new file mode 100644 index 00000000000000..4325d7794a053c --- /dev/null +++ b/crates/oxc_linter/src/rules/typescript/no_require_imports.rs @@ -0,0 +1,393 @@ +use oxc_ast::{ + ast::{Argument, TSModuleReference}, + AstKind, +}; +use regex::Regex; + +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{CompactStr, Span}; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn no_require_imports_diagnostic(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Expected \"import\" statement instead of \"require\" call") + .with_help("Do not use CommonJS `require` calls") + .with_label(span) +} + +#[derive(Debug, Default, Clone)] +pub struct NoRequireImports(Box); + +#[derive(Debug, Default, Clone)] +pub struct NoRequireImportsConfig { + allow: Vec, + allow_as_import: bool, +} + +impl std::ops::Deref for NoRequireImports { + type Target = NoRequireImportsConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Forbids the use of CommonJS `require` calls. + /// + /// ### Why is this bad? + /// + /// `require` imports, while functional in Node.js and older JavaScript environments, are generally + /// considered less desirable than ES modules (`import`) for several key reasons in modern JavaScript development: + /// + /// 1. **Static vs. Dynamic**: `require`` is a __runtime__ function. It executes when the code runs, which means errors related to missing modules or incorrect paths are only discovered during runtime. ES modules (`import`) are static imports. Their resolution and potential errors are checked during the compilation or bundling process, making them easier to catch during development. + /// + /// 2. **Code Organization and Readability**: `require` statements are scattered throughout the code, potentially making it harder to quickly identify the dependencies of a given module. `import` statements are typically grouped at the top of a file, improving code organization and readability. + /// + /// 3. **Tree Shaking and Optimization**: Modern bundlers like Webpack and Rollup use tree-shaking to remove unused code from the final bundle. Tree-shaking works significantly better with ES modules because their dependencies are declared statically and explicitly. `require` makes it harder for bundlers to accurately identify and remove unused code, resulting in larger bundle sizes and slower load times. + /// + /// 4. **Cyclic Dependencies**: Handling cyclic dependencies (where module A imports B, and B imports A) is significantly more challenging with `require``. ES modules, through their declarative nature and the use of dynamic imports (`import()`), provide better mechanisms to handle cyclic imports and manage asynchronous loading. + /// + /// 5. **Maintainability and Refactoring**: Changing module names or paths is simpler with ES modules because the changes are declared directly and the compiler or bundler catches any errors. With `require`, you might have to track down all instances of a specific `require` statement for a particular module, making refactoring more error-prone. + /// + /// 6. Modern JavaScript Standards: import is the standard way to import modules in modern JavaScript, aligned with current best practices and language specifications. Using require necessitates additional build steps or tools to translate it to a format that the browser or modern JavaScript environments can understand. + /// + /// 7. Error Handling: ES modules provide a more structured way to handle errors during module loading using `try...catch` blocks with dynamic imports, enhancing error management. `require` errors can be less predictable. + /// + /// In summary, while `require`` works, the benefits of ES modules in terms of static analysis, better bundling, improved code organization, and easier maintainability make it the preferred method for importing modules in modern JavaScript projects. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```ts + /// const lib1 = require('lib1'); + /// const { lib2 } = require('lib2'); + /// import lib3 = require('lib3'); + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```ts + /// import * as lib1 from 'lib1'; + /// import { lib2 } from 'lib2'; + /// import * as lib3 from 'lib3'; + /// ``` + /// + /// ### Options + /// + /// #### `allow` + /// + /// array of strings + /// + /// These strings will be compiled into regular expressions with the u flag and be used to test against the imported path. + /// A common use case is to allow importing `package.json`. This is because `package.json` commonly lives outside of the TS root directory, + /// so statically importing it would lead to root directory conflicts, especially with `resolveJsonModule` enabled. + /// You can also use it to allow importing any JSON if your environment doesn't support JSON modules, or use it for other cases where `import` statements cannot work. + /// + /// With { allow: ['/package\\.json$'] }: + /// + /// Examples of **correct** code for this rule: + /// ```ts + /// console.log(require('../package.json').version); + /// ``` + /// + /// #### `allowAsImport` + /// + /// When set to `true`, `import ... = require(...)` declarations won't be reported. + /// This is useful if you use certain module options that require strict CommonJS interop semantics. + /// + /// With `{ allowAsImport: true }`: + /// + /// Examples of **incorrect** code for this rule: + /// ```ts + /// var foo = require('foo'); + /// const foo = require('foo'); + /// let foo = require('foo'); + /// ``` + /// Examples of **correct** code for this rule: + /// ```ts + /// import foo = require('foo'); + /// import foo from 'foo'; + /// ``` + /// + NoRequireImports, + restriction, + pending // TODO: fixer (change require to import) +); + +fn match_argument_value_with_regex(allow: &[CompactStr], argument_value: &str) -> bool { + return allow + .iter() + .map(|pattern| Regex::new(pattern).unwrap()) + .any(|regex| regex.is_match(argument_value)); +} + +impl Rule for NoRequireImports { + fn from_configuration(value: serde_json::Value) -> Self { + let obj = value.get(0); + Self(Box::new(NoRequireImportsConfig { + allow: obj + .and_then(|v| v.get("allow")) + .and_then(serde_json::Value::as_array) + .map(|v| { + v.iter().filter_map(serde_json::Value::as_str).map(CompactStr::from).collect() + }) + .unwrap_or_default(), + allow_as_import: obj + .and_then(|v| v.get("allowAsImport")) + .and_then(serde_json::Value::as_bool) + .unwrap_or(false), + })) + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + match node.kind() { + AstKind::CallExpression(call_expr) => { + if node.scope_id() != ctx.scopes().root_scope_id() { + return; + } + + if !call_expr.is_require_call() { + return; + } + + if !self.allow.is_empty() { + let Some(argument) = call_expr.arguments.first() else { + return; + }; + + match argument { + Argument::TemplateLiteral(template_literal) => { + let Some(quasi) = template_literal.quasis.first() else { + return; + }; + + if match_argument_value_with_regex(&self.allow, &quasi.value.raw) { + return; + } + + ctx.diagnostic(no_require_imports_diagnostic(quasi.span)); + } + Argument::StringLiteral(string_literal) => { + if match_argument_value_with_regex(&self.allow, &string_literal.value) { + return; + } + + ctx.diagnostic(no_require_imports_diagnostic(string_literal.span)); + } + _ => {} + } + } + + if ctx.scopes().find_binding(ctx.scopes().root_scope_id(), "require").is_some() { + return; + } + + ctx.diagnostic(no_require_imports_diagnostic(call_expr.span)); + } + AstKind::TSImportEqualsDeclaration(decl) => match decl.module_reference { + TSModuleReference::ExternalModuleReference(ref mod_ref) => { + if self.allow_as_import { + return; + } + + if !self.allow.is_empty() { + if match_argument_value_with_regex(&self.allow, &mod_ref.expression.value) { + return; + } + + ctx.diagnostic(no_require_imports_diagnostic(mod_ref.span)); + } + + ctx.diagnostic(no_require_imports_diagnostic(decl.span)); + } + TSModuleReference::IdentifierReference(_) | TSModuleReference::QualifiedName(_) => { + } + }, + _ => {} + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ("import { l } from 'lib';", None), + ("var lib3 = load('not_an_import');", None), + ("var lib4 = lib2.subImport;", None), + ("var lib7 = 700;", None), + ("import lib9 = lib2.anotherSubImport;", None), + ("import lib10 from 'lib10';", None), + ("var lib3 = load?.('not_an_import');", None), + ( + " + import { createRequire } from 'module'; + const require = createRequire(); + require('remark-preset-prettier'); + ", + None, + ), + ( + "const pkg = require('./package.json');", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ( + "const pkg = require('../package.json');", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ( + "const pkg = require(`./package.json`);", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ( + "const pkg = require('../packages/package.json');", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ( + "import pkg = require('../packages/package.json');", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ( + "import pkg = require('data.json');", + Some(serde_json::json!([{ "allow": ["\\.json$"] }])), + ), + ( + "import pkg = require('some-package');", + Some(serde_json::json!([{ "allow": ["^some-package$"] }])), + ), + ("import foo = require('foo');", Some(serde_json::json!([{ "allowAsImport": true }]))), + ( + " + let require = bazz; + trick(require('foo')); + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + let require = bazz; + const foo = require('./foo.json') as Foo; + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + let require = bazz; + const foo: Foo = require('./foo.json').default; + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + let require = bazz; + const foo = require('./foo.json'); + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + let require = bazz; + const configValidator = new Validator(require('./a.json')); + configValidator.addSchema(require('./a.json')); + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + let require = bazz; + require('foo'); + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + let require = bazz; + require?.('foo'); + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + import { createRequire } from 'module'; + const require = createRequire(); + require('remark-preset-prettier'); + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ]; + + let fail = vec![ + ("var lib = require('lib');", None), + ("let lib2 = require('lib2');", None), + ( + " + var lib5 = require('lib5'), + lib6 = require('lib6'); + ", + None, + ), + ("import lib8 = require('lib8');", None), + ("var lib = require?.('lib');", None), + ("let lib2 = require?.('lib2');", None), + ( + " + var lib5 = require?.('lib5'), + lib6 = require?.('lib6'); + ", + None, + ), + ("const pkg = require('./package.json');", None), + ( + "const pkg = require('./package.jsonc');", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ( + "const pkg = require(`./package.jsonc`);", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ("import pkg = require('./package.json');", None), + ( + "import pkg = require('./package.jsonc');", + Some(serde_json::json!([{ "allow": ["/package\\.json$"] }])), + ), + ( + "import pkg = require('./package.json');", + Some(serde_json::json!([{ "allow": ["^some-package$"] }])), + ), + ("var foo = require?.('foo');", Some(serde_json::json!([{ "allowAsImport": true }]))), + ( + "let foo = trick(require?.('foo'));", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ("trick(require('foo'));", Some(serde_json::json!([{ "allowAsImport": true }]))), + ( + "const foo = require('./foo.json') as Foo;", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + "const foo: Foo = require('./foo.json').default;", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + "const foo = require('./foo.json');", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ( + " + const configValidator = new Validator(require('./a.json')); + configValidator.addSchema(require('./a.json')); + ", + Some(serde_json::json!([{ "allowAsImport": true }])), + ), + ("require('foo');", Some(serde_json::json!([{ "allowAsImport": true }]))), + ("require?.('foo');", Some(serde_json::json!([{ "allowAsImport": true }]))), + ]; + + Tester::new(NoRequireImports::NAME, pass, fail) + .change_rule_path_extension("ts") + .test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_require_imports.snap b/crates/oxc_linter/src/snapshots/no_require_imports.snap new file mode 100644 index 00000000000000..312ea8f58b9ce4 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_require_imports.snap @@ -0,0 +1,217 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:11] + 1 │ var lib = require('lib'); + · ────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:12] + 1 │ let lib2 = require('lib2'); + · ─────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:2:15] + 1 │ + 2 │ var lib5 = require('lib5'), + · ─────────────── + 3 │ lib6 = require('lib6'); + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:3:13] + 2 │ var lib5 = require('lib5'), + 3 │ lib6 = require('lib6'); + · ─────────────── + 4 │ + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:1] + 1 │ import lib8 = require('lib8'); + · ────────────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:11] + 1 │ var lib = require?.('lib'); + · ──────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:12] + 1 │ let lib2 = require?.('lib2'); + · ───────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:2:15] + 1 │ + 2 │ var lib5 = require?.('lib5'), + · ───────────────── + 3 │ lib6 = require?.('lib6'); + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:3:13] + 2 │ var lib5 = require?.('lib5'), + 3 │ lib6 = require?.('lib6'); + · ───────────────── + 4 │ + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:13] + 1 │ const pkg = require('./package.json'); + · ───────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:21] + 1 │ const pkg = require('./package.jsonc'); + · ───────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:13] + 1 │ const pkg = require('./package.jsonc'); + · ────────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:22] + 1 │ const pkg = require(`./package.jsonc`); + · ─────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:13] + 1 │ const pkg = require(`./package.jsonc`); + · ────────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:1] + 1 │ import pkg = require('./package.json'); + · ─────────────────────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:14] + 1 │ import pkg = require('./package.jsonc'); + · ────────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:1] + 1 │ import pkg = require('./package.jsonc'); + · ──────────────────────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:14] + 1 │ import pkg = require('./package.json'); + · ───────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:1] + 1 │ import pkg = require('./package.json'); + · ─────────────────────────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:11] + 1 │ var foo = require?.('foo'); + · ──────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:17] + 1 │ let foo = trick(require?.('foo')); + · ──────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:7] + 1 │ trick(require('foo')); + · ────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:13] + 1 │ const foo = require('./foo.json') as Foo; + · ───────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:18] + 1 │ const foo: Foo = require('./foo.json').default; + · ───────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:18] + 1 │ const foo = require('./foo.json'); + · ───────────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:2:42] + 1 │ + 2 │ const configValidator = new Validator(require('./a.json')); + · ─────────────────── + 3 │ configValidator.addSchema(require('./a.json')); + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:3:30] + 2 │ const configValidator = new Validator(require('./a.json')); + 3 │ configValidator.addSchema(require('./a.json')); + · ─────────────────── + 4 │ + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:1] + 1 │ require('foo'); + · ────────────── + ╰──── + help: Do not use CommonJS `require` calls + + ⚠ typescript-eslint(no-require-imports): Expected "import" statement instead of "require" call + ╭─[no_require_imports.ts:1:1] + 1 │ require?.('foo'); + · ──────────────── + ╰──── + help: Do not use CommonJS `require` calls