-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
refactor(lsp): implement "deno.cacheOnSave" server-side #20632
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nayeemrmn
commented
Sep 22, 2023
Patch on top of this for #16082:diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs
index c1def6012..5a5329e26 100644
--- a/cli/lsp/analysis.rs
+++ b/cli/lsp/analysis.rs
@@ -917,6 +917,24 @@ impl CodeActionCollection {
}
}
}
+
+ pub fn add_cache_all_action(
+ &mut self,
+ specifier: &ModuleSpecifier,
+ diagnostics: Vec<lsp::Diagnostic>,
+ ) {
+ self.actions.push(CodeActionKind::Deno(lsp::CodeAction {
+ title: "Cache all dependencies of this module.".to_string(),
+ kind: Some(lsp::CodeActionKind::QUICKFIX),
+ diagnostics: Some(diagnostics),
+ command: Some(lsp::Command {
+ title: "".to_string(),
+ command: "deno.cache".to_string(),
+ arguments: Some(vec![json!([&specifier]), json!(&specifier)]),
+ }),
+ ..Default::default()
+ }));
+ }
}
/// Prepend the whitespace characters found at the start of line_content to content.
diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs
index 0f3c6c7d4..1566ef311 100644
--- a/cli/lsp/diagnostics.rs
+++ b/cli/lsp/diagnostics.rs
@@ -298,7 +298,7 @@ struct ChannelUpdateMessage {
#[derive(Clone, Debug)]
struct SpecifierState {
- has_no_cache_diagnostic: bool,
+ no_cache_diagnostics: Vec<lsp::Diagnostic>,
}
#[derive(Clone, Debug, Default)]
@@ -319,30 +319,36 @@ impl DiagnosticsState {
_ => true,
};
if is_new {
+ let mut no_cache_diagnostics = vec![];
+ for diagnostic in diagnostics {
+ if diagnostic.code
+ == Some(lsp::NumberOrString::String("no-cache".to_string()))
+ || diagnostic.code
+ == Some(lsp::NumberOrString::String("no-cache-npm".to_string()))
+ {
+ no_cache_diagnostics.push(diagnostic.clone());
+ }
+ }
self.specifiers.insert(
specifier.clone(),
(
version,
SpecifierState {
- has_no_cache_diagnostic: diagnostics.iter().any(|d| {
- d.code
- == Some(lsp::NumberOrString::String("no-cache".to_string()))
- || d.code
- == Some(lsp::NumberOrString::String(
- "no-cache-npm".to_string(),
- ))
- }),
+ no_cache_diagnostics,
},
),
);
}
}
- pub fn has_no_cache_diagnostic(&self, specifier: &ModuleSpecifier) -> bool {
+ pub fn no_cache_diagnostics(
+ &self,
+ specifier: &ModuleSpecifier,
+ ) -> &[lsp::Diagnostic] {
self
.specifiers
.get(specifier)
- .map_or(false, |s| s.1.has_no_cache_diagnostic)
+ .map_or(&[], |s| &s.1.no_cache_diagnostics)
}
}
@@ -374,8 +380,8 @@ impl DiagnosticsServer {
}
}
- pub fn state(&self) -> Arc<RwLock<DiagnosticsState>> {
- self.state.clone()
+ pub fn state(&self) -> &Arc<RwLock<DiagnosticsState>> {
+ &self.state
}
pub fn get_ts_diagnostics(
@@ -902,7 +908,7 @@ async fn generate_ts_diagnostics(
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
-struct DiagnosticDataSpecifier {
+pub struct DiagnosticDataSpecifier {
pub specifier: ModuleSpecifier,
}
@@ -1046,14 +1052,11 @@ impl DenoDiagnostic {
.clone()
.ok_or_else(|| anyhow!("Diagnostic is missing data"))?;
let data: DiagnosticDataSpecifier = serde_json::from_value(data)?;
- let title = match code.as_str() {
- "no-cache" | "no-cache-npm" => {
- format!("Cache \"{}\" and its dependencies.", data.specifier)
- }
- _ => "Cache the data URL and its dependencies.".to_string(),
- };
lsp::CodeAction {
- title,
+ title: format!(
+ "Cache \"{}\" and its dependencies.",
+ data.specifier
+ ),
kind: Some(lsp::CodeActionKind::QUICKFIX),
diagnostics: Some(vec![diagnostic.clone()]),
command: Some(lsp::Command {
diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
index b9f495ab1..b39871edd 100644
--- a/cli/lsp/language_server.rs
+++ b/cli/lsp/language_server.rs
@@ -48,6 +48,7 @@ use super::config::ConfigSnapshot;
use super::config::WorkspaceSettings;
use super::config::SETTINGS_SECTION;
use super::diagnostics;
+use super::diagnostics::DiagnosticDataSpecifier;
use super::diagnostics::DiagnosticServerUpdateMessage;
use super::diagnostics::DiagnosticsServer;
use super::documents::to_hover_text;
@@ -1913,6 +1914,7 @@ impl Inner {
let file_diagnostics = self
.diagnostics_server
.get_ts_diagnostics(&specifier, asset_or_doc.document_lsp_version());
+ let mut includes_no_cache = false;
for diagnostic in &fixable_diagnostics {
match diagnostic.source.as_deref() {
Some("deno-ts") => {
@@ -1956,12 +1958,21 @@ impl Inner {
}
}
}
- Some("deno") => code_actions
- .add_deno_fix_action(&specifier, diagnostic)
- .map_err(|err| {
- error!("{}", err);
- LspError::internal_error()
- })?,
+ Some("deno") => {
+ if diagnostic.code
+ == Some(NumberOrString::String("no-cache".to_string()))
+ || diagnostic.code
+ == Some(NumberOrString::String("no-cache-npm".to_string()))
+ {
+ includes_no_cache = true;
+ }
+ code_actions
+ .add_deno_fix_action(&specifier, diagnostic)
+ .map_err(|err| {
+ error!("{}", err);
+ LspError::internal_error()
+ })?
+ }
Some("deno-lint") => code_actions
.add_deno_lint_ignore_action(
&specifier,
@@ -1976,6 +1987,24 @@ impl Inner {
_ => (),
}
}
+ if includes_no_cache {
+ let state = self.diagnostics_server.state().read().await;
+ let no_cache_diagnostics = state.no_cache_diagnostics(&specifier);
+ let uncached_deps = no_cache_diagnostics
+ .iter()
+ .filter_map(|d| {
+ let data = serde_json::from_value::<DiagnosticDataSpecifier>(
+ d.data.clone().into(),
+ )
+ .ok()?;
+ Some(data.specifier)
+ })
+ .collect::<HashSet<_>>();
+ if uncached_deps.len() > 1 {
+ code_actions
+ .add_cache_all_action(&specifier, no_cache_diagnostics.to_owned());
+ }
+ }
code_actions.set_preferred_fixes();
all_actions.extend(code_actions.get_response());
}
@@ -3283,9 +3312,14 @@ impl tower_lsp::LanguageServer for LanguageServer {
{
return;
}
- inner.diagnostics_server.state()
+ inner.diagnostics_server.state().clone()
};
- if !diagnostics_state.read().await.has_no_cache_diagnostic(uri) {
+ if diagnostics_state
+ .read()
+ .await
+ .no_cache_diagnostics(uri)
+ .is_empty()
+ {
return;
}
if let Err(err) = self I'll do it in a follow-up PR. |
bartlomieju
reviewed
Sep 23, 2023
bartlomieju
approved these changes
Sep 24, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
dsherret
reviewed
Sep 24, 2023
dsherret
reviewed
Sep 24, 2023
This was referenced Sep 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #20587. Will do an accompanying change in vscode_deno to disable the client-side implementation when connected to Deno >1.37.0. Though even if both are enabled, it doesn't seem to be duplicating any work.
Check if the
DiagnosticsState
structure looks good. It's a way to save lightweight, up-to-date and useful diagnostics information for each module, without needing to save every diagnostic for every version and coordinating the cleanup for that. Related: #16082.