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

Introduce measuring scopes #755

Merged
merged 5 commits into from
Oct 1, 2024
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,11 @@ This name should be decided amongst the team before the release.
### Added
- [#705](https://github.com/tweag/topiary/pull/705) Added support for Nickel 1.7 extended pattern formatting
- [#737](https://github.com/tweag/topiary/pull/737) Added the `prefetch` command, that prefetches and caches all grammars in the current configuration
- [#755](https://github.com/tweag/topiary/pull/755) Introduce measuring scopes, which can be used in conjunction with regular scopes to add expressivity to the formatting queries.

### Fixed
- [#720](https://github.com/tweag/topiary/pull/720) [#722](https://github.com/tweag/topiary/pull/722) [#723](https://github.com/tweag/topiary/pull/723) [#724](https://github.com/tweag/topiary/pull/724) [#735](https://github.com/tweag/topiary/pull/735)
[#738](https://github.com/tweag/topiary/pull/738) [#739](https://github.com/tweag/topiary/pull/739) [#745](https://github.com/tweag/topiary/pull/745) Various OCaml improvements
[#738](https://github.com/tweag/topiary/pull/738) [#739](https://github.com/tweag/topiary/pull/739) [#745](https://github.com/tweag/topiary/pull/745) [#755](https://github.com/tweag/topiary/pull/755) Various OCaml improvements
- [#744](https://github.com/tweag/topiary/pull/744) Nickel: fix the indentation of `in` for annotated multiline let-bindings

### Changed
Expand Down
82 changes: 82 additions & 0 deletions README.md
nbacquey marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -1225,6 +1225,88 @@ containing the matched nodes are is single-line (resp. multi-line).
)
```

### `@prepend_begin_measuring_scope` / `@append_begin_measuring_scope` / `@prepend_end_measuring_scope` / `@append_end_measuring_scope`

Sometimes, custom scopes are not enough: we may want to format a node depending on the multi-line-ness of a piece of code that does not include the node in question. For instance, consider this function application in OCaml:
```ocaml
foo bar (fun x -> qux)
```
We may also want to format it as any of the following two, depending on the actual length of `foo`, `bar`, and `qux`:
```ocaml
foo bar (fun x ->
qux
)
```
```ocaml
foo
bar
(fun x ->
qux
)
```
Consider the indentation of `(fun x -> qux)`: if `foo bar` is single-line, we don't want to indent it. But if `foo bar` is multi-line, we do want to indent it.

Because custom scopes can only impact the behaviour of nodes inside the scope, we can't use them to solve this issue. This is why we need `measuring_scope`.

Measuring scopes are opened/closed with a similar syntax as "regular" custom scopes, with any of the following tags, in conjunction with the `#scope_id!` predicate:

* `@prepend_begin_measuring_scope`
* `@append_begin_measuring_scope`
* `@prepend_end_measuring_scope`
* `@prepend_begin_measuring_scope`

Measuring scopes behave as follows:
* A measuring scope must always be contained in a regular custom scope with the same `#scope_id!`. There can't be two measuring scopes with the same `#scope_id!` inside the same regular custom scope.
* If a regular custom scope contains a measuring scope, then all tags contained in the regular scope that depend on its multi-line-ness will instead depend on the multi-line-ness of the measuring scope (hence the name: the inner, measuring scope measures the multi-line-ness of the outer, regular scope).
nbacquey marked this conversation as resolved.
Show resolved Hide resolved

#### Example

The example below solves the problem of indenting function application in OCaml stated above, using measuring scopes.
```scheme
(application_expression
.
(_) @append_indent_start @prepend_begin_scope @prepend_begin_measuring_scope
(#scope_id! "function_application")
(_) @append_end_scope
.
)
; The end of the measuring scope depends on the last argument: if it's a function,
; end it before the function, otherwise end it after the last argument. In that case,
; it's the same as the regular scope.
(application_expression
(#scope_id! "function_application")
(_
[
(fun_expression)
(function_expression)
]? @do_nothing
) @append_end_measuring_scope
.
)
(application_expression
(#scope_id! "function_application")
(_
[
(fun_expression)
(function_expression)
] @prepend_end_measuring_scope
)
.
)
; If the measuring scope is single-line, end indentation _before_ the last node.
; Otherwise, end the indentation after the last node.
(application_expression
(#multi_line_scope_only! "function_application")
(_) @append_indent_end
.
)
(application_expression
(#single_line_scope_only! "function_application")
(_) @prepend_indent_end
.
)
```

## Suggested workflow

In order to work productively on query files, the following is one
Expand Down
10 changes: 5 additions & 5 deletions topiary-cli/tests/samples/expected/ocaml.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1187,8 +1187,8 @@ let () =

let () =
foo x (fun y ->
zzzzzzzzzz
)
zzzzzzzzzz
)

let foo x = function
| y -> zzzzzzzzzz
Expand All @@ -1206,9 +1206,9 @@ let () =

let () =
foo x (function
| y -> zzzzzzzzzz
| u -> vvvvvvvvv
)
| y -> zzzzzzzzzz
| u -> vvvvvvvvv
)

(* #727 proper formatting of multi-lined typed function argument *)
let foo
Expand Down
149 changes: 135 additions & 14 deletions topiary-core/src/atom_collection.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{
borrow::Cow,
cmp::Ordering,
collections::{HashMap, HashSet},
mem,
ops::Deref,
Expand Down Expand Up @@ -307,6 +308,34 @@ impl AtomCollection {
predicates,
);
}
"prepend_begin_measuring_scope" => {
self.prepend(
Atom::MeasuringScopeBegin(scope_information_prepend()?),
node,
predicates,
);
}
"append_begin_measuring_scope" => {
self.append(
Atom::MeasuringScopeBegin(scope_information_append()?),
node,
predicates,
);
}
"prepend_end_measuring_scope" => {
self.prepend(
Atom::MeasuringScopeEnd(scope_information_prepend()?),
node,
predicates,
);
}
"append_end_measuring_scope" => {
self.append(
Atom::MeasuringScopeEnd(scope_information_append()?),
node,
predicates,
);
}
// Scoped softlines
"append_empty_scoped_softline" => {
let id = self.next_id();
Expand Down Expand Up @@ -404,10 +433,29 @@ impl AtomCollection {
pub fn apply_prepends_and_appends(&mut self) {
let mut expanded: Vec<Atom> = Vec::new();

// We sort the prepends/appends so that:
// * BeginScope(s) will always be the first element(s)
// * EndScope(s) will always be the last element(s)
// This permits proper processing of measuring scopes and scoped atoms
// that are added at the same place as Begin/EndScopes.
fn compare_atoms(a1: &Atom, a2: &Atom) -> Ordering {
match (a1, a2) {
(Atom::ScopeBegin(_), Atom::ScopeBegin(_)) => Ordering::Equal,
(Atom::ScopeBegin(_), _) => Ordering::Less,
(_, Atom::ScopeBegin(_)) => Ordering::Greater,
(Atom::ScopeEnd(_), Atom::ScopeEnd(_)) => Ordering::Equal,
(Atom::ScopeEnd(_), _) => Ordering::Greater,
(_, Atom::ScopeEnd(_)) => Ordering::Less,
(_, _) => Ordering::Equal,
}
}

for atom in &mut self.atoms {
if let Atom::Leaf { id, .. } = atom {
let prepends = self.prepend.entry(*id).or_default();
prepends.sort_by(compare_atoms);
let appends = self.append.entry(*id).or_default();
appends.sort_by(compare_atoms);

// Rather than cloning the atom from the old vector, we
// simply take it. This will leave a default (empty) atom
Expand Down Expand Up @@ -605,10 +653,16 @@ impl AtomCollection {
type ScopeId = String;
type LineIndex = u32;
type ScopedNodeId = usize;
// `opened_scopes` maintains stacks of opened scopes,
// the line at which they started,
// and the list of `ScopedSoftline` they contain.
let mut opened_scopes: HashMap<&ScopeId, Vec<(LineIndex, Vec<&Atom>)>> = HashMap::new();
type OpenedScopeInfo<'a> = (LineIndex, Vec<&'a Atom>, Option<bool>);
// `opened_scopes` maintains stacks of opened scopes.
// For each scope, we record:
// * the line at which they started (LineIndex),
// * the list of `ScopedSoftline` and `ScopedConditional` they contain (Vec<&Atom>),
// * if they contain a measuring scope, whether it is multi-line (Option<bool>).
let mut opened_scopes: HashMap<&ScopeId, Vec<OpenedScopeInfo>> = HashMap::new();
// `opened_measuring_scopes` maintains stacks of opened measuring scopes,
// and the line at which they started.
let mut opened_measuring_scopes: HashMap<&ScopeId, Vec<LineIndex>> = HashMap::new();
// We can't process `ScopedSoftline` in-place as we encounter them in the list of
// atoms: we need to know when their encompassing scope ends to decide what to
// replace them with. Instead of in-place modifications, we associate a replacement
Expand All @@ -631,16 +685,20 @@ impl AtomCollection {
opened_scopes
.entry(scope_id)
.or_default()
.push((*line_start, Vec::new()));
.push((*line_start, Vec::new(), None));
} else if let Atom::ScopeEnd(ScopeInformation {
line_number: line_end,
scope_id,
}) = atom
{
if let Some((line_start, atoms)) =
if let Some((line_start, atoms, measuring_scope)) =
opened_scopes.get_mut(scope_id).and_then(Vec::pop)
{
let multiline = line_start != *line_end;
let multiline = if let Some(mult) = measuring_scope {
mult
} else {
line_start != *line_end
};
for atom in atoms {
if let Atom::ScopedSoftline { id, spaced, .. } = atom {
let new_atom = if multiline {
Expand Down Expand Up @@ -671,9 +729,58 @@ impl AtomCollection {
log::warn!("Closing unopened scope {scope_id:?}");
force_apply_modifications = true;
}
// Open measuring scope
} else if let Atom::MeasuringScopeBegin(ScopeInformation {
line_number: line_start,
scope_id,
}) = atom
{
if opened_scopes.entry(scope_id).or_default().is_empty() {
log::warn!(
"Opening measuring scope with no associated regular scope {scope_id:?}"
);
force_apply_modifications = true;
} else {
opened_measuring_scopes
.entry(scope_id)
.or_default()
.push(*line_start)
}
// Close measuring scope and register multi-line-ness in the appropriate regular scope
} else if let Atom::MeasuringScopeEnd(ScopeInformation {
line_number: line_end,
scope_id,
}) = atom
{
if let Some(line_start) =
opened_measuring_scopes.get_mut(scope_id).and_then(Vec::pop)
{
let multi_line = line_start != *line_end;
if let Some((regular_line_start, vec, measuring_scope)) =
opened_scopes.get_mut(scope_id).and_then(Vec::pop)
{
if measuring_scope.is_none() {
opened_scopes.entry(scope_id).or_default().push((
regular_line_start,
vec,
Some(multi_line),
));
} else {
log::warn!("Found several measuring scopes in a single regular scope {scope_id:?}");
force_apply_modifications = true;
}
} else {
log::warn!("Found measuring scope outside of regular scope {scope_id:?}");
force_apply_modifications = true;
}
} else {
log::warn!("Closing unopened measuring scope {scope_id:?}");
force_apply_modifications = true;
}
// Register the ScopedSoftline in the correct scope
} else if let Atom::ScopedSoftline { scope_id, .. } = atom {
if let Some((_, vec)) = opened_scopes.get_mut(&scope_id).and_then(|v| v.last_mut())
if let Some((_, vec, _)) =
opened_scopes.get_mut(&scope_id).and_then(|v| v.last_mut())
{
vec.push(atom);
} else {
Expand All @@ -682,7 +789,8 @@ impl AtomCollection {
}
// Register the ScopedConditional in the correct scope
} else if let Atom::ScopedConditional { scope_id, .. } = atom {
if let Some((_, vec)) = opened_scopes.get_mut(&scope_id).and_then(|v| v.last_mut())
if let Some((_, vec, _)) =
opened_scopes.get_mut(&scope_id).and_then(|v| v.last_mut())
{
vec.push(atom);
} else {
Expand All @@ -691,21 +799,33 @@ impl AtomCollection {
}
}
}
let still_opened: Vec<&String> = opened_scopes
let mut still_opened: Vec<&String> = opened_scopes
.into_iter()
.filter_map(|(scope_id, vec)| if vec.is_empty() { None } else { Some(scope_id) })
.collect();
if !still_opened.is_empty() {
log::warn!("Some scopes have been left opened: {:?}", still_opened);
force_apply_modifications = true;
}
still_opened = opened_measuring_scopes
.into_iter()
.filter_map(|(scope_id, vec)| if vec.is_empty() { None } else { Some(scope_id) })
.collect();
if !still_opened.is_empty() {
log::warn!(
"Some measuring scopes have been left opened: {:?}",
still_opened
);
force_apply_modifications = true;
}

// Remove scopes from the atom list
for atom in &mut self.atoms {
match atom {
Atom::ScopeBegin(_) | Atom::ScopeEnd(_) => {
*atom = Atom::Empty;
}
Atom::ScopeBegin(_)
| Atom::ScopeEnd(_)
| Atom::MeasuringScopeBegin(_)
| Atom::MeasuringScopeEnd(_) => *atom = Atom::Empty,
_ => {}
}
}
Expand Down Expand Up @@ -954,7 +1074,8 @@ fn collapse_spaces_before_antispace(v: &mut [Atom]) {
antispace_mode = true;
} else if *a == Atom::Space && antispace_mode {
*a = Atom::Empty;
} else {
} else if *a != Atom::Empty {
// Don't change mode when encountering Empty
antispace_mode = false;
}
}
Expand Down
22 changes: 16 additions & 6 deletions topiary-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,20 +92,30 @@ pub enum Atom {
/// Indicates the end of a scope, use in combination with the
/// ScopedSoftlines and ScopedConditionals below.
ScopeEnd(ScopeInformation),
// Indicates the beginning of a *measuring* scope, that must be related to a "normal" one.
// Used in combination with ScopedSoftlines and ScopedConditionals below.
MeasuringScopeBegin(ScopeInformation),
// Indicates the end of a *measuring* scope, that must be related to a "normal" one.
// Used in combination with ScopedSoftlines and ScopedConditionals below.
MeasuringScopeEnd(ScopeInformation),
/// Scoped commands
// ScopedSoftline works together with the @{prepend,append}_begin_scope and
// @{prepend,append}_end_scope query tags. To decide if a scoped softline
// ScopedSoftline works together with the @{prepend,append}_begin[_measuring]_scope and
// @{prepend,append}_end[_measuring]_scope query tags. To decide if a scoped softline
// must be expanded into a hardline, we look at the innermost scope having
// the corresponding `scope_id`, that encompasses it. We expand the softline
// if that scope is multi-line. The `id` value is here for technical
// reasons, it allows tracking of the atom during post-processing.
// if that scope is multi-line.
// If that scope contains a *measuring* scope with the same `scope_id`, we expand
// the node iff that *measuring* scope is multi-line.
// The `id` value is here for technical reasons,
// it allows tracking of the atom during post-processing.
ScopedSoftline {
id: usize,
scope_id: String,
spaced: bool,
},
/// Represents an atom that must only be output if the associated scope meets the condition
/// (single-line or multi-line)
/// Represents an atom that must only be output if the associated scope
/// (or its associated measuring scope, see above) meets the condition
/// (single-line or multi-line).
ScopedConditional {
id: usize,
scope_id: String,
Expand Down
Loading
Loading