fix(lsp): move sloppy import resolution from loader to resolver (#23751)

Moves sloppy import resolution from the loader to the resolver.

Also adds some test helper functions to make the lsp tests less verbose

---------

Co-authored-by: David Sherret <dsherret@gmail.com>
This commit is contained in:
Nathan Whitaker 2024-05-09 07:17:31 -07:00 committed by GitHub
parent 263b6b971d
commit dc29986ae5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 283 additions and 287 deletions

View File

@ -13,9 +13,7 @@ use super::tsc::AssetDocument;
use crate::cache::HttpCache;
use crate::graph_util::CliJsrUrlProvider;
use crate::lsp::logging::lsp_warn;
use crate::resolver::SloppyImportsFsEntry;
use crate::resolver::SloppyImportsResolution;
use crate::resolver::SloppyImportsResolver;
use deno_graph::source::Resolver;
use deno_runtime::fs_util::specifier_to_file_path;
use dashmap::DashMap;
@ -390,7 +388,7 @@ impl Document {
d.with_new_resolver(
s,
&CliJsrUrlProvider,
Some(graph_resolver),
Some(&graph_resolver),
Some(npm_resolver),
),
)
@ -400,7 +398,7 @@ impl Document {
maybe_types_dependency = self.maybe_types_dependency.as_ref().map(|d| {
Arc::new(d.with_new_resolver(
&CliJsrUrlProvider,
Some(graph_resolver),
Some(&graph_resolver),
Some(npm_resolver),
))
});
@ -854,8 +852,6 @@ pub struct Documents {
/// Gets if any document had a node: specifier such that a @types/node package
/// should be injected.
has_injected_types_node_package: bool,
/// If --unstable-sloppy-imports is enabled.
unstable_sloppy_imports: bool,
}
impl Documents {
@ -869,7 +865,6 @@ impl Documents {
resolver: Default::default(),
npm_specifier_reqs: Default::default(),
has_injected_types_node_package: false,
unstable_sloppy_imports: false,
}
}
@ -996,54 +991,17 @@ impl Documents {
&self,
specifier: &ModuleSpecifier,
) -> Option<ModuleSpecifier> {
if self.unstable_sloppy_imports && specifier.scheme() == "file" {
Some(
self
.resolve_unstable_sloppy_import(specifier)
.into_specifier()
.into_owned(),
)
let specifier = if let Ok(jsr_req_ref) =
JsrPackageReqReference::from_specifier(specifier)
{
Cow::Owned(self.resolver.jsr_to_registry_url(&jsr_req_ref)?)
} else {
let specifier = if let Ok(jsr_req_ref) =
JsrPackageReqReference::from_specifier(specifier)
{
Cow::Owned(self.resolver.jsr_to_registry_url(&jsr_req_ref)?)
} else {
Cow::Borrowed(specifier)
};
if !DOCUMENT_SCHEMES.contains(&specifier.scheme()) {
return None;
}
self.resolver.resolve_redirects(&specifier)
Cow::Borrowed(specifier)
};
if !DOCUMENT_SCHEMES.contains(&specifier.scheme()) {
return None;
}
}
fn resolve_unstable_sloppy_import<'a>(
&self,
specifier: &'a ModuleSpecifier,
) -> SloppyImportsResolution<'a> {
SloppyImportsResolver::resolve_with_stat_sync(
specifier,
ResolutionMode::Types,
|path| {
if let Ok(specifier) = ModuleSpecifier::from_file_path(path) {
if self.open_docs.contains_key(&specifier)
|| self.cache.contains(&specifier)
{
return Some(SloppyImportsFsEntry::File);
}
}
path.metadata().ok().and_then(|m| {
if m.is_file() {
Some(SloppyImportsFsEntry::File)
} else if m.is_dir() {
Some(SloppyImportsFsEntry::Dir)
} else {
None
}
})
},
)
self.resolver.resolve_redirects(&specifier)
}
/// Return `true` if the specifier can be resolved to a document.
@ -1226,12 +1184,7 @@ impl Documents {
) {
self.config = Arc::new(config.clone());
self.cache = cache;
let config_data = config.tree.root_data();
let config_file = config_data.and_then(|d| d.config_file.as_deref());
self.resolver = resolver.clone();
self.unstable_sloppy_imports = config_file
.map(|c| c.has_unstable("sloppy-imports"))
.unwrap_or(false);
{
let fs_docs = &self.file_system_docs;
// Clean up non-existent documents.
@ -1404,7 +1357,6 @@ fn node_resolve_npm_req_ref(
pub struct OpenDocumentsGraphLoader<'a> {
pub inner_loader: &'a mut dyn deno_graph::source::Loader,
pub open_docs: &'a HashMap<ModuleSpecifier, Arc<Document>>,
pub unstable_sloppy_imports: bool,
}
impl<'a> OpenDocumentsGraphLoader<'a> {
@ -1426,32 +1378,6 @@ impl<'a> OpenDocumentsGraphLoader<'a> {
}
None
}
fn resolve_unstable_sloppy_import<'b>(
&self,
specifier: &'b ModuleSpecifier,
) -> SloppyImportsResolution<'b> {
SloppyImportsResolver::resolve_with_stat_sync(
specifier,
ResolutionMode::Types,
|path| {
if let Ok(specifier) = ModuleSpecifier::from_file_path(path) {
if self.open_docs.contains_key(&specifier) {
return Some(SloppyImportsFsEntry::File);
}
}
path.metadata().ok().and_then(|m| {
if m.is_file() {
Some(SloppyImportsFsEntry::File)
} else if m.is_dir() {
Some(SloppyImportsFsEntry::Dir)
} else {
None
}
})
},
)
}
}
impl<'a> deno_graph::source::Loader for OpenDocumentsGraphLoader<'a> {
@ -1460,17 +1386,9 @@ impl<'a> deno_graph::source::Loader for OpenDocumentsGraphLoader<'a> {
specifier: &ModuleSpecifier,
options: deno_graph::source::LoadOptions,
) -> deno_graph::source::LoadFuture {
let specifier = if self.unstable_sloppy_imports {
self
.resolve_unstable_sloppy_import(specifier)
.into_specifier()
} else {
Cow::Borrowed(specifier)
};
match self.load_from_docs(&specifier) {
match self.load_from_docs(specifier) {
Some(fut) => fut,
None => self.inner_loader.load(&specifier, options),
None => self.inner_loader.load(specifier, options),
}
}
@ -1531,7 +1449,7 @@ fn analyze_module(
// dynamic imports like import(`./dir/${something}`) in the LSP
file_system: &deno_graph::source::NullFileSystem,
jsr_url_provider: &CliJsrUrlProvider,
maybe_resolver: Some(resolver.as_graph_resolver()),
maybe_resolver: Some(&resolver.as_graph_resolver()),
maybe_npm_resolver: Some(resolver.as_graph_npm_resolver()),
},
)),

View File

@ -251,7 +251,6 @@ impl LanguageServer {
let mut loader = crate::lsp::documents::OpenDocumentsGraphLoader {
inner_loader: &mut inner_loader,
open_docs: &open_docs,
unstable_sloppy_imports: cli_options.unstable_sloppy_imports(),
};
let graph = module_graph_creator
.create_graph_with_loader(GraphKind::All, roots.clone(), &mut loader)

View File

@ -21,6 +21,8 @@ use crate::npm::ManagedCliNpmResolver;
use crate::resolver::CliGraphResolver;
use crate::resolver::CliGraphResolverOptions;
use crate::resolver::CliNodeResolver;
use crate::resolver::SloppyImportsFsEntry;
use crate::resolver::SloppyImportsResolver;
use crate::util::progress_bar::ProgressBar;
use crate::util::progress_bar::ProgressBarStyle;
use deno_cache_dir::HttpCache;
@ -60,6 +62,7 @@ pub struct LspResolver {
redirect_resolver: Option<Arc<RedirectResolver>>,
graph_imports: Arc<IndexMap<ModuleSpecifier, GraphImport>>,
config: Arc<Config>,
unstable_sloppy_imports: bool,
}
impl Default for LspResolver {
@ -73,6 +76,7 @@ impl Default for LspResolver {
redirect_resolver: None,
graph_imports: Default::default(),
config: Default::default(),
unstable_sloppy_imports: false,
}
}
}
@ -140,6 +144,10 @@ impl LspResolver {
npm_config_hash,
redirect_resolver,
graph_imports,
unstable_sloppy_imports: config_data
.and_then(|d| d.config_file.as_ref())
.map(|cf| cf.has_unstable("sloppy-imports"))
.unwrap_or(false),
config: Arc::new(config.clone()),
})
}
@ -162,6 +170,7 @@ impl LspResolver {
redirect_resolver: self.redirect_resolver.clone(),
graph_imports: self.graph_imports.clone(),
config: self.config.clone(),
unstable_sloppy_imports: self.unstable_sloppy_imports,
})
}
@ -181,8 +190,11 @@ impl LspResolver {
Ok(())
}
pub fn as_graph_resolver(&self) -> &dyn Resolver {
self.graph_resolver.as_ref()
pub fn as_graph_resolver(&self) -> LspGraphResolver {
LspGraphResolver {
inner: &self.graph_resolver,
unstable_sloppy_imports: self.unstable_sloppy_imports,
}
}
pub fn as_graph_npm_resolver(&self) -> &dyn NpmResolver {
@ -307,6 +319,68 @@ impl LspResolver {
}
}
#[derive(Debug)]
pub struct LspGraphResolver<'a> {
inner: &'a CliGraphResolver,
unstable_sloppy_imports: bool,
}
impl<'a> Resolver for LspGraphResolver<'a> {
fn default_jsx_import_source(&self) -> Option<String> {
self.inner.default_jsx_import_source()
}
fn default_jsx_import_source_types(&self) -> Option<String> {
self.inner.default_jsx_import_source_types()
}
fn jsx_import_source_module(&self) -> &str {
self.inner.jsx_import_source_module()
}
fn resolve(
&self,
specifier_text: &str,
referrer_range: &deno_graph::Range,
mode: deno_graph::source::ResolutionMode,
) -> Result<deno_ast::ModuleSpecifier, deno_graph::source::ResolveError> {
let specifier = self.inner.resolve(specifier_text, referrer_range, mode)?;
if self.unstable_sloppy_imports && specifier.scheme() == "file" {
Ok(
SloppyImportsResolver::resolve_with_stat_sync(
&specifier,
mode,
|path| {
path.metadata().ok().and_then(|m| {
if m.is_file() {
Some(SloppyImportsFsEntry::File)
} else if m.is_dir() {
Some(SloppyImportsFsEntry::Dir)
} else {
None
}
})
},
)
.into_specifier()
.into_owned(),
)
} else {
Ok(specifier)
}
}
fn resolve_types(
&self,
specifier: &deno_ast::ModuleSpecifier,
) -> Result<
Option<(deno_ast::ModuleSpecifier, Option<deno_graph::Range>)>,
deno_graph::source::ResolveError,
> {
self.inner.resolve_types(specifier)
}
}
async fn create_npm_resolver(
config_data: &ConfigData,
global_cache_path: Option<&Path>,
@ -399,9 +473,7 @@ fn create_graph_resolver(
bare_node_builtins_enabled: config_file
.map(|cf| cf.has_unstable("bare-node-builtins"))
.unwrap_or(false),
// Don't set this for the LSP because instead we'll use the OpenDocumentsLoader
// because it's much easier and we get diagnostics/quick fixes about a redirected
// specifier for free.
// not used in the LSP as the LspGraphResolver handles this
sloppy_imports_resolver: None,
}))
}

View File

@ -12,70 +12,13 @@ use test_util::assert_starts_with;
use test_util::assertions::assert_json_subset;
use test_util::deno_cmd_with_deno_dir;
use test_util::env_vars_for_npm_tests;
use test_util::lsp::range_of;
use test_util::lsp::source_file;
use test_util::lsp::LspClient;
use test_util::testdata_path;
use test_util::TestContextBuilder;
use tower_lsp::lsp_types as lsp;
/// Helper to get the `lsp::Range` of the `n`th occurrence of
/// `text` in `src`. `n` is zero-based, like most indexes.
fn range_of_nth(
n: usize,
text: impl AsRef<str>,
src: impl AsRef<str>,
) -> lsp::Range {
let text = text.as_ref();
let src = src.as_ref();
let start = src
.match_indices(text)
.nth(n)
.map(|(i, _)| i)
.unwrap_or_else(|| panic!("couldn't find text {text} in source {src}"));
let end = start + text.len();
let mut line = 0;
let mut col = 0;
let mut byte_idx = 0;
let pos = |line, col| lsp::Position {
line,
character: col,
};
let mut start_pos = None;
let mut end_pos = None;
for c in src.chars() {
if byte_idx == start {
start_pos = Some(pos(line, col));
}
if byte_idx == end {
end_pos = Some(pos(line, col));
break;
}
if c == '\n' {
line += 1;
col = 0;
} else {
col += c.len_utf16() as u32;
}
byte_idx += c.len_utf8();
}
if start_pos.is_some() && end_pos.is_none() {
// range extends to end of string
end_pos = Some(pos(line, col));
}
let (start, end) = (start_pos.unwrap(), end_pos.unwrap());
lsp::Range { start, end }
}
/// Helper to get the `lsp::Range` of the first occurrence of
/// `text` in `src`. Equivalent to `range_of_nth(0, text, src)`.
fn range_of(text: impl AsRef<str>, src: impl AsRef<str>) -> lsp::Range {
range_of_nth(0, text, src)
}
#[test]
fn lsp_startup_shutdown() {
let context = TestContextBuilder::new().use_temp_cwd().build();
@ -12106,15 +12049,19 @@ fn lsp_deno_future_env_byonm() {
}
#[test]
fn lsp_sloppy_imports_warn() {
fn lsp_sloppy_imports() {
let context = TestContextBuilder::new().use_temp_cwd().build();
let temp_dir = context.temp_dir();
let temp_dir = temp_dir.path();
temp_dir
.join("deno.json")
.write(r#"{ "unstable": ["sloppy-imports"] }"#);
// should work when exists on the fs and when doesn't
// for sloppy imports, the file must exist on the file system
// to be resolved correctly
temp_dir.join("a.ts").write("export class A {}");
temp_dir.join("b.ts").write("export class B {}");
temp_dir.join("c.js").write("export class C {}");
temp_dir.join("c.d.ts").write("export class C {}");
let mut client = context.new_lsp_command().build();
client.initialize(|builder| {
builder.set_root_uri(temp_dir.uri_dir());
@ -12161,137 +12108,67 @@ fn lsp_sloppy_imports_warn() {
),
},
}));
assert_eq!(
diagnostics.messages_with_source("deno"),
lsp::PublishDiagnosticsParams {
uri: temp_dir.join("file.ts").uri_file(),
diagnostics: vec![
lsp::Diagnostic {
range: lsp::Range {
start: lsp::Position {
line: 0,
character: 19
},
end: lsp::Position {
line: 0,
character: 24
}
},
severity: Some(lsp::DiagnosticSeverity::INFORMATION),
code: Some(lsp::NumberOrString::String("redirect".to_string())),
source: Some("deno".to_string()),
message: format!(
"The import of \"{}\" was redirected to \"{}\".",
temp_dir.join("a").uri_file(),
temp_dir.join("a.ts").uri_file()
),
data: Some(json!({
"specifier": temp_dir.join("a").uri_file(),
"redirect": temp_dir.join("a.ts").uri_file()
})),
..Default::default()
},
lsp::Diagnostic {
range: lsp::Range {
start: lsp::Position {
line: 1,
character: 19
},
end: lsp::Position {
line: 1,
character: 27
}
},
severity: Some(lsp::DiagnosticSeverity::INFORMATION),
code: Some(lsp::NumberOrString::String("redirect".to_string())),
source: Some("deno".to_string()),
message: format!(
"The import of \"{}\" was redirected to \"{}\".",
temp_dir.join("b.js").uri_file(),
temp_dir.join("b.ts").uri_file()
),
data: Some(json!({
"specifier": temp_dir.join("b.js").uri_file(),
"redirect": temp_dir.join("b.ts").uri_file()
})),
..Default::default()
}
],
version: Some(1),
}
);
let res = client.write_request(
"textDocument/codeAction",
assert_eq!(json!(diagnostics.all()), json!([]));
client.shutdown();
}
#[test]
fn lsp_sloppy_imports_prefers_dts() {
let context = TestContextBuilder::new().use_temp_cwd().build();
let temp_dir = context.temp_dir();
let temp_dir = temp_dir.path();
temp_dir
.join("deno.json")
.write(r#"{ "unstable": ["sloppy-imports"] }"#);
let mut client: LspClient = context
.new_lsp_command()
.set_root_dir(temp_dir.clone())
.build();
client.initialize_default();
temp_dir.join("a.js").write("export const foo: number;");
let a_dts = source_file(temp_dir.join("a.d.ts"), "export const foo = 3;");
let file = source_file(
temp_dir.join("file.ts"),
"import { foo } from './a.js';\nconsole.log(foo);",
);
let diagnostics = client.did_open_file(&file);
// no warnings because "a.js" exists
assert_eq!(diagnostics.all().len(), 0);
let diagnostics = client.did_open_file(&a_dts);
assert_eq!(diagnostics.all().len(), 0, "Got {:#?}", diagnostics.all());
let response = client.write_request(
"textDocument/references",
json!({
"textDocument": {
"uri": temp_dir.join("file.ts").uri_file()
},
"range": {
"start": { "line": 0, "character": 19 },
"end": { "line": 0, "character": 24 }
},
"textDocument": a_dts.identifier(),
"position": a_dts.range_of("foo").start,
"context": {
"diagnostics": [{
"range": {
"start": { "line": 0, "character": 19 },
"end": { "line": 0, "character": 24 }
},
"severity": 3,
"code": "redirect",
"source": "deno",
"message": format!(
"The import of \"{}\" was redirected to \"{}\".",
temp_dir.join("a").uri_file(),
temp_dir.join("a.ts").uri_file()
),
"data": {
"specifier": temp_dir.join("a").uri_file(),
"redirect": temp_dir.join("a.ts").uri_file(),
},
}],
"only": ["quickfix"]
"includeDeclaration": false
}
}),
);
assert_eq!(
res,
json!([{
"title": "Update specifier to its redirected specifier.",
"kind": "quickfix",
"diagnostics": [{
"range": {
"start": { "line": 0, "character": 19 },
"end": { "line": 0, "character": 24 }
},
"severity": 3,
"code": "redirect",
"source": "deno",
"message": format!(
"The import of \"{}\" was redirected to \"{}\".",
temp_dir.join("a").uri_file(),
temp_dir.join("a.ts").uri_file()
),
"data": {
"specifier": temp_dir.join("a").uri_file(),
"redirect": temp_dir.join("a.ts").uri_file()
},
}],
"edit": {
"changes": {
temp_dir.join("file.ts").uri_file(): [{
"range": {
"start": { "line": 0, "character": 19 },
"end": { "line": 0, "character": 24 }
},
"newText": "\"./a.ts\""
}]
}
assert_json_subset(
response,
json!([
{
"uri": file.uri(),
// the import
"range": file.range_of("foo"),
},
{
"uri": file.uri(),
// the usage
"range": file.range_of_nth(1, "foo"),
}
}])
]),
);
client.shutdown();
}
#[test]

View File

@ -778,6 +778,12 @@ impl LspClient {
}
}
pub fn did_open_file(&mut self, file: &SourceFile) -> CollectedDiagnostics {
self.did_open(json!({
"textDocument": file.text_document(),
}))
}
pub fn did_open(&mut self, params: Value) -> CollectedDiagnostics {
self.did_open_raw(params);
self.read_diagnostics()
@ -1138,6 +1144,130 @@ impl CollectedDiagnostics {
}
}
#[derive(Debug, Clone)]
pub struct SourceFile {
path: PathRef,
src: String,
lang: &'static str,
version: i32,
}
impl SourceFile {
pub fn new(path: PathRef, src: String) -> Self {
path.write(&src);
Self::new_in_mem(path, src)
}
pub fn new_in_mem(path: PathRef, src: String) -> Self {
let lang = match path.as_path().extension().unwrap().to_str().unwrap() {
"js" => "javascript",
"ts" | "d.ts" => "typescript",
"json" => "json",
other => panic!("unsupported file extension: {other}"),
};
Self {
path,
src,
lang,
version: 1,
}
}
pub fn range_of(&self, text: &str) -> lsp::Range {
range_of(text, &self.src)
}
pub fn range_of_nth(&self, n: usize, text: &str) -> lsp::Range {
range_of_nth(n, text, &self.src)
}
pub fn uri(&self) -> lsp::Url {
self.path.uri_file()
}
pub fn text_document(&self) -> lsp::TextDocumentItem {
lsp::TextDocumentItem {
uri: self.uri(),
language_id: self.lang.to_string(),
version: self.version,
text: self.src.clone(),
}
}
pub fn identifier(&self) -> lsp::TextDocumentIdentifier {
lsp::TextDocumentIdentifier { uri: self.uri() }
}
}
/// Helper to create a `SourceFile` and write its contents to disk.
pub fn source_file(path: PathRef, src: impl AsRef<str>) -> SourceFile {
SourceFile::new(path, src.as_ref().to_string())
}
/// Helper to create a `SourceFile` in memory without writing to disk.
pub fn source_file_in_mem(path: PathRef, src: impl AsRef<str>) -> SourceFile {
SourceFile::new_in_mem(path, src.as_ref().to_string())
}
/// Helper to get the `lsp::Range` of the `n`th occurrence of
/// `text` in `src`. `n` is zero-based, like most indexes.
pub fn range_of_nth(
n: usize,
text: impl AsRef<str>,
src: impl AsRef<str>,
) -> lsp::Range {
let text = text.as_ref();
let src = src.as_ref();
let start = src
.match_indices(text)
.nth(n)
.map(|(i, _)| i)
.unwrap_or_else(|| panic!("couldn't find text {text} in source {src}"));
let end = start + text.len();
let mut line = 0;
let mut col = 0;
let mut byte_idx = 0;
let pos = |line, col| lsp::Position {
line,
character: col,
};
let mut start_pos = None;
let mut end_pos = None;
for c in src.chars() {
if byte_idx == start {
start_pos = Some(pos(line, col));
}
if byte_idx == end {
end_pos = Some(pos(line, col));
break;
}
if c == '\n' {
line += 1;
col = 0;
} else {
col += c.len_utf16() as u32;
}
byte_idx += c.len_utf8();
}
if start_pos.is_some() && end_pos.is_none() {
// range extends to end of string
end_pos = Some(pos(line, col));
}
let (start, end) = (start_pos.unwrap(), end_pos.unwrap());
lsp::Range { start, end }
}
/// Helper to get the `lsp::Range` of the first occurrence of
/// `text` in `src`. Equivalent to `range_of_nth(0, text, src)`.
pub fn range_of(text: impl AsRef<str>, src: impl AsRef<str>) -> lsp::Range {
range_of_nth(0, text, src)
}
#[cfg(test)]
mod tests {
use super::*;