Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lsp): exclude missing import quick fixes with bad resolutions #26025

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 37 additions & 11 deletions cli/lsp/analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,18 +228,21 @@ pub struct TsResponseImportMapper<'a> {
documents: &'a Documents,
maybe_import_map: Option<&'a ImportMap>,
resolver: &'a LspResolver,
file_referrer: ModuleSpecifier,
}

impl<'a> TsResponseImportMapper<'a> {
pub fn new(
documents: &'a Documents,
maybe_import_map: Option<&'a ImportMap>,
resolver: &'a LspResolver,
file_referrer: &ModuleSpecifier,
) -> Self {
Self {
documents,
maybe_import_map,
resolver,
file_referrer: file_referrer.clone(),
}
}

Expand All @@ -260,8 +263,6 @@ impl<'a> TsResponseImportMapper<'a> {
}
}

let file_referrer = self.documents.get_file_referrer(referrer);

if let Some(jsr_path) = specifier.as_str().strip_prefix(jsr_url().as_str())
{
let mut segments = jsr_path.split('/');
Expand All @@ -276,7 +277,7 @@ impl<'a> TsResponseImportMapper<'a> {
let export = self.resolver.jsr_lookup_export_for_path(
&nv,
&path,
file_referrer.as_deref(),
Some(&self.file_referrer),
)?;
let sub_path = (export != ".").then_some(export);
let mut req = None;
Expand All @@ -302,7 +303,7 @@ impl<'a> TsResponseImportMapper<'a> {
req = req.or_else(|| {
self
.resolver
.jsr_lookup_req_for_nv(&nv, file_referrer.as_deref())
.jsr_lookup_req_for_nv(&nv, Some(&self.file_referrer))
});
let spec_str = if let Some(req) = req {
let req_ref = PackageReqReference { req, sub_path };
Expand Down Expand Up @@ -332,7 +333,7 @@ impl<'a> TsResponseImportMapper<'a> {

if let Some(npm_resolver) = self
.resolver
.maybe_managed_npm_resolver(file_referrer.as_deref())
.maybe_managed_npm_resolver(Some(&self.file_referrer))
{
if npm_resolver.in_npm_package(specifier) {
if let Ok(Some(pkg_id)) =
Expand Down Expand Up @@ -468,6 +469,26 @@ impl<'a> TsResponseImportMapper<'a> {
}
None
}

pub fn is_valid_import(
&self,
specifier_text: &str,
referrer: &ModuleSpecifier,
) -> bool {
self
.resolver
.as_graph_resolver(Some(&self.file_referrer))
.resolve(
specifier_text,
&deno_graph::Range {
specifier: referrer.clone(),
start: deno_graph::Position::zeroed(),
end: deno_graph::Position::zeroed(),
},
deno_graph::source::ResolutionMode::Types,
)
.is_ok()
}
}

fn try_reverse_map_package_json_exports(
Expand Down Expand Up @@ -580,7 +601,7 @@ fn fix_ts_import_action(
referrer: &ModuleSpecifier,
action: &tsc::CodeFixAction,
import_mapper: &TsResponseImportMapper,
) -> Result<tsc::CodeFixAction, AnyError> {
) -> Result<Option<tsc::CodeFixAction>, AnyError> {
if matches!(
action.fix_name.as_str(),
"import" | "fixMissingFunctionDeclaration"
Expand Down Expand Up @@ -623,19 +644,21 @@ fn fix_ts_import_action(
})
.collect();

return Ok(tsc::CodeFixAction {
return Ok(Some(tsc::CodeFixAction {
description,
changes,
commands: None,
fix_name: action.fix_name.clone(),
fix_id: None,
fix_all_description: None,
});
}));
} else if !import_mapper.is_valid_import(specifier, referrer) {
return Ok(None);
}
}
}

Ok(action.clone())
Ok(Some(action.clone()))
}

/// Determines if two TypeScript diagnostic codes are effectively equivalent.
Expand Down Expand Up @@ -976,11 +999,14 @@ impl CodeActionCollection {
"The action returned from TypeScript is unsupported.",
));
}
let action = fix_ts_import_action(
let Some(action) = fix_ts_import_action(
specifier,
action,
&language_server.get_ts_response_import_mapper(specifier),
)?;
)?
else {
return Ok(());
};
let edit = ts_changes_to_edit(&action.changes, language_server)?;
let code_action = lsp::CodeAction {
title: action.description.clone(),
Expand Down
1 change: 1 addition & 0 deletions cli/lsp/language_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1918,6 +1918,7 @@ impl Inner {
// as the import map is an implementation detail
.and_then(|d| d.resolver.maybe_import_map()),
self.resolver.as_ref(),
file_referrer,
)
}

Expand Down
68 changes: 68 additions & 0 deletions tests/integration/lsp_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8504,6 +8504,74 @@ fn lsp_completions_auto_import_and_quick_fix_with_import_map() {
client.shutdown();
}

// Regression test for https://github.com/denoland/deno/issues/25775.
#[test]
fn lsp_quick_fix_missing_import_exclude_bare_node_builtins() {
let context = TestContextBuilder::new()
.use_http_server()
.use_temp_cwd()
.add_npm_env_vars()
.build();
let temp_dir = context.temp_dir();
temp_dir.write(
"package.json",
json!({
"dependencies": {
"@types/node": "*",
},
})
.to_string(),
);
context.run_npm("install");
let mut client = context.new_lsp_command().build();
client.initialize_default();
let diagnostics = client.did_open(json!({
"textDocument": {
"uri": temp_dir.url().join("file.ts").unwrap(),
"languageId": "typescript",
"version": 1,
// Include node:buffer import to ensure @types/node is in the dep graph.
"text": "import \"node:buffer\";\nassert();\n",
},
}));
let diagnostic = diagnostics
.all()
.into_iter()
.find(|d| d.message == "Cannot find name 'assert'.")
.unwrap();
let res = client.write_request(
"textDocument/codeAction",
json!({
"textDocument": {
"uri": temp_dir.url().join("file.ts").unwrap(),
},
"range": {
"start": { "line": 1, "character": 0 },
"end": { "line": 1, "character": 6 },
},
"context": {
"diagnostics": [&diagnostic],
"only": ["quickfix"],
},
}),
);
let code_actions =
serde_json::from_value::<Vec<lsp::CodeAction>>(res).unwrap();
let titles = code_actions
.iter()
.map(|a| a.title.clone())
.collect::<Vec<_>>();
assert_eq!(
json!(titles),
json!([
"Add import from \"node:assert\"",
"Add import from \"node:console\"",
"Add missing function declaration 'assert'",
]),
);
client.shutdown();
}

#[test]
fn lsp_completions_snippet() {
let context = TestContextBuilder::new().use_temp_cwd().build();
Expand Down