Open scampi opened 5 years ago
I'm not sure if this is the correct place for feedback, but I wish there was more distinction that just a boolean. 0..n
looks neat and I prefer it over 0 .. n
. However, for more complex ranges like a + 1..b + 2
, my brain always wants to read it like a + (1..b) + 2
due to the way spaces are placed. So I'd prefer a third option that adds spaces for every range whenever the left or right hand side of the range also contains a space.
@msrd0 - this is indeed a great place to ask questions/make suggestions about the type of a config option, associated variants, etc. so thanks for sharing.
Do you think it's more about there being a space, or is it perhaps specifically when the lhs and/or rhs of the range expression are themselves binary expressions? I'd be open to evolving the current config option to have a variant for adding spaces in the presence of some form of "complexity", but I feel like a whitespace-driven determination would be a bit too blunt of an approach
I wouldn't necessarily limit this to binary expressions, I'd prefer something like 0 .. some_list.iter().map(|foo| foo.some_number).max()
over a version without spaces. A simpler indicator for the "complexity" than if rhs/lhs contain spaces could be to consider it complex unless it consists of only a literal or variable.
We already do formatting consideration of spaces based on the kinds of expressions (since a float expression on the lhs or a nested range on the rhs could be botched without spaces), so it would certainly be feasible to add a variant that would always add a space unless both the lhs and rhs kinds were either a lit or path. However, I do want to be mindful about the potential for this to snowball to account for all sorts of expression kind combinations because there's quite a lot of them.
mindful about ... to snowball to account for all sorts of expression kind combinations because there's quite a lot of them
There are, yes. My personal feeling is that "spaces should align with operator precedence", hence:
..
token: 0 .. n - 1
foo::bar
) or function call (foo.bar()
), it is ambiguous — this should be another option? Note that the example above involving a closure falls into this category (the space is within parentheses): 0..v.len()
or 0 .. v.len()
0..n
I also feel that whitespace-driven rules are a bit blunt. My mental model is that spaces should be inserted if there is some extra computation that needs to happen before the range can be evaluated. Computation only needs to happen on one side of the range for spaces to be inserted on both sides. So if we are only looking at values that need to be retrieved (whether they are struct fields, values from modules (ex. some_mod::some_value
), a literal, or a dereference) then spaces should not be inserted. But if a function is being called, an operator is being used (other than dereference), or a type is being cast then spaces should be inserted.
There are a couple of awkward situations where rustfmt wouldn't be able to do the correct thing because it doesn't have access to the right information in the AST (I think?). For example, a function like v.len()
might internally just be reading a value from a struct and so I wouldn't put spaces in 0..v.len()
. Or a type might be being cast in a way that incurs no runtime overhead so there isn't any actual computation happening. But in situations where rustfmt can't be 100% sure there's no computation happening I would prefer the behaviour where it plays it safe and inserts the spaces anyway.
Tracking issue for unstable option: spaces_around_ranges