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

Process comments (and extra nodes) separately from the rest of the code #500

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions examples/client-app/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ async fn main() {
let language: Language = Language {
name: "json".to_owned(),
query: TopiaryQuery::new(&grammar, query).unwrap(),
comment_query: None,
grammar,
indent: None,
};
Expand Down
78 changes: 70 additions & 8 deletions topiary-cli/src/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ pub struct InputFile<'cfg> {
source: InputSource,
language: &'cfg topiary_config::language::Language,
query: QuerySource,
comment_query: Option<QuerySource>,
}

impl<'cfg> InputFile<'cfg> {
Expand All @@ -125,9 +126,21 @@ impl<'cfg> InputFile<'cfg> {
};
let query = TopiaryQuery::new(&grammar, &contents)?;

// Can't use `map` because of closures, async, and Result.
let comment_contents = match &self.comment_query {
Some(QuerySource::Path(query)) => Some(tokio::fs::read_to_string(query).await?),
Some(QuerySource::BuiltIn(contents)) => Some(contents.to_owned()),
None => None,
};
let comment_query = match comment_contents {
Some(c) => Some(TopiaryQuery::new(&grammar, &c)?),
None => None,
};

Ok(Language {
name: self.language.name.clone(),
query,
comment_query,
grammar,
indent: self.language().config.indent.clone(),
})
Expand Down Expand Up @@ -178,17 +191,18 @@ impl<'cfg, 'i> Inputs<'cfg> {
InputFrom::Stdin(language_name, query) => {
vec![(|| {
let language = config.get_language(&language_name)?;
let query_source: QuerySource = match query {
let (query, comment_query) = match query {
// The user specified a query file
Some(p) => p,
Some(p) => (p, None),
Comment on lines 195 to +196
Copy link
Member

Choose a reason for hiding this comment

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

So if --query is used, then the comment query is None? That feels a bit surprising and that ought to be clearly documented.

Alternatively, what do you think about: If the query file is p, then the comment query file is:

  • Some(q), where q := replace(p, '${language}.scm', '${language}.comment.scm'), if q exists.
  • None, otherwise.

Similar to what you do in topiary-config/src/language.rs... Too magical?

// The user did not specify a file, try the default locations
None => to_query_from_language(language)?,
};

Ok(InputFile {
source: InputSource::Stdin,
language,
query: query_source,
query,
comment_query,
})
})()]
}
Expand All @@ -197,12 +211,13 @@ impl<'cfg, 'i> Inputs<'cfg> {
.into_iter()
.map(|path| {
let language = config.detect(&path)?;
let query: QuerySource = to_query_from_language(language)?;
let (query, comment_query) = to_query_from_language(language)?;

Ok(InputFile {
source: InputSource::Disk(path, None),
language,
query,
comment_query,
})
})
.collect(),
Expand All @@ -212,16 +227,21 @@ impl<'cfg, 'i> Inputs<'cfg> {
}
}

fn to_query_from_language(language: &topiary_config::language::Language) -> CLIResult<QuerySource> {
let query: QuerySource = match language.find_query_file() {
Ok(p) => p.into(),
fn to_query_from_language(
language: &topiary_config::language::Language,
) -> CLIResult<(QuerySource, Option<QuerySource>)> {
let query: (QuerySource, Option<QuerySource>) = match language.find_query_file() {
Ok((path, comment_path)) => (path.into(), comment_path.map(|p| p.into())),
// For some reason, Topiary could not find any
// matching file in a default location. As a final attempt, try the
// builtin ones. Store the error, return that if we
// fail to find anything, because the builtin error might be unexpected.
Err(e) => {
log::warn!("No query files found in any of the expected locations. Falling back to compile-time included files.");
to_query(&language.name).map_err(|_| e)?
(
to_query(&language.name).map_err(|_| e)?,
to_comment_query(&language.name)?,
)
}
};
Ok(query)
Expand Down Expand Up @@ -364,3 +384,45 @@ where
)),
}
}

fn to_comment_query<T>(name: T) -> CLIResult<Option<QuerySource>>
where
T: AsRef<str> + fmt::Display,
{
match name.as_ref() {
#[cfg(feature = "bash")]
"bash" => Ok(Some(topiary_queries::bash_comment().into())),

#[cfg(feature = "css")]
"css" => Ok(Some(topiary_queries::css_comment().into())),

#[cfg(feature = "json")]
"json" => Ok(None),

#[cfg(feature = "nickel")]
"nickel" => Ok(Some(topiary_queries::nickel_comment().into())),

#[cfg(feature = "ocaml")]
"ocaml" => Ok(Some(topiary_queries::ocaml_comment().into())),

#[cfg(feature = "ocaml_interface")]
"ocaml_interface" => Ok(Some(topiary_queries::ocaml_interface_comment().into())),

#[cfg(feature = "ocamllex")]
"ocamllex" => Ok(Some(topiary_queries::ocamllex_comment().into())),

#[cfg(feature = "rust")]
"rust" => Ok(Some(topiary_queries::rust_comment().into())),

#[cfg(feature = "toml")]
"toml" => Ok(Some(topiary_queries::toml_comment().into())),

#[cfg(feature = "tree_sitter_query")]
"tree_sitter_query" => Ok(Some(topiary_queries::tree_sitter_query_comment().into())),

name => Err(TopiaryError::Bin(
format!("The specified language is unsupported: {}", name),
Some(CLIError::UnsupportedLanguage(name.to_string())),
)),
}
}
1 change: 1 addition & 0 deletions topiary-cli/tests/samples/expected/nickel.ncl
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@
x,

# let blocks

let x = 1, y = 2 in x + y,
let
x = 1,
Expand Down
30 changes: 10 additions & 20 deletions topiary-cli/tests/samples/expected/ocaml-interface.mli
Original file line number Diff line number Diff line change
Expand Up @@ -437,8 +437,7 @@ module Gas : sig

val pp_cost_as_gas : Format.formatter -> cost -> unit

type error += Operation_quota_exceeded
(* `Temporary *)
type error += Operation_quota_exceeded (* `Temporary *)

(** [consume ctxt cost] subtracts [cost] to the current operation
gas level in [ctxt]. This operation may fail with
Expand All @@ -453,11 +452,9 @@ module Gas : sig
would fall below [0]. *)
val consume_from : Arith.fp -> cost -> Arith.fp tzresult

type error += Block_quota_exceeded
(* `Temporary *)
type error += Block_quota_exceeded (* `Temporary *)

type error += Gas_limit_too_high
(* `Permanent *)
type error += Gas_limit_too_high (* `Permanent *)

(** See {!Raw_context.consume_gas_limit_in_block}. *)
val consume_limit_in_block : context -> 'a Arith.t -> context tzresult
Expand Down Expand Up @@ -1398,11 +1395,9 @@ module Sapling : sig

val rpc_arg : t RPC_arg.arg

val parse_z : Z.t -> t
(* To be used in parse_data only *)
val parse_z : Z.t -> t (* To be used in parse_data only *)

val unparse_to_z : t -> Z.t
(* To be used in unparse_data only *)
val unparse_to_z : t -> Z.t (* To be used in unparse_data only *)
end

(** Create a fresh sapling state in the context. *)
Expand Down Expand Up @@ -4925,11 +4920,9 @@ module Operation : sig

val compare_by_passes : packed_operation -> packed_operation -> int

type error += Missing_signature
(* `Permanent *)
type error += Missing_signature (* `Permanent *)

type error += Invalid_signature
(* `Permanent *)
type error += Invalid_signature (* `Permanent *)

val check_signature : public_key -> Chain_id.t -> _ operation -> unit tzresult

Expand Down Expand Up @@ -5458,14 +5451,11 @@ module Fees : sig
Z.t ->
(context * Z.t * Receipt.balance_updates) tzresult Lwt.t

type error += Cannot_pay_storage_fee
(* `Temporary *)
type error += Cannot_pay_storage_fee (* `Temporary *)

type error += Operation_quota_exceeded
(* `Temporary *)
type error += Operation_quota_exceeded (* `Temporary *)

type error += Storage_limit_too_high
(* `Permanent *)
type error += Storage_limit_too_high (* `Permanent *)

val check_storage_limit : context -> storage_limit: Z.t -> unit tzresult
end
Expand Down
15 changes: 7 additions & 8 deletions topiary-cli/tests/samples/expected/ocaml.ml
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,10 @@ let id6 = function x -> x

(* Extensible buffers *)

type t = (* Multi-
* line comment with
* too much padding.
*)
{
type t = { (* Multi-
* line comment with
* too much padding.
*)
mutable buffer: bytes;
mutable position: int; (* End-of-line comment *)
mutable length: int;
Expand Down Expand Up @@ -838,12 +837,12 @@ let _ =
[@@@deprecated "writing code is deprecated, use ai-generated code instead"]

type t = {
verbose: int;
(** Verbosity level. *)
loggers: string;
verbose: int;
(** Loggers enabled. *)
loggers: string;
bflags: bool StrMap.t
(** Boolean flags. *)
(** Boolean flags. *)
}

let _ = {
Expand Down
24 changes: 14 additions & 10 deletions topiary-cli/tests/samples/expected/rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ pub fn node_kind_for_id(&self, id: u16) -> &'static str {

// More comments.

enum OneLine { Leaf { content: String, /* comment */ id: usize /* another comment */, size: usize, }, Hardline { content: String, id: usize, }, Space, } // End of line comment
enum OneLine { Leaf { content: String, id: usize, size: usize, }, Hardline { content: String, id: usize, }, Space, } /* comment */ /* another comment */ // End of line comment

enum ExpandEnum {
Leaf { content: String, /* Comment between fields. */ id: usize, size: usize, },
Leaf { content: String, id: usize, size: usize, }, /* Comment between fields. */
Hardline { content: String, id: usize, },
Space,
}
Expand Down Expand Up @@ -91,21 +91,25 @@ enum Mode6 {
fn inline_let() { let hi = 1; }

// While loop spacing
while i == true {
let i = 42;
fn my_while() {
while i == true {
let i = 42;
}
}

// Scoped blocks
{
let i = 42;
}
{
let i = 43;
fn foo() {
{
let i = 42;
}
{
let i = 43;
}
}

// Empty block inside of impl function
impl MyTrait for MyStruct {
fn foo() {
// ... logic ...
// ... logic ...
Copy link
Member

Choose a reason for hiding this comment

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

Is this not a case of an only child comment? How come this one doesn't bail out?

}
}
4 changes: 2 additions & 2 deletions topiary-cli/tests/samples/expected/tree_sitter_query.scm
Original file line number Diff line number Diff line change
Expand Up @@ -1023,8 +1023,8 @@
":" @append_indent_start
(_) @append_indent_end
.
; just doing _ above doesn't work, because it matches the final named node as
; well as the final non-named node, causing double indentation.
; just doing _ above doesn't work, because it matches the final named node as
; well as the final non-named node, causing double indentation.
Comment on lines +1026 to +1027
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the second sentence of this part of the heuristic happen?

If the comment node is alone on its line, anchor it to its next sibling in the CST. If it's the last of its siblings, anchor it to its previous sibling instead.

I haven't looked at the implementation yet, so perhaps that part is not done.

)

(value_specification
Expand Down
18 changes: 11 additions & 7 deletions topiary-cli/tests/samples/input/rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,21 @@ enum Mode6 {
fn inline_let() { let hi = 1; }

// While loop spacing
while i == true {
let i = 42;
fn my_while() {
while i == true {
let i = 42;
}
}


// Scoped blocks
{
let i = 42;
}
{
let i = 43;
fn foo() {
{
let i = 42;
}
{
let i = 43;
}
}

// Empty block inside of impl function
Expand Down
18 changes: 14 additions & 4 deletions topiary-config/src/language.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,10 @@ impl Language {
}

#[cfg(not(target_arch = "wasm32"))]
pub fn find_query_file(&self) -> TopiaryConfigResult<PathBuf> {
let basename = PathBuf::from(self.name.as_str()).with_extension("scm");
pub fn find_query_file(&self) -> TopiaryConfigResult<(PathBuf, Option<PathBuf>)> {
let name = self.name.clone();
let basename = PathBuf::from(&name).with_extension("scm"); // "clang.scm"
let comment_basename = PathBuf::from(&name).with_extension("comment.scm"); // "clang.comment.scm"

#[rustfmt::skip]
let potentials: [Option<PathBuf>; 4] = [
Expand All @@ -92,12 +94,20 @@ impl Language {
Some(PathBuf::from("../topiary-queries/queries")),
];

potentials
let query_file = potentials
.into_iter()
.flatten()
.map(|path| path.join(&basename))
.find(|path| path.exists())
.ok_or_else(|| TopiaryConfigError::QueryFileNotFound(basename))
.ok_or_else(|| TopiaryConfigError::QueryFileNotFound(basename))?;

let comment_query_file = query_file.parent().unwrap().join(comment_basename);

if comment_query_file.exists() {
Ok((query_file, Some(comment_query_file)))
} else {
Ok((query_file, None))
}
}

#[cfg(not(target_arch = "wasm32"))]
Expand Down
5 changes: 5 additions & 0 deletions topiary-core/benches/benchmark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use topiary_core::{formatter, Language, Operation, TopiaryQuery};
async fn format() {
let input = fs::read_to_string("../topiary-cli/tests/samples/input/ocaml.ml").unwrap();
let query_content = fs::read_to_string("../topiary-queries/queries/ocaml.scm").unwrap();
let comment_query_content =
fs::read_to_string("../topiary-queries/queries/ocaml.comment.scm").unwrap();
let ocaml = tree_sitter_ocaml::LANGUAGE_OCAML;

let mut input = input.as_bytes();
Expand All @@ -15,6 +17,9 @@ async fn format() {
let language: Language = Language {
name: "ocaml".to_owned(),
query: TopiaryQuery::new(&ocaml.clone().into(), &query_content).unwrap(),
comment_query: Some(
TopiaryQuery::new(&ocaml.clone().into(), &comment_query_content).unwrap(),
),
grammar: ocaml.into(),
indent: None,
};
Expand Down
Loading
Loading