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.

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

Change default formatter for any language #2942

Merged
merged 21 commits into from
Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
163d262
Change default formatter for any language
PiergiorgioZagaria Jul 2, 2022
9965cd4
Fix clippy error
PiergiorgioZagaria Jul 2, 2022
1af38a0
Close stdin for Stdio formatters
PiergiorgioZagaria Jul 2, 2022
29e24b6
Better indentation and pattern matching
PiergiorgioZagaria Jul 2, 2022
124dd5d
Return Result<Option<...>> for fn format instead of Option
PiergiorgioZagaria Jul 2, 2022
b5dc579
Remove unwrap for stdin
PiergiorgioZagaria Jul 2, 2022
2e70770
Handle FormatterErrors instead of Result<Option<...>>
PiergiorgioZagaria Jul 3, 2022
06ad531
Use Transaction instead of LspFormatting
PiergiorgioZagaria Jul 3, 2022
7416d6f
Use Transaction directly in Document::format
sudormrfbin Jul 3, 2022
b819c21
Perform stdin type formatting asynchronously
sudormrfbin Jul 4, 2022
48d69b2
Rename formatter.type values to kebab-case
sudormrfbin Jul 4, 2022
15685fe
Debug format for displaying io::ErrorKind (msrv fix)
sudormrfbin Jul 4, 2022
57bb4fc
Solve conflict?
PiergiorgioZagaria Jul 5, 2022
016e724
Merge branch 'master' into master
PiergiorgioZagaria Jul 6, 2022
f7ecf9c
Use only stdio type formatters
PiergiorgioZagaria Jul 8, 2022
acdce30
Remove FormatterType enum
PiergiorgioZagaria Jul 12, 2022
aeb4f8f
Remove old comment
PiergiorgioZagaria Jul 22, 2022
d711be0
Check if the formatter exited correctly
PiergiorgioZagaria Jul 23, 2022
7ec0288
Add formatter configuration to the book
PiergiorgioZagaria Jul 23, 2022
1ae13bb
Avoid allocations when writing to stdin and formatting errors
PiergiorgioZagaria Aug 2, 2022
fb93d1e
Remove unused import
PiergiorgioZagaria Aug 2, 2022
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
2 changes: 2 additions & 0 deletions book/src/languages.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ file-types = ["mylang", "myl"]
comment-token = "#"
indent = { tab-width = 2, unit = " " }
language-server = { command = "mylang-lsp", args = ["--stdio"] }
formatter = { command = "mylang-formatter" , args = ["--stdin"] }
```

These configuration keys are available:
Expand All @@ -59,6 +60,7 @@ These configuration keys are available:
| `language-server` | The Language Server to run. See the Language Server configuration section below. |
| `config` | Language Server configuration |
| `grammar` | The tree-sitter grammar to use (defaults to the value of `name`) |
| `formatter` | The formatter for the language, it will take precedence over the lsp when defined. The formatter must be able to take the original file as input from stdin and write the formatted file to stdout |

### Language Server configuration

Expand Down
12 changes: 12 additions & 0 deletions helix-core/src/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ pub struct LanguageConfiguration {
#[serde(default)]
pub auto_format: bool,

#[serde(skip_serializing_if = "Option::is_none")]
pub formatter: Option<FormatterConfiguration>,

#[serde(default)]
pub diagnostic_severity: Severity,

Expand Down Expand Up @@ -126,6 +129,15 @@ pub struct LanguageServerConfiguration {
pub language_id: Option<String>,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub struct FormatterConfiguration {
pub command: String,
#[serde(default)]
#[serde(skip_serializing_if = "Vec::is_empty")]
pub args: Vec<String>,
}

#[derive(Debug, PartialEq, Clone, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub struct AdvancedCompletion {
Expand Down
16 changes: 0 additions & 16 deletions helix-lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,22 +205,6 @@ pub mod util {
}),
)
}

/// The result of asking the language server to format the document. This can be turned into a
/// `Transaction`, but the advantage of not doing that straight away is that this one is
/// `Send` and `Sync`.
#[derive(Clone, Debug)]
pub struct LspFormatting {
pub doc: Rope,
pub edits: Vec<lsp::TextEdit>,
pub offset_encoding: OffsetEncoding,
}

impl From<LspFormatting> for Transaction {
fn from(fmt: LspFormatting) -> Transaction {
generate_transaction_from_edits(&fmt.doc, fmt.edits, fmt.offset_encoding)
}
}
}

#[derive(Debug, PartialEq, Clone)]
Expand Down
8 changes: 4 additions & 4 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use helix_core::{
};
use helix_view::{
clipboard::ClipboardType,
document::{Mode, SCRATCH_BUFFER_NAME},
document::{FormatterError, Mode, SCRATCH_BUFFER_NAME},
editor::{Action, Motion},
info::Info,
input::KeyEvent,
Expand Down Expand Up @@ -2372,14 +2372,14 @@ async fn make_format_callback(
doc_id: DocumentId,
doc_version: i32,
modified: Modified,
format: impl Future<Output = helix_lsp::util::LspFormatting> + Send + 'static,
format: impl Future<Output = Result<Transaction, FormatterError>> + Send + 'static,
) -> anyhow::Result<job::Callback> {
let format = format.await;
let format = format.await?;
let call: job::Callback = Box::new(move |editor, _compositor| {
let view_id = view!(editor).id;
if let Some(doc) = editor.document_mut(doc_id) {
if doc.version() == doc_version {
doc.apply(&Transaction::from(format), view_id);
doc.apply(&format, view_id);
doc.append_changes_to_history(view_id);
doc.detect_indent_and_line_ending();
if let Modified::SetUnmodified = modified {
Expand Down
103 changes: 93 additions & 10 deletions helix-view/src/document.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use anyhow::{anyhow, bail, Context, Error};
use futures_util::future::BoxFuture;
use futures_util::FutureExt;
use helix_core::auto_pairs::AutoPairs;
use helix_core::Range;
use serde::de::{self, Deserialize, Deserializer};
Expand All @@ -20,7 +22,6 @@ use helix_core::{
ChangeSet, Diagnostic, LineEnding, Rope, RopeBuilder, Selection, State, Syntax, Transaction,
DEFAULT_LINE_ENDING,
};
use helix_lsp::util::LspFormatting;

use crate::{DocumentId, Editor, ViewId};

Expand Down Expand Up @@ -397,7 +398,7 @@ impl Document {

/// The same as [`format`], but only returns formatting changes if auto-formatting
/// is configured.
pub fn auto_format(&self) -> Option<impl Future<Output = LspFormatting> + 'static> {
pub fn auto_format(&self) -> Option<BoxFuture<'static, Result<Transaction, FormatterError>>> {
if self.language_config()?.auto_format {
self.format()
} else {
Expand All @@ -407,7 +408,56 @@ impl Document {

/// If supported, returns the changes that should be applied to this document in order
/// to format it nicely.
pub fn format(&self) -> Option<impl Future<Output = LspFormatting> + 'static> {
// We can't use anyhow::Result here since the output of the future has to be
// clonable to be used as shared future. So use a custom error type.
pub fn format(&self) -> Option<BoxFuture<'static, Result<Transaction, FormatterError>>> {
if let Some(formatter) = self.language_config().and_then(|c| c.formatter.clone()) {
use std::process::Stdio;
let text = self.text().clone();
let mut process = tokio::process::Command::new(&formatter.command);
process
.args(&formatter.args)
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.stderr(Stdio::piped());

let formatting_future = async move {
let mut process = process
.spawn()
.map_err(|e| FormatterError::SpawningFailed {
command: formatter.command.clone(),
error: e.kind(),
})?;
{
let mut stdin = process.stdin.take().ok_or(FormatterError::BrokenStdin)?;
to_writer(&mut stdin, encoding::UTF_8, &text)
.await
.map_err(|_| FormatterError::BrokenStdin)?;
}

let output = process
.wait_with_output()
.await
.map_err(|_| FormatterError::WaitForOutputFailed)?;

if !output.stderr.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Stderr doesn't always mean there's an error. There are programs that use it to print diagnostic messages so that e.g. you can still pipe stdout to a file and see informational messages in the output. The only indication of error we can really use reliably is the exit code. What we should probably do is move this whole if expression inside the following one that checks the exit code. The enum variants can probably change the enum to get rid of Stderr and add an Option<String> to NonZeroExitStatus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll open a new PR to fix it.
Btw the function shell_impl in helix-term/src/commands.rs, which is used when calling the shell with ! Or | doesn't implement these checks, should we add them there too?

Copy link
Member

@dead10ck dead10ck Aug 7, 2022

Choose a reason for hiding this comment

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

I see, in that case, yeah, those need to be fixed too. The presence of output on stderr does not necessarily mean there was an error.

return Err(FormatterError::Stderr(
String::from_utf8_lossy(&output.stderr).to_string(),
));
}
PiergiorgioZagaria marked this conversation as resolved.
Show resolved Hide resolved

if !output.status.success() {
return Err(FormatterError::NonZeroExitStatus);
}

let str = String::from_utf8(output.stdout)
Copy link
Member

@kirawi kirawi Aug 8, 2022

Choose a reason for hiding this comment

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

You can use from_reader here instead. I think it is more efficient. Or str::from_utf8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std::str::from_utf8 works, but str::from_utf8 doesn't. Btw for further fixes to this part you can comment on #3349

.map_err(|_| FormatterError::InvalidUtf8Output)?;

Ok(helix_core::diff::compare_ropes(&text, &Rope::from(str)))
};
return Some(formatting_future.boxed());
};

let language_server = self.language_server()?;
let text = self.text.clone();
let offset_encoding = language_server.offset_encoding();
Expand All @@ -427,13 +477,13 @@ impl Document {
log::warn!("LSP formatting failed: {}", e);
Default::default()
});
LspFormatting {
doc: text,
Ok(helix_lsp::util::generate_transaction_from_edits(
&text,
edits,
offset_encoding,
}
))
};
Some(fut)
Some(fut.boxed())
}

pub fn save(&mut self, force: bool) -> impl Future<Output = Result<(), anyhow::Error>> {
Expand All @@ -442,7 +492,7 @@ impl Document {

pub fn format_and_save(
&mut self,
formatting: Option<impl Future<Output = LspFormatting>>,
formatting: Option<impl Future<Output = Result<Transaction, FormatterError>>>,
force: bool,
) -> impl Future<Output = anyhow::Result<()>> {
self.save_impl(formatting, force)
Expand All @@ -454,7 +504,7 @@ impl Document {
/// at its `path()`.
///
/// If `formatting` is present, it supplies some changes that we apply to the text before saving.
fn save_impl<F: Future<Output = LspFormatting>>(
fn save_impl<F: Future<Output = Result<Transaction, FormatterError>>>(
&mut self,
formatting: Option<F>,
force: bool,
Expand Down Expand Up @@ -488,7 +538,8 @@ impl Document {
}

if let Some(fmt) = formatting {
let success = Transaction::from(fmt.await).changes().apply(&mut text);
let transaction = fmt.await?;
let success = transaction.changes().apply(&mut text);
if !success {
// This shouldn't happen, because the transaction changes were generated
// from the same text we're saving.
Expand Down Expand Up @@ -1034,6 +1085,38 @@ impl Default for Document {
}
}

#[derive(Clone, Debug)]
pub enum FormatterError {
SpawningFailed {
command: String,
error: std::io::ErrorKind,
},
BrokenStdin,
WaitForOutputFailed,
Stderr(String),
InvalidUtf8Output,
DiskReloadError(String),
NonZeroExitStatus,
}

impl std::error::Error for FormatterError {}

impl Display for FormatterError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::SpawningFailed { command, error } => {
write!(f, "Failed to spawn formatter {}: {:?}", command, error)
}
Self::BrokenStdin => write!(f, "Could not write to formatter stdin"),
Self::WaitForOutputFailed => write!(f, "Waiting for formatter output failed"),
Self::Stderr(output) => write!(f, "Formatter error: {}", output),
Self::InvalidUtf8Output => write!(f, "Invalid UTF-8 formatter output"),
Self::DiskReloadError(error) => write!(f, "Error reloading file from disk: {}", error),
Self::NonZeroExitStatus => write!(f, "Formatter exited with non zero exit status:"),
}
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
3 changes: 1 addition & 2 deletions helix-view/src/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,6 @@ impl Editor {
syn_loader: Arc<syntax::Loader>,
config: Box<dyn DynAccess<Config>>,
) -> Self {
let language_servers = helix_lsp::Registry::new();
let conf = config.load();
let auto_pairs = (&conf.auto_pairs).into();

Expand All @@ -547,7 +546,7 @@ impl Editor {
macro_recording: None,
macro_replaying: Vec::new(),
theme: theme_loader.default(),
language_servers,
language_servers: helix_lsp::Registry::new(),
diagnostics: BTreeMap::new(),
debugger: None,
debugger_events: SelectAll::new(),
Expand Down
2 changes: 2 additions & 0 deletions languages.toml
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,8 @@ auto-format = true
comment-token = "//"
language-server = { command = "zls" }
indent = { tab-width = 4, unit = " " }
formatter = { command = "zig" , args = ["fmt", "--stdin"] }


[[grammar]]
name = "zig"
Expand Down