foster/deno
0
0
Fork 0
mirror of https://github.com/denoland/deno.git synced 2025-03-03 09:31:22 -05:00
Code Issues Packages 1 Wiki Activity

fix(lsp): remove Sources mutex

Browse source 
The mutex was used to hide the fact that the Sources object mutates
itself when it's queried. Be honest about that and mark everything that
directly or indirectly mutates it as `mut`.

This is a follow-up to commit 2828690fc7
from last month ("fix(lsp): fix deadlocks, use one big mutex (#9271)")
This commit is contained in:
Ben Noordhuis 2021-02-06 13:39:01 +01:00
parent 0cac243a83
commit be10db10fd
4 changed files with 90 additions and 182 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

40
cli/lsp/analysis.rs
View file

@ -1,6 +1,6 @@
// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
use super::text::LineIndex;
use super::language_server;
use super::tsc;
use crate::ast;
@ -13,7 +13,6 @@ use crate::tools::lint::create_linter;
use deno_core::error::custom_error;
use deno_core::error::AnyError;
use deno_core::futures::Future;
use deno_core::serde::Deserialize;
use deno_core::serde::Serialize;
use deno_core::serde_json::json;
@ -353,21 +352,14 @@ fn is_preferred(
/// Convert changes returned from a TypeScript quick fix action into edits
/// for an LSP CodeAction.
pub async fn ts_changes_to_edit<F, Fut, V>(
pub(crate) async fn ts_changes_to_edit(
changes: &[tsc::FileTextChanges],
index_provider: &F,
version_provider: &V,
) -> Result<Option<lsp::WorkspaceEdit>, AnyError>
where
F: Fn(ModuleSpecifier) -> Fut + Clone,
Fut: Future<Output = Result<LineIndex, AnyError>>,
V: Fn(ModuleSpecifier) -> Option<i32>,
{
language_server: &mut language_server::Inner,
) -> Result<Option<lsp::WorkspaceEdit>, AnyError> {
let mut text_document_edits = Vec::new();
for change in changes {
let text_document_edit = change
.to_text_document_edit(index_provider, version_provider)
.await?;
let text_document_edit =
change.to_text_document_edit(language_server).await?;
text_document_edits.push(text_document_edit);
}
Ok(Some(lsp::WorkspaceEdit {
@ -392,18 +384,12 @@ pub struct CodeActionCollection {
impl CodeActionCollection {
/// Add a TypeScript code fix action to the code actions collection.
pub async fn add_ts_fix_action<F, Fut, V>(
pub(crate) async fn add_ts_fix_action(
&mut self,
action: &tsc::CodeFixAction,
diagnostic: &lsp::Diagnostic,
index_provider: &F,
version_provider: &V,
) -> Result<(), AnyError>
where
F: Fn(ModuleSpecifier) -> Fut + Clone,
Fut: Future<Output = Result<LineIndex, AnyError>>,
V: Fn(ModuleSpecifier) -> Option<i32>,
{
language_server: &mut language_server::Inner,
) -> Result<(), AnyError> {
if action.commands.is_some() {
// In theory, tsc can return actions that require "commands" to be applied
// back into TypeScript. Currently there is only one command, `install
@ -417,9 +403,7 @@ impl CodeActionCollection {
"The action returned from TypeScript is unsupported.",
));
}
let edit =
ts_changes_to_edit(&action.changes, index_provider, version_provider)
.await?;
let edit = ts_changes_to_edit(&action.changes, language_server).await?;
let code_action = lsp::CodeAction {
title: action.description.clone(),
kind: Some(lsp::CodeActionKind::QUICKFIX),
@ -502,7 +486,7 @@ impl CodeActionCollection {
&self,
action: &tsc::CodeFixAction,
diagnostic: &lsp::Diagnostic,
file_diagnostics: &[&lsp::Diagnostic],
file_diagnostics: &[lsp::Diagnostic],
) -> bool {
// If the action does not have a fix id (indicating it can be "bundled up")
// or if the collection already contains a "bundled" action return false
@ -517,7 +501,7 @@ impl CodeActionCollection {
// other diagnostics that could be bundled together in a "fix all" code
// action
file_diagnostics.iter().any(|d| {
if d == &diagnostic || d.code.is_none() || diagnostic.code.is_none() {
if d == diagnostic || d.code.is_none() || diagnostic.code.is_none() {
false
} else {
d.code == diagnostic.code

73
cli/lsp/language_server.rs
View file

@ -66,7 +66,7 @@ pub struct StateSnapshot {
}
#[derive(Debug)]
struct Inner {
pub(crate) struct Inner {
/// Cached versions of "fixed" assets that can either be inlined in Rust or
/// are part of the TypeScript snapshot and have to be fetched out.
assets: HashMap<ModuleSpecifier, Option<AssetDocument>>,
@ -132,8 +132,8 @@ impl Inner {
/// Searches assets, open documents and external sources for a line_index,
/// which might be performed asynchronously, hydrating in memory caches for
/// subsequent requests.
async fn get_line_index(
&self,
pub(crate) async fn get_line_index(
&mut self,
specifier: ModuleSpecifier,
) -> Result<LineIndex, AnyError> {
let mark = self.performance.mark("get_line_index");
@ -170,7 +170,7 @@ impl Inner {
/// Only searches already cached assets and documents for a line index. If
/// the line index cannot be found, `None` is returned.
fn get_line_index_sync(
&self,
&mut self,
specifier: &ModuleSpecifier,
) -> Option<LineIndex> {
let mark = self.performance.mark("get_line_index_sync");
@ -501,6 +501,13 @@ impl Inner {
self.performance.measure(mark);
Ok(())
}
pub(crate) fn document_version(
&mut self,
specifier: ModuleSpecifier,
) -> Option<i32> {
self.documents.version(&specifier)
}
}
// lspower::LanguageServer methods. This file's LanguageServer delegates to us.
@ -826,7 +833,7 @@ impl Inner {
}
}
async fn hover(&self, params: HoverParams) -> LspResult<Option<Hover>> {
async fn hover(&mut self, params: HoverParams) -> LspResult<Option<Hover>> {
if !self.enabled() {
return Ok(None);
}
@ -898,9 +905,10 @@ impl Inner {
return Ok(None);
}
let line_index = self.get_line_index_sync(&specifier).unwrap();
let file_diagnostics: Vec<&Diagnostic> = self
let file_diagnostics: Vec<Diagnostic> = self
.diagnostics
.diagnostics_for(&specifier, &DiagnosticSource::TypeScript)
.cloned()
.collect();
let mut code_actions = CodeActionCollection::default();
for diagnostic in &fixable_diagnostics {
@ -931,12 +939,7 @@ impl Inner {
})?;
for action in actions {
code_actions
.add_ts_fix_action(
&action,
diagnostic,
&|s| self.get_line_index(s),
&|s| self.documents.version(&s),
)
.add_ts_fix_action(&action, diagnostic, self)
.await
.map_err(|err| {
error!("Unable to convert fix: {}", err);
@ -958,7 +961,7 @@ impl Inner {
}
async fn code_action_resolve(
&self,
&mut self,
params: CodeAction,
) -> LspResult<CodeAction> {
let mark = self.performance.mark("code_action_resolve");
@ -989,16 +992,13 @@ impl Inner {
Err(LspError::invalid_request())
} else {
let mut code_action = params.clone();
code_action.edit = ts_changes_to_edit(
&combined_code_actions.changes,
&|s| self.get_line_index(s),
&|s| self.documents.version(&s),
)
.await
.map_err(|err| {
error!("Unable to convert changes to edits: {}", err);
LspError::internal_error()
})?;
code_action.edit =
ts_changes_to_edit(&combined_code_actions.changes, self)
.await
.map_err(|err| {
error!("Unable to convert changes to edits: {}", err);
LspError::internal_error()
})?;
Ok(code_action)
}
} else {
@ -1214,7 +1214,7 @@ impl Inner {
}
async fn document_highlight(
&self,
&mut self,
params: DocumentHighlightParams,
) -> LspResult<Option<Vec<DocumentHighlight>>> {
if !self.enabled() {
@ -1342,9 +1342,7 @@ impl Inner {
serde_json::from_value(res).unwrap();
if let Some(definition) = maybe_definition {
let results = definition
.to_definition(&line_index, |s| self.get_line_index(s))
.await;
let results = definition.to_definition(&line_index, self).await;
self.performance.measure(mark);
Ok(results)
} else {
@ -1354,7 +1352,7 @@ impl Inner {
}
async fn completion(
&self,
&mut self,
params: CompletionParams,
) -> LspResult<Option<CompletionResponse>> {
if !self.enabled() {
@ -1399,7 +1397,7 @@ impl Inner {
}
async fn goto_implementation(
&self,
&mut self,
params: GotoImplementationParams,
) -> LspResult<Option<GotoImplementationResponse>> {
if !self.enabled() {
@ -1447,10 +1445,7 @@ impl Inner {
ModuleSpecifier::resolve_url(&document_span.file_name).unwrap();
let impl_line_index =
&self.get_line_index(impl_specifier).await.unwrap();
if let Some(link) = document_span
.to_link(impl_line_index, |s| self.get_line_index(s))
.await
{
if let Some(link) = document_span.to_link(impl_line_index, self).await {
results.push(link);
}
}
@ -1463,13 +1458,13 @@ impl Inner {
}
async fn rename(
&self,
&mut self,
params: RenameParams,
) -> LspResult<Option<WorkspaceEdit>> {
if !self.enabled() {
return Ok(None);
}
let mark = self.performance.mark("goto_implementation");
let mark = self.performance.mark("rename");
let specifier =
utils::normalize_url(params.text_document_position.text_document.uri);
@ -1515,11 +1510,7 @@ impl Inner {
if let Some(locations) = maybe_locations {
let rename_locations = tsc::RenameLocations { locations };
let workspace_edits = rename_locations
.into_workspace_edit(
&params.new_name,
|s| self.get_line_index(s),
|s| self.documents.version(&s),
)
.into_workspace_edit(&params.new_name, self)
.await
.map_err(|err| {
error!("Failed to get workspace edits: {:#?}", err);
@ -1747,7 +1738,7 @@ impl Inner {
}
async fn virtual_text_document(
&self,
&mut self,
params: VirtualTextDocumentParams,
) -> LspResult<Option<String>> {
let mark = self.performance.mark("virtual_text_document");

87
cli/lsp/sources.rs
View file

@ -51,10 +51,7 @@ struct Metadata {
}
#[derive(Debug, Clone, Default)]
pub struct Sources(Arc<Mutex<Inner>>);
#[derive(Debug, Default)]
struct Inner {
pub struct Sources {
http_cache: HttpCache,
maybe_import_map: Option<ImportMap>,
metadata: HashMap<ModuleSpecifier, Metadata>,
@ -64,64 +61,13 @@ struct Inner {
impl Sources {
pub fn new(location: &Path) -> Self {
Self(Arc::new(Mutex::new(Inner::new(location))))
}
pub fn contains(&self, specifier: &ModuleSpecifier) -> bool {
self.0.lock().unwrap().contains(specifier)
}
/// Provides the length of the source content, calculated in a way that should
/// match the behavior of JavaScript, where strings are stored effectively as
/// `&[u16]` and when counting "chars" we need to represent the string as a
/// UTF-16 string in Rust.
pub fn get_length_utf16(&self, specifier: &ModuleSpecifier) -> Option<usize> {
self.0.lock().unwrap().get_length_utf16(specifier)
}
pub fn get_line_index(
&self,
specifier: &ModuleSpecifier,
) -> Option<LineIndex> {
self.0.lock().unwrap().get_line_index(specifier)
}
pub fn get_media_type(
&self,
specifier: &ModuleSpecifier,
) -> Option<MediaType> {
self.0.lock().unwrap().get_media_type(specifier)
}
pub fn get_script_version(
&self,
specifier: &ModuleSpecifier,
) -> Option<String> {
self.0.lock().unwrap().get_script_version(specifier)
}
pub fn get_text(&self, specifier: &ModuleSpecifier) -> Option<String> {
self.0.lock().unwrap().get_text(specifier)
}
pub fn resolve_import(
&self,
specifier: &str,
referrer: &ModuleSpecifier,
) -> Option<(ModuleSpecifier, MediaType)> {
self.0.lock().unwrap().resolve_import(specifier, referrer)
}
}
impl Inner {
fn new(location: &Path) -> Self {
Self {
http_cache: HttpCache::new(location),
..Default::default()
}
}
fn contains(&mut self, specifier: &ModuleSpecifier) -> bool {
pub fn contains(&mut self, specifier: &ModuleSpecifier) -> bool {
if let Some(specifier) = self.resolve_specifier(specifier) {
if self.get_metadata(&specifier).is_some() {
return true;
@ -130,13 +76,20 @@ impl Inner {
false
}
fn get_length_utf16(&mut self, specifier: &ModuleSpecifier) -> Option<usize> {
/// Provides the length of the source content, calculated in a way that should
/// match the behavior of JavaScript, where strings are stored effectively as
/// `&[u16]` and when counting "chars" we need to represent the string as a
/// UTF-16 string in Rust.
pub fn get_length_utf16(
&mut self,
specifier: &ModuleSpecifier,
) -> Option<usize> {
let specifier = self.resolve_specifier(specifier)?;
let metadata = self.get_metadata(&specifier)?;
Some(metadata.source.encode_utf16().count())
}
fn get_line_index(
pub fn get_line_index(
&mut self,
specifier: &ModuleSpecifier,
) -> Option<LineIndex> {
@ -145,7 +98,7 @@ impl Inner {
Some(metadata.line_index)
}
fn get_media_type(
pub fn get_media_type(
&mut self,
specifier: &ModuleSpecifier,
) -> Option<MediaType> {
@ -283,7 +236,7 @@ impl Inner {
None
}
fn get_script_version(
pub fn get_script_version(
&mut self,
specifier: &ModuleSpecifier,
) -> Option<String> {
@ -304,7 +257,7 @@ impl Inner {
None
}
fn get_text(&mut self, specifier: &ModuleSpecifier) -> Option<String> {
pub fn get_text(&mut self, specifier: &ModuleSpecifier) -> Option<String> {
let specifier = self.resolve_specifier(specifier)?;
let metadata = self.get_metadata(&specifier)?;
Some(metadata.source)
@ -324,7 +277,7 @@ impl Inner {
Some((resolved_specifier, media_type))
}
fn resolve_import(
pub fn resolve_import(
&mut self,
specifier: &str,
referrer: &ModuleSpecifier,
@ -420,7 +373,7 @@ mod tests {
#[test]
fn test_sources_get_script_version() {
let (sources, _) = setup();
let (mut sources, _) = setup();
let c = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
let tests = c.join("tests");
let specifier = ModuleSpecifier::resolve_path(
@ -433,7 +386,7 @@ mod tests {
#[test]
fn test_sources_get_text() {
let (sources, _) = setup();
let (mut sources, _) = setup();
let c = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
let tests = c.join("tests");
let specifier = ModuleSpecifier::resolve_path(
@ -448,7 +401,7 @@ mod tests {
#[test]
fn test_sources_get_length_utf16() {
let (sources, _) = setup();
let (mut sources, _) = setup();
let c = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
let tests = c.join("tests");
let specifier = ModuleSpecifier::resolve_path(
@ -463,10 +416,10 @@ mod tests {
#[test]
fn test_sources_resolve_specifier_non_supported_schema() {
let (sources, _) = setup();
let (mut sources, _) = setup();
let specifier = ModuleSpecifier::resolve_url("foo://a/b/c.ts")
.expect("could not create specifier");
let actual = sources.0.lock().unwrap().resolve_specifier(&specifier);
let actual = sources.resolve_specifier(&specifier);
assert!(actual.is_none());
}
}

72
cli/lsp/tsc.rs
View file

@ -2,6 +2,7 @@
use super::analysis::CodeLensSource;
use super::analysis::ResolvedDependency;
use super::language_server;
use super::language_server::StateSnapshot;
use super::text;
use super::text::LineIndex;
@ -16,7 +17,6 @@ use crate::tsc_config::TsConfig;
use deno_core::error::anyhow;
use deno_core::error::custom_error;
use deno_core::error::AnyError;
use deno_core::futures::Future;
use deno_core::json_op_sync;
use deno_core::serde::Deserialize;
use deno_core::serde::Serialize;
@ -443,18 +443,16 @@ pub struct DocumentSpan {
}
impl DocumentSpan {
pub async fn to_link<F, Fut>(
pub(crate) async fn to_link(
&self,
line_index: &LineIndex,
index_provider: F,
) -> Option<lsp::LocationLink>
where
F: Fn(ModuleSpecifier) -> Fut,
Fut: Future<Output = Result<LineIndex, AnyError>>,
{
language_server: &mut language_server::Inner,
) -> Option<lsp::LocationLink> {
let target_specifier =
ModuleSpecifier::resolve_url(&self.file_name).unwrap();
if let Ok(target_line_index) = index_provider(target_specifier).await {
if let Ok(target_line_index) =
language_server.get_line_index(target_specifier).await
{
let target_uri = utils::normalize_file_name(&self.file_name).unwrap();
let (target_range, target_selection_range) =
if let Some(context_span) = &self.context_span {
@ -559,17 +557,11 @@ pub struct RenameLocations {
}
impl RenameLocations {
pub async fn into_workspace_edit<F, Fut, V>(
pub(crate) async fn into_workspace_edit(
self,
new_name: &str,
index_provider: F,
version_provider: V,
) -> Result<lsp::WorkspaceEdit, AnyError>
where
F: Fn(ModuleSpecifier) -> Fut,
Fut: Future<Output = Result<LineIndex, AnyError>>,
V: Fn(ModuleSpecifier) -> Option<i32>,
{
language_server: &mut language_server::Inner,
) -> Result<lsp::WorkspaceEdit, AnyError> {
let mut text_document_edit_map: HashMap<Url, lsp::TextDocumentEdit> =
HashMap::new();
for location in self.locations.iter() {
@ -584,7 +576,7 @@ impl RenameLocations {
lsp::TextDocumentEdit {
text_document: lsp::OptionalVersionedTextDocumentIdentifier {
uri: uri.clone(),
version: version_provider(specifier.clone()),
version: language_server.document_version(specifier.clone()),
},
edits:
Vec::<lsp::OneOf<lsp::TextEdit, lsp::AnnotatedTextEdit>>::new(),
@ -598,7 +590,7 @@ impl RenameLocations {
range: location
.document_span
.text_span
.to_range(&index_provider(specifier.clone()).await?),
.to_range(&language_server.get_line_index(specifier.clone()).await?),
new_text: new_name.to_string(),
}));
}
@ -655,22 +647,16 @@ pub struct DefinitionInfoAndBoundSpan {
}
impl DefinitionInfoAndBoundSpan {
pub async fn to_definition<F, Fut>(
pub(crate) async fn to_definition(
&self,
line_index: &LineIndex,
index_provider: F,
) -> Option<lsp::GotoDefinitionResponse>
where
F: Fn(ModuleSpecifier) -> Fut + Clone,
Fut: Future<Output = Result<LineIndex, AnyError>>,
{
language_server: &mut language_server::Inner,
) -> Option<lsp::GotoDefinitionResponse> {
if let Some(definitions) = &self.definitions {
let mut location_links = Vec::<lsp::LocationLink>::new();
for di in definitions {
if let Some(link) = di
.document_span
.to_link(line_index, index_provider.clone())
.await
if let Some(link) =
di.document_span.to_link(line_index, language_server).await
{
location_links.push(link);
}
@ -739,18 +725,12 @@ pub struct FileTextChanges {
}
impl FileTextChanges {
pub async fn to_text_document_edit<F, Fut, V>(
pub(crate) async fn to_text_document_edit(
&self,
index_provider: &F,
version_provider: &V,
) -> Result<lsp::TextDocumentEdit, AnyError>
where
F: Fn(ModuleSpecifier) -> Fut + Clone,
Fut: Future<Output = Result<LineIndex, AnyError>>,
V: Fn(ModuleSpecifier) -> Option<i32>,
{
language_server: &mut language_server::Inner,
) -> Result<lsp::TextDocumentEdit, AnyError> {
let specifier = ModuleSpecifier::resolve_url(&self.file_name)?;
let line_index = index_provider(specifier.clone()).await?;
let line_index = language_server.get_line_index(specifier.clone()).await?;
let edits = self
.text_changes
.iter()
@ -759,7 +739,7 @@ impl FileTextChanges {
Ok(lsp::TextDocumentEdit {
text_document: lsp::OptionalVersionedTextDocumentIdentifier {
uri: specifier.as_url().clone(),
version: version_provider(specifier),
version: language_server.document_version(specifier),
},
edits,
})
@ -1043,7 +1023,7 @@ fn get_length(state: &mut State, args: Value) -> Result<Value, AnyError> {
.unwrap();
Ok(json!(content.encode_utf16().count()))
} else {
let sources = &state.state_snapshot.sources;
let sources = &mut state.state_snapshot.sources;
Ok(json!(sources.get_length_utf16(&specifier).unwrap()))
}
}
@ -1068,7 +1048,7 @@ fn get_text(state: &mut State, args: Value) -> Result<Value, AnyError> {
.unwrap()
.clone()
} else {
let sources = &state.state_snapshot.sources;
let sources = &mut state.state_snapshot.sources;
sources.get_text(&specifier).unwrap()
};
Ok(json!(text::slice(&content, v.start..v.end)))
@ -1078,7 +1058,7 @@ fn resolve(state: &mut State, args: Value) -> Result<Value, AnyError> {
let v: ResolveArgs = serde_json::from_value(args)?;
let mut resolved = Vec::<Option<(String, String)>>::new();
let referrer = ModuleSpecifier::resolve_url(&v.base)?;
let sources = &state.state_snapshot.sources;
let sources = &mut state.state_snapshot.sources;
if state.state_snapshot.documents.contains(&referrer) {
if let Some(dependencies) =
@ -1172,7 +1152,7 @@ fn script_version(state: &mut State, args: Value) -> Result<Value, AnyError> {
if let Some(version) = state.state_snapshot.documents.version(&specifier) {
return Ok(json!(version.to_string()));
} else {
let sources = &state.state_snapshot.sources;
let sources = &mut state.state_snapshot.sources;
if let Some(version) = sources.get_script_version(&specifier) {
return Ok(json!(version));
}