Skip to content

Commit

Permalink
Fix runnables code lens in multi-file sway projects (#4984)
Browse files Browse the repository at this point in the history
## Description

Closes #4983

The issue was that we were returning all of the runnables for the
workspace, rather than just the file. We were also calling
`get_range_from_span` for every runnable at request time, which
@JoshuaBatty discovered is very costly.

- The runnables map is now mapping [file path] => [vector of runnables
for that file]
- Rather than iterating over all of the runnables in the session, we
just fetch the vector of runnables indexed by the file path.
- Runnables are now storing the range rather than the span, so we don't
need to convert it at request time.
- Added another test that expects _no_ runnables to be returned for a
file in a workspace that has runnables in other files. This test was
failing before the change. Previously, the runnables test was running on
test files from the e2e directory, so I copied that into the LSP
fixtures.
- Moved the code_lens logic into a capability and updated the bencher

## Checklist

- [x] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.
  • Loading branch information
sdankel authored Aug 23, 2023
1 parent 73b02df commit 6387087
Show file tree
Hide file tree
Showing 12 changed files with 119 additions and 60 deletions.
16 changes: 3 additions & 13 deletions sway-lsp/benches/lsp_benchmarks/requests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use criterion::{black_box, criterion_group, Criterion};
use lsp_types::{
CodeLens, CompletionResponse, DocumentSymbolResponse, Position, Range,
TextDocumentContentChangeEvent, TextDocumentIdentifier,
CompletionResponse, DocumentSymbolResponse, Position, Range, TextDocumentContentChangeEvent,
TextDocumentIdentifier,
};
use sway_lsp::{capabilities, lsp_ext::OnEnterParams, utils::keyword_docs::KeywordDocs};

Expand Down Expand Up @@ -79,17 +79,7 @@ fn benchmarks(c: &mut Criterion) {
});

c.bench_function("code_lens", |b| {
b.iter(|| {
let mut result = vec![];
session.runnables.iter().for_each(|item| {
let runnable = item.value();
result.push(CodeLens {
range: runnable.range(),
command: Some(runnable.command()),
data: None,
});
});
})
b.iter(|| capabilities::code_lens::code_lens(&session, &uri.clone()))
});

c.bench_function("on_enter", |b| {
Expand Down
27 changes: 27 additions & 0 deletions sway-lsp/src/capabilities/code_lens.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
use std::{path::PathBuf, sync::Arc};

use lsp_types::{CodeLens, Url};

use crate::core::session::Session;

pub fn code_lens(session: &Arc<Session>, url: &Url) -> Vec<CodeLens> {
let url_path = PathBuf::from(url.path());

// Construct code lenses for runnable functions
let runnables_for_path = session.runnables.get(&url_path);
let mut result: Vec<CodeLens> = runnables_for_path
.map(|runnables| {
runnables
.iter()
.map(|runnable| CodeLens {
range: *runnable.range(),
command: Some(runnable.command()),
data: None,
})
.collect()
})
.unwrap_or_default();
// Sort the results
result.sort_by(|a, b| a.range.start.line.cmp(&b.range.start.line));
result
}
1 change: 1 addition & 0 deletions sway-lsp/src/capabilities/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pub mod code_actions;
pub mod code_lens;
pub mod completion;
pub mod diagnostic;
pub mod document_symbol;
Expand Down
22 changes: 8 additions & 14 deletions sway-lsp/src/capabilities/runnable.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
use crate::core::token::get_range_from_span;
use lsp_types::{Command, Range};
use serde_json::{json, Value};
use sway_core::language::parsed::TreeType;
use sway_types::Span;

#[derive(Debug, Eq, PartialEq, Clone)]
pub struct RunnableMainFn {
/// The location in the file where the runnable button should be displayed
pub span: Span,
pub range: Range,
/// The program kind of the current file
pub tree_type: TreeType,
}

#[derive(Debug, Eq, PartialEq, Clone)]
pub struct RunnableTestFn {
/// The location in the file where the runnable button should be displayed.
pub span: Span,
/// The location in the file where the runnable button should be displayed
pub range: Range,
/// The program kind of the current file.
pub tree_type: TreeType,
/// Additional arguments to use with the runnable command.
Expand All @@ -38,12 +36,8 @@ pub trait Runnable: core::fmt::Debug + Send + Sync + 'static {
fn label_string(&self) -> String;
/// The arguments to pass to the command.
fn arguments(&self) -> Option<Vec<Value>>;
/// The span where the runnable button should be displayed.
fn span(&self) -> &Span;
/// The range in the file where the runnable button should be displayed.
fn range(&self) -> Range {
get_range_from_span(self.span())
}
fn range(&self) -> &Range;
}

impl Runnable for RunnableMainFn {
Expand All @@ -56,8 +50,8 @@ impl Runnable for RunnableMainFn {
fn arguments(&self) -> Option<Vec<Value>> {
None
}
fn span(&self) -> &Span {
&self.span
fn range(&self) -> &Range {
&self.range
}
}

Expand All @@ -73,7 +67,7 @@ impl Runnable for RunnableTestFn {
.as_ref()
.map(|test_name| vec![json!({ "name": test_name })])
}
fn span(&self) -> &Span {
&self.span
fn range(&self) -> &Range {
&self.range
}
}
53 changes: 38 additions & 15 deletions sway-lsp/src/core/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ use sway_core::{
},
BuildTarget, Engines, Namespace, Programs,
};
use sway_types::{Span, Spanned};
use sway_types::{SourceEngine, Spanned};
use sway_utils::helpers::get_sway_files;
use tokio::sync::Semaphore;

use super::token::get_range_from_span;

pub type Documents = DashMap<String, TextDocument>;
pub type ProjectDirectory = PathBuf;

Expand Down Expand Up @@ -75,7 +77,7 @@ pub struct ParseResult {
pub struct Session {
token_map: TokenMap,
pub documents: Documents,
pub runnables: DashMap<Span, Box<dyn Runnable>>,
pub runnables: DashMap<PathBuf, Vec<Box<dyn Runnable>>>,
pub compiled_program: RwLock<CompiledProgram>,
pub engines: RwLock<Engines>,
pub sync: SyncWorkspace,
Expand Down Expand Up @@ -154,7 +156,11 @@ impl Session {
self.token_map.insert(s.clone(), t.clone());
});

self.create_runnables(&res.typed, self.engines.read().de());
self.create_runnables(
&res.typed,
self.engines.read().de(),
self.engines.read().se(),
);
self.compiled_program.write().lexed = Some(res.lexed);
self.compiled_program.write().parsed = Some(res.parsed);
self.compiled_program.write().typed = Some(res.typed);
Expand Down Expand Up @@ -332,20 +338,31 @@ impl Session {
}

/// Create runnables if the `TyProgramKind` of the `TyProgram` is a script.
fn create_runnables(&self, typed_program: &ty::TyProgram, decl_engine: &DeclEngine) {
fn create_runnables(
&self,
typed_program: &ty::TyProgram,
decl_engine: &DeclEngine,
source_engine: &SourceEngine,
) {
// Insert runnable test functions.
for (decl, _) in typed_program.test_fns(decl_engine) {
// Get the span of the first attribute if it exists, otherwise use the span of the function name.
let span = decl
.attributes
.first()
.map_or_else(|| decl.name.span(), |(_, attr)| attr.span.clone());
let runnable = Box::new(RunnableTestFn {
span,
tree_type: typed_program.kind.tree_type(),
test_name: Some(decl.name.to_string()),
});
self.runnables.insert(runnable.span().clone(), runnable);
if let Some(source_id) = span.source_id() {
let path = source_engine.get_path(source_id);
let runnable = Box::new(RunnableTestFn {
range: get_range_from_span(&span.clone()),
tree_type: typed_program.kind.tree_type(),
test_name: Some(decl.name.to_string()),
});
self.runnables
.entry(path)
.or_insert(Vec::new())
.push(runnable);
}
}

// Insert runnable main function if the program is a script.
Expand All @@ -354,11 +371,17 @@ impl Session {
} = typed_program.kind
{
let span = main_function.name.span();
let runnable = Box::new(RunnableMainFn {
span,
tree_type: typed_program.kind.tree_type(),
});
self.runnables.insert(runnable.span().clone(), runnable);
if let Some(source_id) = span.source_id() {
let path = source_engine.get_path(source_id);
let runnable = Box::new(RunnableMainFn {
range: get_range_from_span(&span.clone()),
tree_type: typed_program.kind.tree_type(),
});
self.runnables
.entry(path)
.or_insert(Vec::new())
.push(runnable);
}
}
}

Expand Down
16 changes: 1 addition & 15 deletions sway-lsp/src/handlers/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,25 +240,11 @@ pub fn handle_code_lens(
state: &ServerState,
params: lsp_types::CodeLensParams,
) -> Result<Option<Vec<CodeLens>>> {
let mut result = vec![];
match state
.sessions
.uri_and_session_from_workspace(&params.text_document.uri)
{
Ok((_, session)) => {
// Construct code lenses for runnable functions
session.runnables.iter().for_each(|item| {
let runnable = item.value();
result.push(CodeLens {
range: runnable.range(),
command: Some(runnable.command()),
data: None,
});
});
// Sort the results
result.sort_by(|a, b| a.range.start.line.cmp(&b.range.start.line));
Ok(Some(result))
}
Ok((url, session)) => Ok(Some(capabilities::code_lens::code_lens(&session, &url))),
Err(err) => {
tracing::error!("{}", err.to_string());
Ok(None)
Expand Down
8 changes: 8 additions & 0 deletions sway-lsp/tests/fixtures/runnables/Forc.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[project]
authors = ["Fuel Labs <[email protected]>"]
entry = "main.sw"
license = "Apache-2.0"
name = "script_multi_test"

[dependencies]
std = { path = "../../../../sway-lib-std" }
16 changes: 16 additions & 0 deletions sway-lsp/tests/fixtures/runnables/src/main.sw
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
script;

fn main() {
revert(0);
}

#[test]
fn test_foo() {
assert(true);
}

#[test]
fn test_bar() {
log("test");
assert(4 / 2 == 2);
}
1 change: 1 addition & 0 deletions sway-lsp/tests/fixtures/runnables/src/other.sw
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
library;
10 changes: 10 additions & 0 deletions sway-lsp/tests/integration/lsp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,16 @@ pub(crate) fn highlight_request(server: &ServerState, uri: &Url) {
assert_eq!(expected, response.unwrap());
}

pub(crate) fn code_lens_empty_request(server: &ServerState, uri: &Url) {
let params = CodeLensParams {
text_document: TextDocumentIdentifier { uri: uri.clone() },
work_done_progress_params: Default::default(),
partial_result_params: Default::default(),
};
let response = request::handle_code_lens(server, params.clone()).unwrap();
assert_eq!(response.unwrap().len(), 0);
}

pub(crate) fn code_lens_request(server: &ServerState, uri: &Url) {
let params = CodeLensParams {
text_document: TextDocumentIdentifier { uri: uri.clone() },
Expand Down
5 changes: 5 additions & 0 deletions sway-lsp/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1723,6 +1723,11 @@ lsp_capability_test!(
lsp::code_lens_request,
runnables_test_dir().join("src/main.sw")
);
lsp_capability_test!(
code_lens_empty,
lsp::code_lens_empty_request,
runnables_test_dir().join("src/other.sw")
);
lsp_capability_test!(
completion,
lsp::completion_request,
Expand Down
4 changes: 1 addition & 3 deletions sway-lsp/tests/utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ pub fn e2e_test_dir() -> PathBuf {
}

pub fn runnables_test_dir() -> PathBuf {
sway_workspace_dir()
.join(e2e_unit_dir())
.join("script_multi_test")
test_fixtures_dir().join("runnables")
}

pub fn test_fixtures_dir() -> PathBuf {
Expand Down

0 comments on commit 6387087

Please sign in to comment.