Skip to content

Commit

Permalink
refactor(linter): use diagnostic codes in lint rules (#4349)
Browse files Browse the repository at this point in the history
> This PR is (unfortunately) quite large, but all changes are needed in tandem for this to work properly.

## What This PR Does

Updates the linter to populate diagnostics reported by rules with error codes statically derived from `RuleMeta` + `RuleEnum`.

Doing so required changing how we handle vitest rules. I know @mysterven was hoping to refactor that part of the code, and I think this approach is an improvement (but could probably be cleaned up further).

## Changes

### 1. Auto-Populate Error Codes
`LintContext` now sets an error code scope + error code number for diagnostics reported by lint rules. `LintContext` will not clobber existing codes set by rules, allowing for rule-specific override behavior (e.g. to use `eslint-plugin-react-hooks` as an error scope).

In order to accomplish this, I had to update every diagnostic factory for every rule. While doing this I found some incorrect error messages, or messages that could be easily improved. This is where a large majority of the snapshot diffs come from. Additionally, I was able to reduce string allocations from `format!` usages in diagnostic factories, especially within jest rules.

### 2. Framework and Library Detection
This PR adds `FrameworkFlags`, which specify what (if any) set of libraries and frameworks are being used by a project and/or file. They are passed in two ways:

1. `LintOptions` can specify a set of `framework_hints` that apply to the entire target codebase. Right now these are always empty, but I'm thinking in the future we could sniff `package.json`. It may be helpful for enabling/disabling default rules.
2. When `Linter` gets run on a file, framework information is sniffed from the `LintContext`. Right now, we are only checking for `vitest` imports in `ModuleRecord` and test path prefixes from `source_path`. It may be useful to do something similar for React/NextJS rules in the future. I know that [next/no-html-link-for-pages](https://nextjs.org/docs/messages/no-html-link-for-pages) could benefit greatly from this.
  • Loading branch information
DonIsaac committed Jul 20, 2024
1 parent 5f1e070 commit 7a75e0f
Show file tree
Hide file tree
Showing 424 changed files with 2,018 additions and 2,218 deletions.
17 changes: 15 additions & 2 deletions crates/oxc_diagnostics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ impl Diagnostic for OxcDiagnostic {
.map(Box::new)
.map(|b| b as Box<dyn Iterator<Item = LabeledSpan>>)
}

fn code<'a>(&'a self) -> Option<Box<dyn Display + 'a>> {
// self.code.is_some().then(|| Box::new(&self.code) as Box<dyn Display>)
None
}
}

impl OxcDiagnostic {
Expand Down Expand Up @@ -138,21 +143,29 @@ impl OxcDiagnostic {

#[inline]
pub fn with_error_code_scope<T: Into<Cow<'static, str>>>(mut self, code_scope: T) -> Self {
self.inner.code.scope = Some(code_scope.into());
self.inner.code.scope = match self.inner.code.scope {
Some(scope) => Some(scope),
None => Some(code_scope.into()),
};
debug_assert!(
self.inner.code.scope.as_ref().is_some_and(|s| !s.is_empty()),
"Error code scopes cannot be empty"
);

self
}

#[inline]
pub fn with_error_code_num<T: Into<Cow<'static, str>>>(mut self, code_num: T) -> Self {
self.inner.code.number = Some(code_num.into());
self.inner.code.number = match self.inner.code.number {
Some(num) => Some(num),
None => Some(code_num.into()),
};
debug_assert!(
self.inner.code.number.as_ref().is_some_and(|n| !n.is_empty()),
"Error code numbers cannot be empty"
);

self
}

Expand Down
9 changes: 2 additions & 7 deletions crates/oxc_language_server/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use oxc_linter::{
AstroPartialLoader, JavaScriptSource, SveltePartialLoader, VuePartialLoader,
LINT_PARTIAL_LOADER_EXT,
},
FixKind, LintContext, Linter,
FixKind, Linter,
};
use oxc_parser::Parser;
use oxc_semantic::SemanticBuilder;
Expand Down Expand Up @@ -304,12 +304,7 @@ impl IsolatedLintHandler {
return Some(Self::wrap_diagnostics(path, &original_source_text, reports, start));
};

let lint_ctx = LintContext::new(
path.to_path_buf().into_boxed_path(),
Rc::new(semantic_ret.semantic),
);

let result = linter.run(lint_ctx);
let result = linter.run(path, Rc::new(semantic_ret.semantic));

let reports = result
.into_iter()
Expand Down
64 changes: 59 additions & 5 deletions crates/oxc_linter/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ use crate::{
disable_directives::{DisableDirectives, DisableDirectivesBuilder},
fixer::{FixKind, Message, RuleFix, RuleFixer},
javascript_globals::GLOBALS,
AllowWarnDeny, OxlintConfig, OxlintEnv, OxlintGlobals, OxlintSettings,
AllowWarnDeny, FrameworkFlags, OxlintConfig, OxlintEnv, OxlintGlobals, OxlintSettings,
};

#[derive(Clone)]
#[must_use]
pub struct LintContext<'a> {
semantic: Rc<Semantic<'a>>,

Expand All @@ -38,6 +39,7 @@ pub struct LintContext<'a> {
eslint_config: Arc<OxlintConfig>,

// states
current_plugin_prefix: &'static str,
current_rule_name: &'static str,

/// Current rule severity. Allows for user severity overrides, e.g.
Expand All @@ -50,6 +52,7 @@ pub struct LintContext<'a> {
/// }
/// ```
severity: Severity,
frameworks: FrameworkFlags,
}

impl<'a> LintContext<'a> {
Expand All @@ -74,36 +77,51 @@ impl<'a> LintContext<'a> {
fix: FixKind::None,
file_path: file_path.into(),
eslint_config: Arc::new(OxlintConfig::default()),
current_plugin_prefix: "eslint",
current_rule_name: "",
severity: Severity::Warning,
frameworks: FrameworkFlags::empty(),
}
}

/// Enable/disable automatic code fixes.
#[must_use]
pub fn with_fix(mut self, fix: FixKind) -> Self {
self.fix = fix;
self
}

#[must_use]
pub fn with_eslint_config(mut self, eslint_config: &Arc<OxlintConfig>) -> Self {
self.eslint_config = Arc::clone(eslint_config);
self
}

#[must_use]
pub fn with_plugin_name(mut self, plugin: &'static str) -> Self {
self.current_plugin_prefix = plugin_name_to_prefix(plugin);
self
}

pub fn with_rule_name(mut self, name: &'static str) -> Self {
self.current_rule_name = name;
self
}

#[must_use]
pub fn with_severity(mut self, severity: AllowWarnDeny) -> Self {
self.severity = Severity::from(severity);
self
}

/// Set [`FrameworkFlags`], overwriting any existing flags.
pub fn with_frameworks(mut self, frameworks: FrameworkFlags) -> Self {
self.frameworks = frameworks;
self
}

/// Add additional [`FrameworkFlags`]
pub fn and_frameworks(mut self, frameworks: FrameworkFlags) -> Self {
self.frameworks |= frameworks;
self
}

pub fn semantic(&self) -> &Rc<Semantic<'a>> {
&self.semantic
}
Expand Down Expand Up @@ -182,6 +200,8 @@ impl<'a> LintContext<'a> {
fn add_diagnostic(&self, message: Message<'a>) {
if !self.disable_directives.contains(self.current_rule_name, message.span()) {
let mut message = message;
message.error =
message.error.with_error_code(self.current_plugin_prefix, self.current_rule_name);
if message.error.severity != self.severity {
message.error = message.error.with_severity(self.severity);
}
Expand Down Expand Up @@ -306,6 +326,10 @@ impl<'a> LintContext<'a> {
}
}

pub fn frameworks(&self) -> FrameworkFlags {
self.frameworks
}

/// AST nodes
///
/// Shorthand for `self.semantic().nodes()`.
Expand Down Expand Up @@ -340,4 +364,34 @@ impl<'a> LintContext<'a> {
pub fn jsdoc(&self) -> &JSDocFinder<'a> {
self.semantic().jsdoc()
}

// #[inline]
// fn plugin_name_to_prefix(&self, plugin_name: &'static str) -> &'static str {
// let plugin_name = if self. plugin_name == "jest" && self.frameworks.contains(FrameworkFlags::Vitest) {
// "vitest"
// } else {
// plugin_name
// };
// PLUGIN_PREFIXES.get(plugin_name).copied().unwrap_or(plugin_name)
// }
}

#[inline]
fn plugin_name_to_prefix(plugin_name: &'static str) -> &'static str {
PLUGIN_PREFIXES.get(plugin_name).copied().unwrap_or(plugin_name)
}

const PLUGIN_PREFIXES: phf::Map<&'static str, &'static str> = phf::phf_map! {
"import" => "eslint-plugin-import",
"jest" => "eslint-plugin-jest",
"jsdoc" => "eslint-plugin-jsdoc",
"jsx_a11y" => "eslint-plugin-jsx-a11y",
"nextjs" => "eslint-plugin-next",
"promise" => "eslint-plugin-promise",
"react_perf" => "eslint-plugin-react-perf",
"react" => "eslint-plugin-react",
"tree_shaking" => "eslint-plugin-tree-shaking",
"typescript" => "typescript-eslint",
"unicorn" => "eslint-plugin-unicorn",
"vitest" => "eslint-plugin-vitest",
};
85 changes: 85 additions & 0 deletions crates/oxc_linter/src/frameworks.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
use bitflags::bitflags;
use oxc_semantic::ModuleRecord;
use std::{hash, path::Path};

bitflags! {
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct FrameworkFlags: u32 {
// front-end frameworks

/// Uses [React](https://reactjs.org/).
///
/// May be part of a meta-framework like Next.js.
const React = 1 << 0;
/// Uses [Preact](https://preactjs.com/).
const Preact = 1 << 1;
/// Uses [Next.js](https://nextjs.org/).
const NextOnly = 1 << 2;
const Next = Self::NextOnly.bits() | Self::React.bits();
const JsxLike = Self::React.bits() | Self::Preact.bits() | Self::Next.bits();

const Vue = 1 << 3;
const NuxtOnly = 1 << 4;
const Nuxt = Self::NuxtOnly.bits() | Self::Vue.bits();

const Angular = 1 << 5;

const Svelte = 1 << 6;
const SvelteKitOnly = 1 << 7;
const SvelteKit = Self::SvelteKitOnly.bits() | Self::Svelte.bits();

const Astro = 1 << 8;

// Testing frameworks
const Jest = 1 << 9;
const Vitest = 1 << 10;
const OtherTest = 1 << 11;
const Test = Self::Jest.bits() | Self::Vitest.bits() | Self::OtherTest.bits();
}
}

impl Default for FrameworkFlags {
#[inline]
fn default() -> Self {
Self::empty()
}
}
impl hash::Hash for FrameworkFlags {
#[inline]
fn hash<H: hash::Hasher>(&self, state: &mut H) {
state.write_u32(self.bits());
}
}

impl FrameworkFlags {
#[inline]
pub const fn is_test(self) -> bool {
self.intersects(Self::Test)
}

#[inline]
pub const fn is_vitest(self) -> bool {
self.contains(Self::Vitest)
}
}

/// <https://jestjs.io/docs/configuration#testmatch-arraystring>
pub(crate) fn is_jestlike_file(path: &Path) -> bool {
use std::ffi::OsStr;

if path.components().any(|c| match c {
std::path::Component::Normal(p) => p == OsStr::new("__tests__"),
_ => false,
}) {
return true;
}

path.file_name() // foo/bar/baz.test.ts -> baz.test.ts
.and_then(OsStr::to_str)
.and_then(|filename| filename.split('.').rev().nth(1)) // baz.test.ts -> test
.is_some_and(|name_or_first_ext| name_or_first_ext == "test" || name_or_first_ext == "spec")
}

pub(crate) fn has_vitest_imports(module_record: &ModuleRecord) -> bool {
module_record.import_entries.iter().any(|entry| entry.module_request.name() == "vitest")
}
55 changes: 51 additions & 4 deletions crates/oxc_linter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ mod config;
mod context;
mod disable_directives;
mod fixer;
mod frameworks;
mod globals;
mod javascript_globals;
mod options;
Expand All @@ -19,15 +20,16 @@ mod utils;
pub mod partial_loader;
pub mod table;

use std::{io::Write, rc::Rc, sync::Arc};
use std::{io::Write, path::Path, rc::Rc, sync::Arc};

use oxc_diagnostics::Error;
use oxc_semantic::AstNode;
use oxc_semantic::{AstNode, Semantic};

pub use crate::{
config::OxlintConfig,
context::LintContext,
fixer::FixKind,
frameworks::FrameworkFlags,
options::{AllowWarnDeny, LintOptions},
rule::{RuleCategory, RuleMeta, RuleWithSeverity},
service::{LintService, LintServiceOptions},
Expand Down Expand Up @@ -109,15 +111,26 @@ impl Linter {
self.rules.len()
}

pub fn run<'a>(&self, ctx: LintContext<'a>) -> Vec<Message<'a>> {
// pub fn run<'a>(&self, ctx: LintContext<'a>) -> Vec<Message<'a>> {
pub fn run<'a>(&self, path: &Path, semantic: Rc<Semantic<'a>>) -> Vec<Message<'a>> {
let ctx = self.create_ctx(path, semantic);
let semantic = Rc::clone(ctx.semantic());

let ctx = ctx.with_fix(self.options.fix).with_eslint_config(&self.eslint_config);
let rules = self
.rules
.iter()
.map(|rule| {
(rule, ctx.clone().with_rule_name(rule.name()).with_severity(rule.severity))
let rule_name = rule.name();
let plugin_name = self.map_jest(rule.plugin_name(), rule_name);

(
rule,
ctx.clone()
.with_plugin_name(plugin_name)
.with_rule_name(rule_name)
.with_severity(rule.severity),
)
})
.collect::<Vec<_>>();

Expand Down Expand Up @@ -149,6 +162,40 @@ impl Linter {
writeln!(writer, "Default: {}", table.turned_on_by_default_count).unwrap();
writeln!(writer, "Total: {}", table.total).unwrap();
}

fn create_ctx<'a>(&self, path: &Path, semantic: Rc<Semantic<'a>>) -> LintContext<'a> {
let mut ctx = LintContext::new(path.to_path_buf().into_boxed_path(), semantic)
.with_fix(self.options.fix)
.with_eslint_config(&self.eslint_config)
.with_frameworks(self.options.framework_hints);

// set file-specific jest/vitest flags
if self.options.jest_plugin || self.options.vitest_plugin {
let mut test_flags = FrameworkFlags::empty();

if frameworks::is_jestlike_file(path) {
test_flags.set(FrameworkFlags::Jest, self.options.jest_plugin);
test_flags.set(FrameworkFlags::Vitest, self.options.vitest_plugin);
} else if frameworks::has_vitest_imports(ctx.module_record()) {
test_flags.set(FrameworkFlags::Vitest, true);
}

ctx = ctx.and_frameworks(test_flags);
}

ctx
}

fn map_jest(&self, plugin_name: &'static str, rule_name: &str) -> &'static str {
if self.options.vitest_plugin
&& plugin_name == "jest"
&& utils::is_jest_rule_adapted_to_vitest(rule_name)
{
"vitest"
} else {
plugin_name
}
}
}

#[cfg(test)]
Expand Down
Loading

0 comments on commit 7a75e0f

Please sign in to comment.