Rust: Incorrect borrowing suggestion for range literals

[varkor]

use std::ops::RangeInclusive;

fn take_range(r: &RangeInclusive<i8>) {}

fn main() {
    take_range(0..=1);
}

error[E0308]: mismatched types
 --> src/main.rs:6:16
  |
6 |     take_range(0..=1);
  |                ^^^^^
  |                |
  |                expected reference, found struct `std::ops::RangeInclusive`
  |                help: consider borrowing here: `&0..=1`
  |
  = note: expected type `&std::ops::RangeInclusive<i8>`
             found type `std::ops::RangeInclusive<{integer}>`

&0..=1 is an incorrect suggestion because & has higher precedence than ..=. It should suggest &(0..=1).

[varkor]

If someone wants to tackle this issue, it should be straightforward to fix. In:
https://github.com/rust-lang/rust/blob/f0bae412e97db7ef26e24546bce5199c6a086152/src/librustc_typeck/check/demand.rs#L308-L312
there should be another branch for ExprKind::Call, where the path of the call is ops::RangeInclusive.
This code is a good reference:
https://github.com/rust-lang/rust/blob/f0bae412e97db7ef26e24546bce5199c6a086152/src/librustc/hir/lowering.rs#L3777-L3788

---

This issue affects the other range syntaxes too, so those should also be fixed (although these are not ExprKind::Call — look at hir/lowering.rs for clues).

[pawroman]

Hi @varkor, I'd like to tackle this and take the plunge into the Rust compiler code :)

[varkor]

@pawroman: great! Let me know if you hit any snags or need any more tips. You'll want to add tests for these (in src/test/ui). The rustc guide is a good place for general information about the compiler. When you're done, include r? @varkor in the pull request and I'll review it!

[pawroman]

@varkor you've anticipated most of my newbie questions already :) Thanks for that info, very useful! I'm now in the process of stressing my PC with lots of compiling to actually get started.

Skimming the test code I'm really impressed by how easy it is to add "UI" tests for these sorts of things (compile this, expect this error on this line). Snazzy!

That's all great, but do you anticipate I should also add "deeper" tests? I guess I'm trying to understand if the scope of this is just pure UI or do I need to touch some syntax parsing/immediate representations, since you mentioned code in lowering.rs.

Because the compiler is emitting a syntax suggestion, we might want to assert that the suggestion is correct syntax in the first place. I don't know if that's how deep the rabbit hole goes and if there's a facility to help out, but my hunch is that since we have all of the compiler at hand, why not use it to validate the suggestion syntax?

EDIT:
OK, reading up on adding new tests to the compiler I encountered mention of rust-runfix, and it seems to be sort-of what I was after.

[varkor]

That's all great, but do you anticipate I should also add "deeper" tests? I guess I'm trying to understand if the scope of this is just pure UI or do I need to touch some syntax parsing/immediate representations, since you mentioned code in lowering.rs.

The issue here is just a diagnostic issue, so checking that we're getting the updated hint is enough. The code in lowering.rs is just helpful to see what ranges like ..= and .. are desugared into, so you can catch them in demand.rs — you shouldn't need to modify anything (apart from the test) outside of demand.rs.

Because the compiler is emitting a syntax suggestion, we might want to assert that the suggestion is correct syntax in the first place. I don't know if that's how deep the rabbit hole goes and if there's a facility to help out, but my hunch is that since we have all of the compiler at hand, why not use it to validate the suggestion syntax?

It would technically be possible to do something like this, but it would probably be more trouble than it's worth. (I'm not aware of a facility to do this easily already.) I think manually going through the possible ExprKinds and checking that the parenthesisation behaviour is correct in each case would be a lot simpler: if we handle the different ExprKinds in an exhaustive match now (it's likely that if the ranges were handled incorrectly, there could be some others that are also missed at the moment), then this should be a one-time fix and the tests should prevent it regressing.

OK, reading up on adding new tests to the compiler I encountered mention of rust-runfix, and it seems to be sort-of what I was after.

A simple UI test should be sufficient here: --bless will generate the output files, which will include all the relevant notes.

[estebank]

There was a similar problem with the .into() suggestions, and it was fixed in #47702. Further use of the operator precedence list can be seen in #47247 (which introduced the error fixed in the other PR).

[pawroman]

Thanks for your guidance @varkor and @estebank, much appreciated. I have dug into HIR and built-in ranges a bit more.

Decided to check all the built-in range types and concluded they are all indeed affected by this problem.

The following test I came up with demonstrates my intended fix (fails with nightly as well as stage 1 compiled from master): https://gist.github.com/pawroman/6ed825d66c0f8d910b99ce3cf22cd4d9

None of the fix suggestions, when applied, constitute valid syntax. My test checks for syntax I believe should be suggested by the compiler.

I'm keen to tackle all of the range types at once in the scope of this issue. The problem I see though is just how wildly different each of the range types is represented in HIR. Adding a debug! call just above this line: https://github.com/rust-lang/rust/blob/992d1e4d3de364c895963167b70934599574d9a7/src/librustc_typeck/check/demand.rs#L311

Revealed the following:

• expr.node for 0..1 is Struct(Resolved(None, path(::std::ops::Range)), [Field { id: NodeId(116), ident: start#0, expr: expr(74: 0), span: rust/src/test/ui/range/issue-54505.rs:31:16: 31:17, is_shorthand: false }, Field { id: NodeId(117), ident: end#0, expr: expr(75: 1), span: rust/src/test/ui/range/issue-54505.rs:31:19: 31:20, is_shorthand: false }], None)

• expr.node for 1.. is Struct(Resolved(None, path(::std::ops::RangeFrom)), [Field { id: NodeId(118), ident: start#0, expr: expr(79: 1), span: rust/src/test/ui/range/issue-54505.rs:36:21: 36:22, is_shorthand: false }], None)

• expr.node for .. is Path(Resolved(None, path(::std::ops::RangeFull)))

• expr.node for 0..=1 is Call(expr(120: <::std::ops::RangeInclusive>::new), [expr(86: 0), expr(87: 1)])

• expr.node for ..5 is Struct(Resolved(None, path(::std::ops::RangeTo)), [Field { id: NodeId(121), ident: end#0, expr: expr(91: 5), span: rust/src/test/ui/range/issue-54505.rs:51:21: 51:22, is_shorthand: false }], None)

• expr.node for ..=42 is Struct(Resolved(None, path(::std::ops::RangeToInclusive)), [Field { id: NodeId(122), ident: end#0, expr: expr(95: 42), span: rust/src/test/ui/range/issue-54505.rs:56:32: 56:34, is_shorthand: false }], None)

I'm quite bewildered by this non-uniformity and I guess I need some help unifying all of these into a elegant condition to tell "is this a built-in range?". I could probably brute-force it in a match with explicit checks for path, but maybe we can do better.

Thanks in advance!

EDIT realized that this non-uniformity was actually mentioned in the second comment from @varkor , but my plea still stands. Thanks for your patience.

[estebank]

I think you can match on all of these with something along the lines of (pseudocode):

match expr.node{
    Struct(Resolved(None, path, ..), _) if path.startswith("std::ops::Range") => {
    }
    _ => {}
}

[pawroman]

Right, that's what I had in mind, but was also wondering if something like a check for RangeBounds trait would work better.

I guess we want to match against built-in range literals though, and explicitly. Here's how I rationalize it: because they are parsed and desugared by the compiler, the check needs to be explicit and hardcode all paths to builtin ranges. Handle a special case with a special case.

[varkor]

The problem I see though is just how wildly different each of the range types is represented in HIR.

Yes, this is where the lowering code comes in handy, because you can see exactly how the different syntax is desugared differently, to make sure you really are catching all the cases.

https://github.com/rust-lang/rust/blob/f0bae412e97db7ef26e24546bce5199c6a086152/src/librustc/hir/lowering.rs#L3789-L3834

Here's how I rationalize it: because they are parsed and desugared by the compiler, the check needs to be explicit and hardcode all paths to builtin ranges. Handle a special case with a special case.

Yes, I agree. It'd be nice if there was a more uniform way to handle them, but it's not convenient at the moment. You could include a reference back to lowering.rs in a comment to where the desugaring takes place, which will provide some motivation for matching in demand.rs.

[pawroman]

All clear now, thanks. I have succeeded in matching all range types and getting the test to pass. Needs some cleanups, but hopefully should be ready for PR soon. I have a few more questions before that though :smile:

I have noticed that most of my code would actually be irrelevant if I re-used functions and constants defined in clippy, e.g.:

https://github.com/rust-lang-nursery/rust-clippy/blob/a72e786c5d8866d554effd246c30fb553b63fa06/clippy_lints/src/utils/mod.rs#L154-L173

https://github.com/rust-lang-nursery/rust-clippy/blob/a72e786c5d8866d554effd246c30fb553b63fa06/clippy_lints/src/utils/higher.rs#L92

https://github.com/rust-lang-nursery/rust-clippy/blob/a72e786c5d8866d554effd246c30fb553b63fa06/clippy_lints/src/utils/paths.rs#L64-L75

Can't deny that I have used these as inspiration / guideline (albeit I did not do a stupid copy-paste). What's the policy on using clippy code (clippy is a submodule of main repo)? This is purely in the interest of pursuing DRY.

The diff is not that big (52 new lines added to demand.rs, fairly specific to matching Range paths) -- but still, my clean code sense tingles because of possible DRY violation.

Another conundrum I have is whether we should address core and std paths (and if so, how to work out we're not using std at compile time)?

[varkor]

I have noticed that most of my code would actually be irrelevant if I re-used functions and constants defined in clippy

Unfortunately, there's not much sharing from clippy to rustc. I wouldn't worry about it. If you can encapsulate it easily, it's possible that clippy could make use of the code in rustc instead.

Another conundrum I have is whether we should address core and std paths (and if so, how to work out we're not using std at compile time)?

The std_path function in lowering.rs picks the correct path for core/path — you should be able to use that.

[pawroman]

Thanks for the suggestion for std_path, unfortunately I can't really make the association of FnCtxt and LoweringContext.

It seems to me they are disjoint -- in driver.rs: lowering happens in phase2_configure_and_expand_inner, whereas typechecking happens in phase_3_run_analysis_passes. std_path in LoweringContext takes &mut self:

https://github.com/rust-lang/rust/blob/7e7bc060c1564b19742fd241f40a02c9210e445c/src/librustc/hir/lowering.rs#L4756-L4768

The Resolver it uses needs an awful lot of things to be instantiated. Not sure if I can get the hold of it all in FnCtxt.

Another problem I encountered while writing tests is that the changes I have made will affect suggestions for code like this:

use std::ops::RangeBounds;

fn take_range(_r: &impl RangeBounds<i8>) {}

fn main() {
    take_range(::std::ops::Range { start: 0, end: 1 });
}

With my changes, the suggestion would be &(::std::ops::RangeToInclusive { end: 5 }). I believe the correct suggestion would not involve the needless parentheses.

This is due to not differentiating between "de-sugared" form coming from hir and between these de-sugared forms being supplied explicitly as input source code.

With this in mind, I think current code is not PR worthy just yet. If of interest, here's the branch: https://github.com/pawroman/rust/tree/fix_range_borrowing_suggestion

[estebank]

Left a couple of comments on possible approaches on that commit. The code looks fine.