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

feat(lsp): quick fix for @deno-types="npm:@types/*" #25954

Merged
merged 4 commits into from
Oct 1, 2024
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
12 changes: 6 additions & 6 deletions Cargo.lock

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

6 changes: 3 additions & 3 deletions cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "proposa
deno_cache_dir = { workspace = true }
deno_config = { version = "=0.35.0", features = ["workspace", "sync"] }
deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
deno_doc = { version = "0.150.0", features = ["html", "syntect"] }
deno_graph = { version = "=0.82.3" }
deno_doc = { version = "0.150.1", features = ["html", "syntect"] }
deno_graph = { version = "=0.83.0" }
deno_lint = { version = "=0.67.0", features = ["docs"] }
deno_lockfile.workspace = true
deno_npm.workspace = true
Expand All @@ -79,7 +79,7 @@ deno_runtime = { workspace = true, features = ["include_js_files_for_snapshottin
deno_semver.workspace = true
deno_task_shell = "=0.17.0"
deno_terminal.workspace = true
eszip = "=0.78.0"
eszip = "=0.79.1"
libsui = "0.4.0"
napi_sym.workspace = true
node_resolver.workspace = true
Expand Down
159 changes: 159 additions & 0 deletions cli/lsp/analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@

use super::diagnostics::DenoDiagnostic;
use super::diagnostics::DiagnosticSource;
use super::documents::Document;
use super::documents::Documents;
use super::language_server;
use super::resolver::LspResolver;
use super::tsc;
use super::urls::url_to_uri;

use crate::args::jsr_url;
use crate::lsp::search::PackageSearchApi;
use crate::tools::lint::CliLinter;
use deno_config::workspace::MappedResolution;
use deno_lint::diagnostic::LintDiagnosticRange;

use deno_ast::SourceRange;
Expand Down Expand Up @@ -1151,6 +1154,162 @@ impl CodeActionCollection {
..Default::default()
}));
}

pub async fn add_source_actions(
&mut self,
document: &Document,
range: &lsp::Range,
language_server: &language_server::Inner,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future we need to eventually refactor this so that we're not passing dependencies around via method parameters.

) {
async fn deno_types_for_npm_action(
document: &Document,
range: &lsp::Range,
language_server: &language_server::Inner,
) -> Option<lsp::CodeAction> {
let (dep_key, dependency, _) =
document.get_maybe_dependency(&range.end)?;
if dependency.maybe_deno_types_specifier.is_some() {
return None;
}
if dependency.maybe_code.maybe_specifier().is_none()
&& dependency.maybe_type.maybe_specifier().is_none()
{
// We're using byonm and the package is not cached.
return None;
}
let position = deno_graph::Position::new(
range.end.line as usize,
range.end.character as usize,
);
let import_range = dependency.imports.iter().find_map(|i| {
if json!(i.kind) != json!("es") && json!(i.kind) != json!("tsType") {
return None;
}
if !i.specifier_range.includes(&position) {
return None;
}
i.full_range.as_ref()
})?;
let referrer = document.specifier();
let file_referrer = document.file_referrer();
let config_data = language_server
.config
.tree
.data_for_specifier(file_referrer?)?;
let workspace_resolver = config_data.resolver.clone();
let npm_ref = if let Ok(resolution) =
workspace_resolver.resolve(&dep_key, document.specifier())
{
let specifier = match resolution {
MappedResolution::Normal { specifier, .. }
| MappedResolution::ImportMap { specifier, .. } => specifier,
_ => {
return None;
}
};
NpmPackageReqReference::from_specifier(&specifier).ok()?
} else {
// Only resolve bare package.json deps for byonm.
if !config_data.byonm {
return None;
}
if !language_server
.resolver
.is_bare_package_json_dep(&dep_key, referrer)
{
return None;
}
NpmPackageReqReference::from_str(&format!("npm:{}", &dep_key)).ok()?
};
let package_name = &npm_ref.req().name;
if package_name.starts_with("@types/") {
return None;
}
let managed_npm_resolver = language_server
.resolver
.maybe_managed_npm_resolver(file_referrer);
if let Some(npm_resolver) = managed_npm_resolver {
if !npm_resolver.is_pkg_req_folder_cached(npm_ref.req()) {
return None;
}
}
if language_server
.resolver
.npm_to_file_url(&npm_ref, document.specifier(), file_referrer)
.is_some()
{
// The package import has types.
return None;
}
let types_package_name = format!("@types/{package_name}");
let types_package_version = language_server
.npm_search_api
.versions(&types_package_name)
.await
.ok()
.and_then(|versions| versions.first().cloned())?;
let types_specifier_text =
if let Some(npm_resolver) = managed_npm_resolver {
let mut specifier_text = if let Some(req) =
npm_resolver.top_package_req_for_name(&types_package_name)
{
format!("npm:{req}")
} else {
format!("npm:{}@^{}", &types_package_name, types_package_version)
};
let specifier = ModuleSpecifier::parse(&specifier_text).ok()?;
if let Some(file_referrer) = file_referrer {
if let Some(text) = language_server
.get_ts_response_import_mapper(file_referrer)
.check_specifier(&specifier, referrer)
{
specifier_text = text;
}
}
specifier_text
} else {
types_package_name.clone()
};
let uri = language_server
.url_map
.specifier_to_uri(referrer, file_referrer)
.ok()?;
let position = lsp::Position {
line: import_range.start.line as u32,
character: import_range.start.character as u32,
};
let new_text = format!(
"{}// @deno-types=\"{}\"\n",
if position.character == 0 { "" } else { "\n" },
&types_specifier_text
);
let text_edit = lsp::TextEdit {
range: lsp::Range {
start: position,
end: position,
},
new_text,
};
Some(lsp::CodeAction {
title: format!(
"Add @deno-types directive for \"{}\"",
&types_specifier_text
),
kind: Some(lsp::CodeActionKind::QUICKFIX),
diagnostics: None,
edit: Some(lsp::WorkspaceEdit {
changes: Some([(uri, vec![text_edit])].into_iter().collect()),
..Default::default()
}),
..Default::default()
})
}
if let Some(action) =
deno_types_for_npm_action(document, range, language_server).await
{
self.actions.push(CodeActionKind::Deno(action));
}
}
}

/// Prepend the whitespace characters found at the start of line_content to content.
Expand Down
12 changes: 7 additions & 5 deletions cli/lsp/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1517,17 +1517,19 @@ fn diagnose_dependency(
let import_ranges: Vec<_> = dependency
.imports
.iter()
.map(|i| documents::to_lsp_range(&i.range))
.map(|i| documents::to_lsp_range(&i.specifier_range))
.collect();
// TODO(nayeemrmn): This is a crude way of detecting `@deno-types` which has
// a different specifier and therefore needs a separate call to
// `diagnose_resolution()`. It would be much cleaner if that were modelled as
// a separate dependency: https://github.com/denoland/deno_graph/issues/247.
let is_types_deno_types = !dependency.maybe_type.is_none()
&& !dependency
.imports
.iter()
.any(|i| dependency.maybe_type.includes(&i.range.start).is_some());
&& !dependency.imports.iter().any(|i| {
dependency
.maybe_type
.includes(&i.specifier_range.start)
.is_some()
});

diagnostics.extend(
diagnose_resolution(
Expand Down
15 changes: 10 additions & 5 deletions cli/lsp/language_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,11 @@ pub struct Inner {
module_registry: ModuleRegistry,
/// A lazily create "server" for handling test run requests.
maybe_testing_server: Option<testing::TestServer>,
npm_search_api: CliNpmSearchApi,
pub npm_search_api: CliNpmSearchApi,
project_version: usize,
/// A collection of measurements which instrument that performance of the LSP.
performance: Arc<Performance>,
resolver: Arc<LspResolver>,
pub resolver: Arc<LspResolver>,
task_queue: LanguageServerTaskQueue,
/// A memoized version of fixable diagnostic codes retrieved from TypeScript.
ts_fixable_diagnostics: Vec<String>,
Expand Down Expand Up @@ -1612,8 +1612,8 @@ impl Inner {
None => false,
})
.collect();
let mut code_actions = CodeActionCollection::default();
if !fixable_diagnostics.is_empty() {
let mut code_actions = CodeActionCollection::default();
let file_diagnostics = self
.diagnostics_server
.get_ts_diagnostics(&specifier, asset_or_doc.document_lsp_version());
Expand Down Expand Up @@ -1721,9 +1721,14 @@ impl Inner {
.add_cache_all_action(&specifier, no_cache_diagnostics.to_owned());
}
}
code_actions.set_preferred_fixes();
all_actions.extend(code_actions.get_response());
}
if let Some(document) = asset_or_doc.document() {
code_actions
.add_source_actions(document, &params.range, self)
.await;
}
code_actions.set_preferred_fixes();
all_actions.extend(code_actions.get_response());

// Refactor
let only = params
Expand Down
26 changes: 23 additions & 3 deletions cli/lsp/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,11 +328,11 @@ impl LspResolver {
) -> Option<(ModuleSpecifier, MediaType)> {
let resolver = self.get_scope_resolver(file_referrer);
let node_resolver = resolver.node_resolver.as_ref()?;
Some(NodeResolution::into_specifier_and_media_type(
Some(NodeResolution::into_specifier_and_media_type(Some(
node_resolver
.resolve_req_reference(req_ref, referrer, NodeResolutionMode::Types)
.ok(),
))
.ok()?,
)))
}

pub fn in_node_modules(&self, specifier: &ModuleSpecifier) -> bool {
Expand Down Expand Up @@ -373,6 +373,26 @@ impl LspResolver {
Some(NodeResolution::into_specifier_and_media_type(Some(resolution)).1)
}

pub fn is_bare_package_json_dep(
&self,
specifier_text: &str,
referrer: &ModuleSpecifier,
) -> bool {
let resolver = self.get_scope_resolver(Some(referrer));
let Some(node_resolver) = resolver.node_resolver.as_ref() else {
return false;
};
node_resolver
.resolve_if_for_npm_pkg(
specifier_text,
referrer,
NodeResolutionMode::Types,
)
.ok()
.flatten()
.is_some()
}

pub fn get_closest_package_json(
&self,
referrer: &ModuleSpecifier,
Expand Down
10 changes: 10 additions & 0 deletions cli/npm/managed/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,16 @@ impl ManagedCliNpmResolver {
self.resolution.snapshot()
}

pub fn top_package_req_for_name(&self, name: &str) -> Option<PackageReq> {
let package_reqs = self.resolution.package_reqs();
let mut entries = package_reqs
.iter()
.filter(|(_, nv)| nv.name == name)
.collect::<Vec<_>>();
entries.sort_by_key(|(_, nv)| &nv.version);
Some(entries.last()?.0.clone())
}

pub fn serialized_valid_snapshot_for_system(
&self,
system_info: &NpmSystemInfo,
Expand Down
Loading