Skip to content

Synchronize code_lens request & provide better UX for forc-fmt <> sway-lsp interaction. #5094

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

Merged
merged 17 commits into from
Sep 15, 2023
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
8 changes: 5 additions & 3 deletions Cargo.lock

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

1 change: 0 additions & 1 deletion forc-pkg/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ repository.workspace = true
ansi_term = "0.12"
anyhow = "1"
cid = "0.10"
fd-lock = "3.0"
forc-tracing = { version = "0.46.0", path = "../forc-tracing" }
forc-util = { version = "0.46.0", path = "../forc-util" }
fuel-abi-types = "0.1"
Expand Down
46 changes: 2 additions & 44 deletions forc-pkg/src/pkg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
use anyhow::{anyhow, bail, Context, Error, Result};
use forc_util::{
default_output_directory, find_file_name, kebab_to_snake_case, print_compiling,
print_on_failure, print_warnings, user_forc_directory,
print_on_failure, print_warnings,
};
use fuel_abi_types::program_abi;
use petgraph::{
Expand All @@ -26,7 +26,6 @@ use std::{
str::FromStr,
sync::Arc,
};
use sway_core::fuel_prelude::fuel_types::ChainId;
pub use sway_core::Programs;
use sway_core::{
abi_generation::{
Expand All @@ -38,6 +37,7 @@ use sway_core::{
fuel_prelude::{
fuel_crypto,
fuel_tx::{self, Contract, ContractId, StorageSlot},
fuel_types::ChainId,
},
language::{parsed::TreeType, Visibility},
semantic_analysis::namespace,
Expand Down Expand Up @@ -1516,48 +1516,6 @@ fn fetch_deps(
Ok(added)
}

/// Given a path to a directory we wish to lock, produce a path for an associated lock file.
///
/// Note that the lock file itself is simply a placeholder for co-ordinating access. As a result,
/// we want to create the lock file if it doesn't exist, but we can never reliably remove it
/// without risking invalidation of an existing lock. As a result, we use a dedicated, hidden
/// directory with a lock file named after the checkout path.
///
/// Note: This has nothing to do with `Forc.lock` files, rather this is about fd locks for
/// coordinating access to particular paths (e.g. git checkout directories).
fn fd_lock_path(path: &Path) -> PathBuf {
const LOCKS_DIR_NAME: &str = ".locks";
const LOCK_EXT: &str = "forc-lock";

// Hash the path to produce a file-system friendly lock file name.
// Append the file stem for improved readability.
let mut hasher = hash_map::DefaultHasher::default();
path.hash(&mut hasher);
let hash = hasher.finish();
let file_name = match path.file_stem().and_then(|s| s.to_str()) {
None => format!("{hash:X}"),
Some(stem) => format!("{hash:X}-{stem}"),
};

user_forc_directory()
.join(LOCKS_DIR_NAME)
.join(file_name)
.with_extension(LOCK_EXT)
}

/// Create an advisory lock over the given path.
///
/// See [fd_lock_path] for details.
pub(crate) fn path_lock(path: &Path) -> Result<fd_lock::RwLock<File>> {
let lock_path = fd_lock_path(path);
let lock_dir = lock_path
.parent()
.expect("lock path has no parent directory");
std::fs::create_dir_all(lock_dir).context("failed to create forc advisory lock directory")?;
let lock_file = File::create(&lock_path).context("failed to create advisory lock file")?;
Ok(fd_lock::RwLock::new(lock_file))
}

/// Given a `forc_pkg::BuildProfile`, produce the necessary `sway_core::BuildConfig` required for
/// compilation.
pub fn sway_build_config(
Expand Down
4 changes: 2 additions & 2 deletions forc-pkg/src/source/git/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ impl source::Pin for Source {
impl source::Fetch for Pinned {
fn fetch(&self, ctx: source::PinCtx, repo_path: &Path) -> Result<PackageManifestFile> {
// Co-ordinate access to the git checkout directory using an advisory file lock.
let mut lock = crate::pkg::path_lock(repo_path)?;
let mut lock = forc_util::path_lock(repo_path)?;
// TODO: Here we assume that if the local path already exists, that it contains the
// full and correct source for that commit and hasn't been tampered with. This is
// probably fine for most cases as users should never be touching these
Expand Down Expand Up @@ -217,7 +217,7 @@ impl source::DepPath for Pinned {
fn dep_path(&self, name: &str) -> anyhow::Result<source::DependencyPath> {
let repo_path = commit_path(name, &self.source.repo, &self.commit_hash);
// Co-ordinate access to the git checkout directory using an advisory file lock.
let lock = crate::pkg::path_lock(&repo_path)?;
let lock = forc_util::path_lock(&repo_path)?;
let _guard = lock.read()?;
let path = manifest::find_within(&repo_path, name)
.ok_or_else(|| anyhow!("failed to find package `{}` in {}", name, self))?;
Expand Down
4 changes: 2 additions & 2 deletions forc-pkg/src/source/ipfs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl source::Fetch for Pinned {
anyhow::bail!("offline fetching for IPFS sources is not supported")
}

let mut lock = crate::pkg::path_lock(repo_path)?;
let mut lock = forc_util::path_lock(repo_path)?;
{
let _guard = lock.write()?;
if !repo_path.exists() {
Expand Down Expand Up @@ -111,7 +111,7 @@ impl source::DepPath for Pinned {
fn dep_path(&self, name: &str) -> anyhow::Result<source::DependencyPath> {
let repo_path = pkg_cache_dir(&self.0);
// Co-ordinate access to the ipfs checkout directory using an advisory file lock.
let lock = crate::pkg::path_lock(&repo_path)?;
let lock = forc_util::path_lock(&repo_path)?;
let _guard = lock.read()?;
let path = manifest::find_within(&repo_path, name)
.ok_or_else(|| anyhow::anyhow!("failed to find package `{}` in {}", name, self))?;
Expand Down
20 changes: 18 additions & 2 deletions forc-plugins/forc-fmt/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ fn run() -> Result<()> {
};

let manifest_file = forc_pkg::manifest::ManifestFile::from_dir(&dir)?;

match manifest_file {
ManifestFile::Workspace(ws) => {
format_workspace_at_dir(&app, &ws, &dir)?;
Expand All @@ -87,10 +86,19 @@ fn run() -> Result<()> {
format_pkg_at_dir(&app, &dir, &mut formatter)?;
}
}

Ok(())
}

/// Checks if the specified file is marked as "dirty".
/// This is used to prevent formatting files that are currently open in an editor
/// with unsaved changes.
///
/// Returns `true` if a corresponding "dirty" flag file exists, `false` otherwise.
fn is_file_dirty(path: &Path) -> bool {
let dirty_file_path = forc_util::is_dirty_path(path);
dirty_file_path.exists()
}

/// Recursively get a Vec<PathBuf> of subdirectories that contains a Forc.toml.
fn get_sway_dirs(workspace_dir: PathBuf) -> Vec<PathBuf> {
let mut dirs_to_format = vec![];
Expand Down Expand Up @@ -126,6 +134,14 @@ fn format_file(
formatter: &mut Formatter,
) -> Result<bool> {
let file = file.canonicalize()?;
if is_file_dirty(&file) {
bail!(
"The below file is open in an editor and contains unsaved changes.\n \
Please save it before formatting.\n \
{}",
file.display()
);
}
if let Ok(file_content) = fs::read_to_string(&file) {
let mut edited = false;
let file_content: Arc<str> = Arc::from(file_content);
Expand Down
1 change: 1 addition & 0 deletions forc-util/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ ansi_term = "0.12"
anyhow = "1"
clap = { version = "3.1", features = ["cargo", "derive", "env"] }
dirs = "3.0.2"
fd-lock = "4.0"
forc-tracing = { version = "0.46.0", path = "../forc-tracing" }
fuel-tx = { workspace = true, features = ["serde"], optional = true }
hex = "0.4.3"
Expand Down
67 changes: 65 additions & 2 deletions forc-util/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,17 @@ use annotate_snippets::{
snippet::{Annotation, AnnotationType, Slice, Snippet, SourceAnnotation},
};
use ansi_term::Colour;
use anyhow::{bail, Result};
use anyhow::{bail, Context, Result};
use forc_tracing::{println_red_err, println_yellow_err};
use std::{collections::HashSet, str};
use std::{
collections::{hash_map, HashSet},
str,
};
use std::{ffi::OsStr, process::Termination};
use std::{
fmt::Display,
fs::File,
hash::{Hash, Hasher},
path::{Path, PathBuf},
};
use sway_core::language::parsed::TreeType;
Expand Down Expand Up @@ -349,6 +354,64 @@ pub fn git_checkouts_directory() -> PathBuf {
user_forc_directory().join("git").join("checkouts")
}

/// Given a path to a directory we wish to lock, produce a path for an associated lock file.
///
/// Note that the lock file itself is simply a placeholder for co-ordinating access. As a result,
/// we want to create the lock file if it doesn't exist, but we can never reliably remove it
/// without risking invalidation of an existing lock. As a result, we use a dedicated, hidden
/// directory with a lock file named after the checkout path.
///
/// Note: This has nothing to do with `Forc.lock` files, rather this is about fd locks for
/// coordinating access to particular paths (e.g. git checkout directories).
fn fd_lock_path(path: &Path) -> PathBuf {
const LOCKS_DIR_NAME: &str = ".locks";
const LOCK_EXT: &str = "forc-lock";
let file_name = hash_path(path);
user_forc_directory()
.join(LOCKS_DIR_NAME)
.join(file_name)
.with_extension(LOCK_EXT)
}

/// Constructs the path for the "dirty" flag file corresponding to the specified file.
///
/// This function uses a hashed representation of the original path for uniqueness.
pub fn is_dirty_path(path: &Path) -> PathBuf {
const LOCKS_DIR_NAME: &str = ".lsp-locks";
const LOCK_EXT: &str = "dirty";
let file_name = hash_path(path);
user_forc_directory()
.join(LOCKS_DIR_NAME)
.join(file_name)
.with_extension(LOCK_EXT)
}

/// Hash the path to produce a file-system friendly file name.
/// Append the file stem for improved readability.
fn hash_path(path: &Path) -> String {
let mut hasher = hash_map::DefaultHasher::default();
path.hash(&mut hasher);
let hash = hasher.finish();
let file_name = match path.file_stem().and_then(|s| s.to_str()) {
None => format!("{hash:X}"),
Some(stem) => format!("{hash:X}-{stem}"),
};
file_name
}

/// Create an advisory lock over the given path.
///
/// See [fd_lock_path] for details.
pub fn path_lock(path: &Path) -> Result<fd_lock::RwLock<File>> {
let lock_path = fd_lock_path(path);
let lock_dir = lock_path
.parent()
.expect("lock path has no parent directory");
std::fs::create_dir_all(lock_dir).context("failed to create forc advisory lock directory")?;
let lock_file = File::create(&lock_path).context("failed to create advisory lock file")?;
Ok(fd_lock::RwLock::new(lock_file))
}

pub fn program_type_str(ty: &TreeType) -> &'static str {
match ty {
TreeType::Script {} => "script",
Expand Down
2 changes: 2 additions & 0 deletions sway-lsp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ repository.workspace = true
[dependencies]
anyhow = "1.0.41"
dashmap = "5.4"
fd-lock = "4.0"
forc-pkg = { version = "0.46.0", path = "../forc-pkg" }
forc-tracing = { version = "0.46.0", path = "../forc-tracing" }
forc-util = { version = "0.46.0", path = "../forc-util" }
lsp-types = { version = "0.94", features = ["proposed"] }
notify = "5.0.0"
notify-debouncer-mini = { version = "0.2.0" }
Expand Down
2 changes: 2 additions & 0 deletions sway-lsp/benches/lsp_benchmarks/requests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ fn benchmarks(c: &mut Criterion) {
};
b.iter(|| capabilities::on_enter::on_enter(&config.on_enter, &session, &uri, &params))
});

c.bench_function("format", |b| b.iter(|| session.format_text(&uri)));
}

criterion_group! {
Expand Down
42 changes: 39 additions & 3 deletions sway-lsp/src/core/document.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
#![allow(dead_code)]
use lsp_types::{Position, Range, TextDocumentContentChangeEvent};
use crate::{
error::{DirectoryError, DocumentError, LanguageServerError},
utils::document,
};
use lsp_types::{Position, Range, TextDocumentContentChangeEvent, Url};
use ropey::Rope;

use crate::error::DocumentError;

#[derive(Debug, Clone)]
pub struct TextDocument {
#[allow(dead_code)]
Expand Down Expand Up @@ -104,6 +106,40 @@ impl TextDocument {
}
}

/// Marks the specified file as "dirty" by creating a corresponding flag file.
///
/// This function ensures the necessary directory structure exists before creating the flag file.
pub fn mark_file_as_dirty(uri: &Url) -> Result<(), LanguageServerError> {
let path = document::get_path_from_url(uri)?;
let dirty_file_path = forc_util::is_dirty_path(&path);
if let Some(dir) = dirty_file_path.parent() {
// Ensure the directory exists
std::fs::create_dir_all(dir).map_err(|_| DirectoryError::LspLocksDirFailed)?;
}
// Create an empty "dirty" file
std::fs::File::create(&dirty_file_path).map_err(|err| DocumentError::UnableToCreateFile {
path: uri.path().to_string(),
err: err.to_string(),
})?;
Ok(())
}

/// Removes the corresponding flag file for the specifed Url.
///
/// If the flag file does not exist, this function will do nothing.
pub fn remove_dirty_flag(uri: &Url) -> Result<(), LanguageServerError> {
let path = document::get_path_from_url(uri)?;
let dirty_file_path = forc_util::is_dirty_path(&path);
if dirty_file_path.exists() {
// Remove the "dirty" file
std::fs::remove_file(dirty_file_path).map_err(|err| DocumentError::UnableToRemoveFile {
path: uri.path().to_string(),
err: err.to_string(),
})?;
}
Ok(())
}

#[derive(Debug)]
struct EditText<'text> {
start_index: usize,
Expand Down
4 changes: 4 additions & 0 deletions sway-lsp/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ pub enum DocumentError {
UnableToCreateFile { path: String, err: String },
#[error("Unable to write string to file at {:?} : {:?}", path, err)]
UnableToWriteFile { path: String, err: String },
#[error("File wasn't able to be removed at path {:?} : {:?}", path, err)]
UnableToRemoveFile { path: String, err: String },
}

#[derive(Debug, Error, PartialEq, Eq)]
Expand All @@ -50,6 +52,8 @@ pub enum DirectoryError {
ManifestDirNotFound,
#[error("Can't extract project name from {:?}", dir)]
CantExtractProjectName { dir: String },
#[error("Failed to create hidden .lsp_locks directory")]
LspLocksDirFailed,
#[error("Failed to create temp directory")]
TempDirFailed,
#[error("Failed to canonicalize path")]
Expand Down
Loading