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.

Sign up for GitHub

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

Jump to bottom

fix(lsp): don't normalize urls in cache command params #22182

Merged
merged 1 commit into from Jan 30, 2024
Merged

fix(lsp): don't normalize urls in cache command params #22182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
163 changes: 61 additions & 102 deletions cli/lsp/language_server.rs
View file
Open in desktop
Expand Up @@ -284,7 +284,8 @@ impl LanguageServer {
/// in the Deno cache, including any of their dependencies.
pub async fn cache_request(
&self,
params: Option<Value>,
specifiers: Vec<ModuleSpecifier>,
referrer: ModuleSpecifier,
) -> LspResult<Option<Value>> {
async fn create_graph_for_caching(
cli_options: CliOptions,
Expand Down Expand Up @@ -330,50 +331,44 @@ impl LanguageServer {
Ok(())
}

match params.map(serde_json::from_value) {
Some(Ok(params)) => {
// do as much as possible in a read, then do a write outside
let maybe_prepare_cache_result = {
let inner = self.0.read().await; // ensure dropped
match inner.prepare_cache(params) {
Ok(maybe_cache_result) => maybe_cache_result,
Err(err) => {
self
.0
.read()
.await
.client
.show_message(MessageType::WARNING, err);
return Err(LspError::internal_error());
}
}
};
if let Some(result) = maybe_prepare_cache_result {
let cli_options = result.cli_options;
let roots = result.roots;
let open_docs = result.open_docs;
let handle = spawn(async move {
create_graph_for_caching(cli_options, roots, open_docs).await
});
if let Err(err) = handle.await.unwrap() {
self
.0
.read()
.await
.client
.show_message(MessageType::WARNING, err);
}
// do npm resolution in a write—we should have everything
// cached by this point anyway
self.0.write().await.refresh_npm_specifiers().await;
// now refresh the data in a read
self.0.read().await.post_cache(result.mark).await;
// do as much as possible in a read, then do a write outside
let maybe_prepare_cache_result = {
let inner = self.0.read().await; // ensure dropped
match inner.prepare_cache(specifiers, referrer) {
Ok(maybe_cache_result) => maybe_cache_result,
Err(err) => {
self
.0
.read()
.await
.client
.show_message(MessageType::WARNING, err);
return Err(LspError::internal_error());
}
Ok(Some(json!(true)))
}
Some(Err(err)) => Err(LspError::invalid_params(err.to_string())),
None => Err(LspError::invalid_params("Missing parameters")),
};
if let Some(result) = maybe_prepare_cache_result {
let cli_options = result.cli_options;
let roots = result.roots;
let open_docs = result.open_docs;
let handle = spawn(async move {
create_graph_for_caching(cli_options, roots, open_docs).await
});
if let Err(err) = handle.await.unwrap() {
self
.0
.read()
.await
.client
.show_message(MessageType::WARNING, err);
}
// do npm resolution in a write—we should have everything
// cached by this point anyway
self.0.write().await.refresh_npm_specifiers().await;
// now refresh the data in a read
self.0.read().await.post_cache(result.mark).await;
}
Ok(Some(json!(true)))
}

/// This request is only used by the lsp integration tests to
Expand All @@ -396,12 +391,6 @@ impl LanguageServer {
Ok(Some(self.0.read().await.get_performance()))
}

pub async fn reload_import_registries_request(
&self,
) -> LspResult<Option<Value>> {
self.0.write().await.reload_import_registries().await
}

pub async fn task_definitions(&self) -> LspResult<Vec<TaskDefinition>> {
self.0.read().await.task_definitions()
}
Expand Down Expand Up @@ -1081,24 +1070,18 @@ impl Inner {
compiler_options_obj.get("jsxImportSource")
{
if let Some(jsx_import_source) = jsx_import_source.as_str() {
let cache_params = lsp_custom::CacheParams {
referrer: TextDocumentIdentifier {
uri: config_file.specifier.clone(),
},
uris: vec![TextDocumentIdentifier {
uri: Url::parse(&format!(
"data:application/typescript;base64,{}",
base64::engine::general_purpose::STANDARD.encode(
format!("import '{jsx_import_source}/jsx-runtime';")
)
))
.unwrap(),
}],
};
let specifiers = vec![Url::parse(&format!(
"data:application/typescript;base64,{}",
base64::engine::general_purpose::STANDARD.encode(format!(
"import '{jsx_import_source}/jsx-runtime';"
))
))
.unwrap()];
let referrer = config_file.specifier.clone();
self.task_queue.queue_task(Box::new(|ls: LanguageServer| {
spawn(async move {
if let Err(err) =
ls.cache_request(Some(json!(cache_params))).await
ls.cache_request(specifiers, referrer).await
Copy link
Member

Choose a reason for hiding this comment

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

+1... kind of crazy we were doing that 😅

{
lsp_warn!("{}", err);
}
Expand Down Expand Up @@ -3222,24 +3205,13 @@ impl tower_lsp::LanguageServer for LanguageServer {
) -> LspResult<Option<Value>> {
if params.command == "deno.cache" {
let mut arguments = params.arguments.into_iter();
let uris = serde_json::to_value(arguments.next()).unwrap();
let uris: Vec<Url> = serde_json::from_value(uris)
let specifiers = serde_json::to_value(arguments.next()).unwrap();
let specifiers: Vec<Url> = serde_json::from_value(specifiers)
.map_err(|err| LspError::invalid_params(err.to_string()))?;
let referrer = serde_json::to_value(arguments.next()).unwrap();
let referrer: Url = serde_json::from_value(referrer)
.map_err(|err| LspError::invalid_params(err.to_string()))?;
self
.cache_request(Some(
serde_json::to_value(lsp_custom::CacheParams {
referrer: TextDocumentIdentifier { uri: referrer },
uris: uris
.into_iter()
.map(|uri| TextDocumentIdentifier { uri })
.collect(),
})
.expect("well formed json"),
))
.await
self.cache_request(specifiers, referrer).await
} else if params.command == "deno.reloadImportRegistries" {
self.0.write().await.reload_import_registries().await
} else {
Expand Down Expand Up @@ -3421,7 +3393,7 @@ impl tower_lsp::LanguageServer for LanguageServer {

async fn did_save(&self, params: DidSaveTextDocumentParams) {
let uri = &params.text_document.uri;
{
let specifier = {
let mut inner = self.0.write().await;
let specifier = inner.url_map.normalize_url(uri, LspUrlKind::File);
inner.documents.save(&specifier);
Expand All @@ -3438,18 +3410,10 @@ impl tower_lsp::LanguageServer for LanguageServer {
Ok(path) if is_importable_ext(&path) => {}
_ => return,
}
}
if let Err(err) = self
.cache_request(Some(
serde_json::to_value(lsp_custom::CacheParams {
referrer: TextDocumentIdentifier { uri: uri.clone() },
uris: vec![TextDocumentIdentifier { uri: uri.clone() }],
})
.unwrap(),
))
.await
{
lsp_warn!("Failed to cache \"{}\" on save: {}", uri.to_string(), err);
specifier
};
if let Err(err) = self.cache_request(vec![], specifier.clone()).await {
lsp_warn!("Failed to cache \"{}\" on save: {}", &specifier, err);
}
}

Expand Down Expand Up @@ -3693,22 +3657,17 @@ struct PrepareCacheResult {
impl Inner {
fn prepare_cache(
&self,
params: lsp_custom::CacheParams,
specifiers: Vec<ModuleSpecifier>,
referrer: ModuleSpecifier,
) -> Result<Option<PrepareCacheResult>, AnyError> {
let referrer = self
.url_map
.normalize_url(&params.referrer.uri, LspUrlKind::File);
let mark = self.performance.mark_with_args("lsp.cache", &params);
let roots = if !params.uris.is_empty() {
params
.uris
.iter()
.map(|t| self.url_map.normalize_url(&t.uri, LspUrlKind::File))
.collect()
let mark = self
.performance
.mark_with_args("lsp.cache", (&specifiers, &referrer));
let roots = if !specifiers.is_empty() {
specifiers
} else {
vec![referrer]
};

let workspace_settings = self.config.workspace_settings();
let mut cli_options = CliOptions::new(
Flags {
Expand Down
14 changes: 0 additions & 14 deletions cli/lsp/lsp_custom.rs
View file
Open in desktop
Expand Up @@ -4,26 +4,12 @@ use deno_core::serde::Deserialize;
use deno_core::serde::Serialize;
use tower_lsp::lsp_types as lsp;

pub const CACHE_REQUEST: &str = "deno/cache";
pub const PERFORMANCE_REQUEST: &str = "deno/performance";
pub const TASK_REQUEST: &str = "deno/taskDefinitions";
pub const RELOAD_IMPORT_REGISTRIES_REQUEST: &str =
"deno/reloadImportRegistries";
pub const VIRTUAL_TEXT_DOCUMENT: &str = "deno/virtualTextDocument";
pub const LATEST_DIAGNOSTIC_BATCH_INDEX: &str =
"deno/internalLatestDiagnosticBatchIndex";

#[derive(Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct CacheParams {
/// The document currently open in the editor. If there are no `uris`
/// supplied, the referrer will be cached.
pub referrer: lsp::TextDocumentIdentifier,
/// Any documents that have been specifically asked to be cached via the
/// command.
pub uris: Vec<lsp::TextDocumentIdentifier>,
}

#[derive(Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct TaskDefinition {
Expand Down
10 changes: 0 additions & 10 deletions cli/lsp/mod.rs
View file
Open in desktop
Expand Up @@ -48,20 +48,10 @@ pub async fn start() -> Result<(), AnyError> {
token.clone(),
)
})
// TODO(nayeemrmn): The extension has replaced this with the `deno.cache`
// command as of vscode_deno 3.21.0 / 2023.09.05. Remove this eventually.
.custom_method(lsp_custom::CACHE_REQUEST, LanguageServer::cache_request)
.custom_method(
lsp_custom::PERFORMANCE_REQUEST,
LanguageServer::performance_request,
)
// TODO(nayeemrmn): The extension has replaced this with the
// `deno.reloadImportRegistries` command as of vscode_deno
// 3.26.0 / 2023.10.10. Remove this eventually.
.custom_method(
lsp_custom::RELOAD_IMPORT_REGISTRIES_REQUEST,
LanguageServer::reload_import_registries_request,
)
.custom_method(lsp_custom::TASK_REQUEST, LanguageServer::task_definitions)
// TODO(nayeemrmn): Rename this to `deno/taskDefinitions` in vscode_deno and
// remove this alias.
Expand Down
67 changes: 67 additions & 0 deletions cli/tests/integration/lsp_tests.rs
View file
Open in desktop
Expand Up @@ -4945,6 +4945,73 @@ fn lsp_cache_on_save() {
client.shutdown();
}

// Regression test for https://github.com/denoland/deno/issues/22122.
#[test]
fn lsp_cache_then_definition() {
let context = TestContextBuilder::new()
.use_http_server()
.use_temp_cwd()
.build();
let temp_dir = context.temp_dir();
let mut client = context.new_lsp_command().build();
client.initialize_default();
client.did_open(json!({
"textDocument": {
"uri": temp_dir.uri().join("file.ts").unwrap(),
"languageId": "typescript",
"version": 1,
"text": r#"import "http://localhost:4545/run/002_hello.ts";"#,
},
}));
// Prior to the fix, this would cause a faulty memoization that maps the
// URL "http://localhost:4545/run/002_hello.ts" to itself, preventing it from
// being reverse-mapped to "deno:/http/localhost%3A4545/run/002_hello.ts" on
// "textDocument/definition" request.
client.write_request(
"workspace/executeCommand",
json!({
"command": "deno.cache",
"arguments": [
["http://localhost:4545/run/002_hello.ts"],
temp_dir.uri().join("file.ts").unwrap(),
],
}),
);
let res = client.write_request(
"textDocument/definition",
json!({
"textDocument": { "uri": temp_dir.uri().join("file.ts").unwrap() },
"position": { "line": 0, "character": 8 },
}),
);
assert_eq!(
res,
json!([{
"targetUri": "deno:/http/localhost%3A4545/run/002_hello.ts",
"targetRange": {
"start": {
"line": 0,
"character": 0,
},
"end": {
"line": 1,
"character": 0,
},
},
"targetSelectionRange": {
"start": {
"line": 0,
"character": 0,
},
"end": {
"line": 1,
"character": 0,
},
},
}]),
);
}

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