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

Ensure space around binary exprs #6085

Merged
merged 3 commits into from Mar 5, 2024

Conversation

MarcusGrass
Copy link
Contributor

Fixes #6059.

It just needed the same caveat for binary exprs.

It was already implemented for unaries so 2. .. 4. worked but not 4. / 3. .. 5. for example.

@@ -0,0 +1,3 @@
fn float_range() {
let _range = 3. / 2. ..4.;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your help on this!

What happens if the 2. was written as 2.0? Do we still add the space? Can you include a test case for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries! If the literal ends with a dot it doesn't add a space, I think that's intended right? I added a test case for it

src/expr.rs Outdated
Comment on lines 285 to 287
ast::ExprKind::Binary(_, ref expr, _) => {
needs_space_before_range(context, expr)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? The ast::ExprKind::Binary is defined as Binary(BinOp, P<Expr>, P<Expr>), and I'm assuming that the first expr is the left expr (the one that comes before the operator), while the second is the right expr (the one that comes after the operator). The way the code is written it looks like you're testing whether the left expr needs a space before the range, when I'd expect this to check if the right expr needs a space before the range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's incorrect, you're absolutely right, nice catch!

Changed to the rhs and added the source example and the other test cases.

@@ -0,0 +1,3 @@
fn float_range_no_trailing_dot_compacts() {
let _range = 3.0 / 2.0..4.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add the following test case:

let _range = 3. / 2.0..4.0;

@@ -0,0 +1,3 @@
fn float_range_trailing_dot() {
let _range = 3. / 2. ..4.;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the following test case:

let _range = 3.0 / 2. ..4.;

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

The code changes seem pretty straightforward. Just want to make sure that we're testing the correct expression when adding a check for ast::ExprKind::Binary in needs_space_before_range. I know needs_space_before_range takes the lhs of the range, but we need to make sure we're testing the rhs of the binary expression.

I've requested some additional test cases to help confirm that.

@ytmimi
Copy link
Contributor

ytmimi commented Feb 22, 2024

Also I think it would be helpful if we could work the original code snippet from #6059 into a test case.

@ytmimi ytmimi force-pushed the space-around-binary-exprs branch from abb10bd to d5674a3 Compare March 5, 2024 02:37
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

Just took another look at the PR, and now that we're passing the rhs_expr to needs_space_before_range it looks like rustfmt is producing the correct formatting. Thanks again for the help on this one!

@ytmimi ytmimi merged commit 73c8149 into rust-lang:master Mar 5, 2024
27 checks passed
@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Needs an associated changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Rust format might break ranges in specific cases
3 participants