Conflicting opinions between `air` and `lintr` for multi-line assign expressions

[dgkf]

Unsure whether compatibility with lintr's formatting opinions is a design goal. If it isn't, feel free to ignore this!

When doing multiple assignments, as with the following code

a_really_long_symbol_name_that_requires_its_own_line <-
  a_really_long_symbol_name_that_requires_its_own_line_2 <-
  a_really_long_symbol_name_that_requires_its_own_line_3 <- "hello, world"

air will format it like this

a_really_long_symbol_name_that_requires_its_own_line <-
  a_really_long_symbol_name_that_requires_its_own_line_2 <-
    a_really_long_symbol_name_that_requires_its_own_line_3 <- "hello, world"

while lintr wants it to look like this (note the indentation)

a_really_long_symbol_name_that_requires_its_own_line <-
  a_really_long_symbol_name_that_requires_its_own_line_2 <-
  a_really_long_symbol_name_that_requires_its_own_line_3 <- "hello, world"

I used a simple example to illustrate the issue with just long symbol names, which is a pretty obscure use case. I've seen this in more practical examples when declaring multiple S7 methods from the same function definition.

There's some precedence for this operator indentation, given how the |> is often used in dplyr chains.

[DavisVaughan]

lintr wants it to look like this (note the indentation)

Regardless of what the expected outcome is here, our stance so far in Air is that if you are using a formatter, then any indentation or line-width related details should only be handled by the formatter. i.e. you should not have lintr weigh in on any formatting related decisions, because there are bound to be cases where they conflict.

[DavisVaughan]

Regarding the actual indentation, hmmmm, maybe?

This is a pretty niche case. But we do have some LeftAligned special casing for things like this

x <-
  this |>
  that |>
  this_other_thing

where you'll note that this |> is at the same indentation level as that |>, which wouldn't be the case if you removed the assignment.

This is kind of like a pipe chain after an initial <- if you squint and ignore the final <- "hello world" (but it's a bit of a stretch)

a_really_long_symbol_name_that_requires_its_own_line <-
  a_really_long_symbol_name_that_requires_its_own_line_2 <-
  a_really_long_symbol_name_that_requires_its_own_line_3 <- "hello, world"

[dgkf]

our stance so far in Air is that if you are using a formatter, then any indentation or line-width related details should only be handled by the formatter

Totally fair!

Another similar case that jumped to mind is for boolean operators, which air will align without additional indentation levels.

a ||
  b ||
  c ||
  d

In fact, most infix operators seem to behave this way! (although I found a couple other interesting exceptions in %% and %/%, whereas "special" (user-defined) infix operators do maintain the indent level)

a *
  b /
  c %%
    d %special%
    e

[DavisVaughan]

Binary expression chains like that have actually been painstakingly crafted to give the output you see there

air/crates/air_r_formatter/src/r/auxiliary/binary_expression.rs

Lines 288 to 492 in 2d1b389

	/// Format a binary expression chain
	///
	/// # Chain inversion
	///
	/// Binary expression chains (like pipe chains or ggplot2 `+` chains) work by turning:
	///
	/// ```r
	/// df |>
	/// foo() %>%
	/// bar()
	/// ```
	///
	/// Which generates a tree like:
	///
	/// ```text
	/// %>%
	/// / \
	/// / \
	/// |> bar()
	/// /\
	/// / \
	/// df foo()
	/// ```
	///
	/// Into a flat sequence of:
	///
	/// ```text
	/// df
	/// (|>, foo())
	/// (%>%, bar())
	///
	/// # Or, put differently:
	/// left
	/// (operator, right) # Tail piece 1
	/// (operator, right) # Tail piece 2
	/// ```
	///
	/// which you can then iterate through and print in order. This allows you to `group()`
	/// and `indent()` all of the `operator` and `right` nodes into a single block,
	/// so if any pipes break, then they all break.
	///
	/// It accomplishes this by looking down the LHS of the tree, accumulating
	/// `operator` and `right` as it goes, stopping at the first non-chainable
	/// element (here, the `df`), which becomes the overarching `left`.
	///
	/// # Deciding when to "break" the chain
	///
	/// We use the guiding principle that we want the reading order of
	/// a chain to match its execution order. That means that we never want
	/// to generate a chain with indentation like this:
	///
	/// ```r
	/// # Bad output
	/// a +
	/// b -
	/// c *
	/// d
	/// ```
	///
	/// The above indentation implies a top to bottom reading order of
	/// "a plus b, then minus c, then times d", but that isn't what happens. It's
	/// really more like "a plus b, then minus (c times d)".
	///
	/// A better indentation for this is:
	///
	/// ```r
	/// # Good output
	/// a +
	/// b -
	/// c *
	/// d
	///
	/// # Also allowed, since it fits on one line
	/// # (depends on persistent line breaks and object lengths)
	/// a +
	/// b -
	/// c * d
	/// ```
	///
	/// This better conveys that `c * d` is grouped together, either by being on
	/// the same line when possible, or by indenting to show that the binary chain
	/// has broken.
	///
	/// Implementing this is complex to think about but simple in practice. Because
	/// we only look down the left-hand side of the parse tree, this naturally
	/// enforces that any left` candidate binary operator that we see has an
	/// operator precedence that is `>=` its parent binary operator, making it
	/// a good candidate for chaining because it binds tighter to its operators
	/// than the parent does. When that candidate is written to the front of
	/// the chain, reading order still matches execution order. With this example:
	///
	/// ```r
	/// a + b - c * d
	/// ```
	///
	/// ```text
	/// -
	/// / \
	/// / \
	/// + *
	/// / \ / \
	/// a b c d
	/// ```
	///
	/// Note that `-` and `+` have the same precedence level, so `+` is a good
	/// candidate for chaining. `*` is placed on the RHS of the tree during parsing
	/// due to precedence rules, and is not considered for chaining, so it ends up
	/// in its own separate group, causing the extra indent from above.
	///
	/// This works nicely with `%>%` chains too. Since a chain of `%>%` are all at
	/// the same precedence level, you get a series of pipes on the `left` that you
	/// can collect and chain.
	///
	/// ```r
	/// a %>% b() %>% c()
	/// ```
	///
	/// ```text
	/// %>%
	/// / \
	/// / \
	/// %>% c()
	/// / \
	/// a b()
	/// ```
	///
	/// Mixing `|>` and `%>%` works fine because those are at the same precedence
	/// level.
	///
	/// # Piping into ggplot2
	///
	/// Chaining in this way extends naturally to `%>%` and `+` with ggplot2 calls.
	/// Consider this very common ggplot2 call:
	///
	/// ```r
	/// df %>%
	/// ggplot() +
	/// geom_bar()
	/// ```
	///
	/// ```text
	/// +
	/// / \
	/// / \
	/// %>% geom_line()
	/// / \
	/// df ggplot()
	/// ```
	///
	/// The precedences of `%>%` and `+` don't match here, but note that the `+` is
	/// the parent operator and the `%>%` is the child operator. The child `%>%` has
	/// a higher precedence than the parent `+` does, which means that we can
	/// continue chaining (i.e. reading order will match execution order if we keep
	/// chaining because `df %>% ggplot()` executes before the `+ geom_bar()`).
	///
	/// If you try and pipe OUT of a ggplot2 chain, then you get an indent at the
	/// end of the chain.
	///
	/// ```r
	/// df %>%
	/// ggplot() +
	/// geom_bar() %>%
	/// identity()
	/// ```
	///
	/// ```text
	/// +
	/// / \
	/// / \
	/// / \
	/// %>% %>%
	/// / \ / \
	/// / \ / \
	/// df ggplot() geom_bar() identity()
	/// ```
	///
	/// The final `%>%` ends up on the RHS of `+` in the parse tree,
	/// so it isn't considered chainable. The result follows our guiding principle
	/// that reading order should follow execution order. The
	/// `geom_bar() %>% identity()` is executed first, and the extra indent helps you
	/// see that.
	///
	/// As a final example, consider this line break, which we actually want to ignore:
	///
	/// ```r
	/// # Input
	/// df |> ggplot() +
	/// geom_bar() + geom_line()
	///
	/// # Expected output (ignore line break, flatten)
	/// df |> ggplot() + geom_bar() + geom_line()
	///
	/// # Not
	/// df |> ggplot() +
	/// geom_bar() +
	/// geom_line()
	/// ```
	///
	/// The entire binary chain is chainable here, because every operator ends up
	/// on the LHS in the parse tree, so it forms 1 group. We perform the user
	/// requested line break check on the `|>`, see that there isn't a line break,
	/// and flatten the whole chain. If `|>` and the `+`s were split into different
	/// groups, then we'd have 2 places where the user could request a line break
	/// and we'd end up with the bad result.
	fn fmt_binary_chain(

The intent is to bump the indentation level when the implied order of operations prevents you from just reading it left to right

If you take this flat chain

xx <- 
  a * b / c %% d %special% e

and put a break after the * and reformat then it will also break on the /

xx <-
  a *
  b /
  c %% d %special% e

It's trying to convey to you that the order of operations here is really a * b / (c %% d %special% e)

i.e. reading it like this would be misleading

xx <-
  a *
  b / c %% d %special% e

because you'd think it is a * (b / c %% d %special% e) but that isn't the case

As you add more persistent line breaks in there, things get a bit harder to talk about, but that's the basic idea

[dgkf]

The intent is to bump the indentation level when the implied order of operations prevents you from just reading it left to right

Brilliant. Love the attention to detail -- I didn't even consider operator precedence.

[MichaelChirico]

Consideration of operator precedence is also something under consideration for operator spacing here: tidyverse/style#160