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

[unstable option] spaces_around_ranges #3367

Open
Tracked by #1895
scampi opened this issue Feb 13, 2019 · 6 comments
Open
Tracked by #1895

[unstable option] spaces_around_ranges #3367

scampi opened this issue Feb 13, 2019 · 6 comments
Labels
unstable option tracking issue of an unstable option

Comments

@scampi
Copy link
Contributor

scampi commented Feb 13, 2019

Tracking issue for unstable option: spaces_around_ranges

@scampi scampi added the unstable option tracking issue of an unstable option label Feb 13, 2019
@scampi scampi changed the title [unstable option] unstable option: spaces_around_ranges [unstable option] spaces_around_ranges Feb 18, 2019
@msrd0
Copy link

msrd0 commented Nov 20, 2021

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.

@calebcartwright
Copy link
Member

@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

@msrd0
Copy link

msrd0 commented Nov 22, 2021

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.

@calebcartwright
Copy link
Member

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.

@dhardy
Copy link

dhardy commented Oct 8, 2022

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:

  • Given an expression on the left/write with tighter operator binding and which contains a space without parentheses, there should be spaces around the .. token: 0 .. n - 1
  • Given a path (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()
  • Given only a single token on either side, there should be no spaces (as the default option): 0..n

@LukeFranceschini
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unstable option tracking issue of an unstable option
Projects
None yet
Development

No branches or pull requests

5 participants