Skip to content

Commit

Permalink
refactor(linter): consolidate file loading logic (#6130)
Browse files Browse the repository at this point in the history
# Human Description
Low on time, so this one is short.
- consolidate source file and partial loader logic into `loader` module. I have more plans for this.
- ~LSP no longer uses `VALID_EXTENSIONS`, so now `.d.ts` files (and the like) will be linted as well~ LSP does not respect `.gitignore` files, so this change was reverted.

# AI Description
## Refactor Loader and Partial Loader

This PR refactors the loader and partial loader functionality in the oxc_linter crate:

* Introduce a new `Loader` struct with methods for checking if a file can be loaded and loading file contents
* Move `partial_loader` module to `loader/partial_loader`
* Rename `JavaScriptSource` to `source.rs` and move it to the `loader` module
* Update `JavaScriptSource` to use `u32` for `start` offset instead of `usize`
* Refactor `IsolatedLintHandler` to use the new `Loader`
* Update imports and module references throughout the codebase

This change improves the organization of the loader-related code and provides a more unified interface for loading different file types.
  • Loading branch information
DonIsaac committed Sep 29, 2024
1 parent 183739f commit ea908f7
Show file tree
Hide file tree
Showing 13 changed files with 219 additions and 108 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions apps/oxlint/src/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use std::{env, io::BufWriter, time::Instant};
use ignore::gitignore::Gitignore;
use oxc_diagnostics::{DiagnosticService, GraphicalReportHandler};
use oxc_linter::{
partial_loader::LINT_PARTIAL_LOADER_EXT, AllowWarnDeny, InvalidFilterKind, LintFilter,
LintService, LintServiceOptions, Linter, OxlintOptions,
loader::LINT_PARTIAL_LOADER_EXT, AllowWarnDeny, InvalidFilterKind, LintFilter, LintService,
LintServiceOptions, Linter, OxlintOptions,
};
use oxc_span::VALID_EXTENSIONS;

Expand Down
14 changes: 8 additions & 6 deletions crates/oxc_language_server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,21 @@ test = false
doctest = false

[dependencies]
dashmap = { workspace = true }
env_logger = { workspace = true, features = ["humantime"] }
futures = { workspace = true }
globset = { workspace = true }
ignore = { workspace = true, features = ["simd-accel"] }
log = { workspace = true }
oxc_allocator = { workspace = true }
oxc_diagnostics = { workspace = true }
oxc_linter = { workspace = true }
oxc_parser = { workspace = true }
oxc_semantic = { workspace = true }
oxc_span = { workspace = true }

dashmap = { workspace = true }
env_logger = { workspace = true, features = ["humantime"] }
futures = { workspace = true }
globset = { workspace = true }
ignore = { workspace = true, features = ["simd-accel"] }
log = { workspace = true }
ropey = { workspace = true }
rustc-hash = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }
tokio = { workspace = true, features = ["full"] }
Expand Down
112 changes: 43 additions & 69 deletions crates/oxc_language_server/src/linter.rs
Original file line number Diff line number Diff line change
@@ -1,24 +1,23 @@
use oxc_linter::loader::LINT_PARTIAL_LOADER_EXT;
use std::{
fs,
path::{Path, PathBuf},
rc::Rc,
sync::Arc,
sync::{Arc, OnceLock},
};

use log::debug;
use oxc_allocator::Allocator;
use oxc_diagnostics::{Error, NamedSource, Severity};
use oxc_linter::{
partial_loader::{
AstroPartialLoader, JavaScriptSource, SveltePartialLoader, VuePartialLoader,
LINT_PARTIAL_LOADER_EXT,
},
loader::{JavaScriptSource, Loader},
FixKind, Linter,
};
use oxc_parser::{ParseOptions, Parser};
use oxc_semantic::SemanticBuilder;
use oxc_span::{SourceType, VALID_EXTENSIONS};
use oxc_span::VALID_EXTENSIONS;
use ropey::Rope;
use rustc_hash::FxHashSet;
use tower_lsp::lsp_types::{
self, DiagnosticRelatedInformation, DiagnosticSeverity, Position, Range, Url,
};
Expand Down Expand Up @@ -154,20 +153,21 @@ pub struct FixedContent {

pub struct IsolatedLintHandler {
linter: Arc<Linter>,
loader: Loader,
}

impl IsolatedLintHandler {
pub fn new(linter: Arc<Linter>) -> Self {
Self { linter }
Self { linter, loader: Loader }
}

pub fn run_single(
&self,
path: &Path,
content: Option<String>,
) -> Option<Vec<DiagnosticReport>> {
if Self::is_wanted_ext(path) {
Some(Self::lint_path(&self.linter, path, content).map_or(vec![], |(p, errors)| {
if Self::should_lint_path(path) {
Some(self.lint_path(path, content).map_or(vec![], |(p, errors)| {
let mut diagnostics: Vec<DiagnosticReport> =
errors.into_iter().map(|e| e.into_diagnostic_report(&p)).collect();
// a diagnostics connected from related_info to original diagnostic
Expand Down Expand Up @@ -212,62 +212,33 @@ impl IsolatedLintHandler {
}
}

fn is_wanted_ext(path: &Path) -> bool {
let extensions = get_valid_extensions();
path.extension().map_or(false, |ext| extensions.contains(&ext.to_string_lossy().as_ref()))
}

fn get_source_type_and_text(
fn lint_path(
&self,
path: &Path,
source_text: Option<String>,
ext: &str,
) -> Option<(SourceType, String)> {
let source_type = SourceType::from_path(path);
let not_supported_yet =
source_type.as_ref().is_err_and(|_| !LINT_PARTIAL_LOADER_EXT.contains(&ext));
if not_supported_yet {
debug!("extension {ext} not supported yet.");
) -> Option<(PathBuf, Vec<ErrorWithPosition>)> {
if !Loader::can_load(path) {
debug!("extension not supported yet.");
return None;
}
let source_type = source_type.unwrap_or_default();
let source_text = source_text.map_or_else(
|| fs::read_to_string(path).unwrap_or_else(|_| panic!("Failed to read {path:?}")),
|source_text| source_text,
);

Some((source_type, source_text))
}

fn may_need_extract_js_content<'a>(
source_text: &'a str,
ext: &str,
) -> Option<Vec<JavaScriptSource<'a>>> {
match ext {
"vue" => Some(VuePartialLoader::new(source_text).parse()),
"astro" => Some(AstroPartialLoader::new(source_text).parse()),
"svelte" => Some(SveltePartialLoader::new(source_text).parse()),
_ => None,
}
}

fn lint_path(
linter: &Linter,
path: &Path,
source_text: Option<String>,
) -> Option<(PathBuf, Vec<ErrorWithPosition>)> {
let ext = path.extension().and_then(std::ffi::OsStr::to_str)?;
let (source_type, original_source_text) =
Self::get_source_type_and_text(path, source_text, ext)?;
let javascript_sources = Self::may_need_extract_js_content(&original_source_text, ext)
.unwrap_or_else(|| {
vec![JavaScriptSource { source_text: &original_source_text, source_type, start: 0 }]
});
let javascript_sources = match self.loader.load_str(path, &source_text) {
Ok(s) => s,
Err(e) => {
debug!("failed to load {path:?}: {e}");
return None;
}
};

debug!("lint {path:?}");
let mut diagnostics = vec![];
for source in javascript_sources {
let JavaScriptSource { source_text: javascript_source_text, source_type, start } =
source;
let JavaScriptSource {
source_text: javascript_source_text, source_type, start, ..
} = source;
let allocator = Allocator::default();
let ret = Parser::new(&allocator, javascript_source_text, source_type)
.with_options(ParseOptions {
Expand All @@ -285,7 +256,7 @@ impl IsolatedLintHandler {
fixed_content: None,
})
.collect();
return Some(Self::wrap_diagnostics(path, &original_source_text, reports, start));
return Some(Self::wrap_diagnostics(path, &source_text, reports, start));
};

let program = allocator.alloc(ret.program);
Expand All @@ -304,10 +275,10 @@ impl IsolatedLintHandler {
fixed_content: None,
})
.collect();
return Some(Self::wrap_diagnostics(path, &original_source_text, reports, start));
return Some(Self::wrap_diagnostics(path, &source_text, reports, start));
};

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

let reports = result
.into_iter()
Expand All @@ -316,12 +287,12 @@ impl IsolatedLintHandler {
code: f.content.to_string(),
range: Range {
start: offset_to_position(
f.span.start as usize + start,
(f.span.start + start) as usize,
javascript_source_text,
)
.unwrap_or_default(),
end: offset_to_position(
f.span.end as usize + start,
(f.span.end + start) as usize,
javascript_source_text,
)
.unwrap_or_default(),
Expand All @@ -332,18 +303,29 @@ impl IsolatedLintHandler {
})
.collect::<Vec<ErrorReport>>();
let (_, errors_with_position) =
Self::wrap_diagnostics(path, &original_source_text, reports, start);
Self::wrap_diagnostics(path, &source_text, reports, start);
diagnostics.extend(errors_with_position);
}

Some((path.to_path_buf(), diagnostics))
}

fn should_lint_path(path: &Path) -> bool {
static WANTED_EXTENSIONS: OnceLock<FxHashSet<&'static str>> = OnceLock::new();
let wanted_exts = WANTED_EXTENSIONS.get_or_init(|| {
VALID_EXTENSIONS.iter().chain(LINT_PARTIAL_LOADER_EXT.iter()).copied().collect()
});

path.extension()
.and_then(std::ffi::OsStr::to_str)
.map_or(false, |ext| wanted_exts.contains(ext))
}

fn wrap_diagnostics(
path: &Path,
source_text: &str,
reports: Vec<ErrorReport>,
start: usize,
start: u32,
) -> (PathBuf, Vec<ErrorWithPosition>) {
let source = Arc::new(NamedSource::new(path.to_string_lossy(), source_text.to_owned()));
let diagnostics = reports
Expand All @@ -353,22 +335,14 @@ impl IsolatedLintHandler {
report.error.with_source_code(Arc::clone(&source)),
source_text,
report.fixed_content,
start,
start as usize,
)
})
.collect();
(path.to_path_buf(), diagnostics)
}
}

fn get_valid_extensions() -> Vec<&'static str> {
VALID_EXTENSIONS
.iter()
.chain(LINT_PARTIAL_LOADER_EXT.iter())
.copied()
.collect::<Vec<&'static str>>()
}

#[allow(clippy::cast_possible_truncation)]
fn offset_to_position(offset: usize, source_text: &str) -> Option<Position> {
let rope = Rope::from_str(source_text);
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ mod rules;
mod service;
mod utils;

pub mod partial_loader;
pub mod loader;
pub mod table;

use std::{io::Write, path::Path, rc::Rc, sync::Arc};
Expand Down
107 changes: 107 additions & 0 deletions crates/oxc_linter/src/loader/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
mod partial_loader;
mod source;

use std::{error::Error, fmt, path::Path};

use oxc_span::SourceType;

pub use partial_loader::{PartialLoader, LINT_PARTIAL_LOADER_EXT};
pub use source::JavaScriptSource;

// TODO: use oxc_resolver::FileSystem. We can't do so until that crate exposes FileSystemOs
// externally.
#[derive(Default, Clone)]
pub struct Loader;

impl Loader {
pub fn can_load<P: AsRef<Path>>(path: P) -> bool {
let path = path.as_ref();
SourceType::from_path(path).is_ok()
|| path
.extension()
.and_then(std::ffi::OsStr::to_str)
.is_some_and(|ext| LINT_PARTIAL_LOADER_EXT.contains(&ext))
}

/// # Errors
/// - If the file is too large (> 4GB, or u32::MAX)
/// - If the file has no extension
/// - If the file extension is not supported
pub fn load_str<'a, P: AsRef<Path>>(
&self,
path: P,
source_text: &'a str,
) -> Result<Vec<JavaScriptSource<'a>>, LoadError> {
if source_text.len() > u32::MAX as usize {
return Err(LoadError::TooLarge);
}

let path = path.as_ref();
let ext = path.extension().ok_or(LoadError::NoExtension)?;
// file extension is not unicode, we definitely don't support it.
let ext = ext.to_str().ok_or_else(|| LoadError::unsupported(ext))?;

// let source_type = SourceType::from_path(path);
if let Ok(source_type) = SourceType::from_path(path) {
Ok(vec![JavaScriptSource::new(source_text, source_type)])
} else {
let partial = PartialLoader::parse(ext, source_text);
partial.ok_or_else(|| LoadError::UnsupportedFileType(ext.to_string()))
}
}
}

#[derive(Debug, Clone)]
pub enum LoadError {
TooLarge,
NoExtension,
UnsupportedFileType(String),
}

impl LoadError {
pub(super) fn unsupported(ext: &std::ffi::OsStr) -> Self {
Self::UnsupportedFileType(ext.to_string_lossy().to_string())
}
}

impl fmt::Display for LoadError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::TooLarge => write!(f, "file is too large. Only files up to 4GB are supported."),
Self::NoExtension => write!(f, "no extension"),
Self::UnsupportedFileType(ext) => write!(f, "unsupported file type: {ext}"),
}
}
}

impl Error for LoadError {}

#[cfg(test)]
mod test {
use super::*;

#[test]
fn test_loader_can_handle() {
let paths = [
"foo.js",
"foo.jsx",
"foo.mjs",
"foo.cjs",
"foo.ts",
"foo.tsx",
"foo.mts",
"foo.cts",
"foo.d.ts",
"foo.d.tsx",
"foo.d.mts",
"foo.d.cts",
"foo.astro",
"foo.svelte",
"foo.vue",
];

for path in paths {
assert!(Loader::can_load(path));
}
}
}
Loading

0 comments on commit ea908f7

Please sign in to comment.