Fix range literals borrowing suggestions #54734

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.

Sign up for GitHub

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

Jump to bottom

Merged

bors merged 6 commits into rust-lang:master from pawroman:fix_range_borrowing_suggestion

Oct 10, 2018

Contributor

pawroman commented Oct 1, 2018

Fixes #54505. The compiler issued incorrect range borrowing suggestions (missing () around borrows of range literals). This was not correct syntax (see the issue for an example).

With changes in this PR, this is fixed for all types of Range literals.

Thanks again to @varkor and @estebank for their invaluable help and guidance.

pawroman added 2 commits

September 29, 2018 15:18


          First stab at fixing rust-lang#54505

0626afb


          Fix range borrowing suggestions logic

f7d35cc

rust-highfive assigned varkor

Contributor

rust-highfive commented Oct 1, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rust-highfive added the S-waiting-on-review label

pawroman changed the title ~~Fix range borrowing suggestion~~ Fix range literals borrowing suggestions

varkor suggested changes

View reviewed changes

Member

varkor left a comment

This is looking really good, thanks! I've left a few comments, but they're mostly minor things. Nice work!

src/librustc_typeck/check/demand.rs Outdated

		};
		let sugg_expr = if needs_parens { format!("({})", src) } else { src };

Member

varkor Oct 2, 2018

Generally prefer to split if-else across separate lines.

src/librustc_typeck/check/demand.rs Outdated

+                          } else {
+                              false
+                          }

Member

varkor Oct 2, 2018

Nit: delete blank line here.

src/librustc_typeck/check/demand.rs Outdated Show resolved Hide resolved

src/librustc_typeck/check/demand.rs Outdated Show resolved Hide resolved

src/librustc_typeck/check/demand.rs Show resolved Hide resolved

src/librustc_typeck/check/demand.rs Show resolved Hide resolved

src/test/ui/range/issue-54505-no-literals.fixed Outdated Show resolved Hide resolved

src/librustc_typeck/check/demand.rs Outdated

@@ @@ -374,6 +379,77 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { @@
                       None
                   }
+                  // This function checks if the specified expression is a built-in range literal

Member

varkor Oct 2, 2018

Use /// for documentation comments for functions. (Nit: And full stops for the ends of sentences 😄)

Contributor Author

pawroman Oct 2, 2018

Right. Does nightly rustdoc support code references yet?

Member

varkor Oct 3, 2018

As in references to source files? Not that I'm aware of.

src/librustc_typeck/check/demand.rs Outdated

@@ @@ -374,6 +379,77 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { @@
                       None
                   }
+                  // This function checks if the specified expression is a built-in range literal
+                  // (See: librustc/hir/lowering.rs::LoweringContext::lower_expr() )

Member

varkor Oct 2, 2018

Nit: I'd surround the path with backticks, like any piece of code.

src/librustc_typeck/check/demand.rs

+                      // (via split on "|")
+                      let ops_path = ["{{root}}", "std|core", "ops"];
+                      let is_range_path = |path: &Path| {

Member

varkor Oct 2, 2018

I might be alone in this (cc @estebank), but I think for a short path like this, a more explicit approach (like below) is more readable. (I specifically don't like splitting on "|" because it feels a little too ad hoc.)

let is_range_path = |path: &Path| {
    let segs = path.segments.iter().map(|seg| seg.ident.as_str());
	if let (Some("{{root}}"), std_core, "ops", range, None) = (segs.next(), segs.next(), segs.next(), segs.next(), segs.next()) {
		(std_core == "std" || std_core == "ops") && range.starts_with("Range")
	} else {
		false
	}
};

Contributor Author

pawroman Oct 2, 2018

That's a great suggestion! I was looking for something concise like this syntax-wise, it didn't occur to me that I could use .next() and rely on it returning None when there's no more elements.


          Address review comments

87bf9e2

pawroman commented

View reviewed changes

src/librustc_typeck/check/demand.rs

+                              .map(|seg| seg.ident.as_str());
+                          if let (Some(root), Some(std_core), Some(ops), Some(range), None) =
+                              (segs.next(), segs.next(), segs.next(), segs.next(), segs.next())

Contributor Author

pawroman Oct 2, 2018

Ident.as_str() returns a LocalInternedString which can be compared with &str, but I found it too verbose to use in Some().

varkor reviewed

View reviewed changes

src/librustc_typeck/check/demand.rs Outdated

@@ @@ -374,6 +383,66 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { @@
                       None
                   }
+                  /// This function checks if the specified expression is a built-in range literal.
+                  /// (See: ``librustc::hir::lowering::LoweringContext::lower_expr()``).

Member

varkor Oct 3, 2018

Sorry, I meant single rather than double backticks; I should have clarified. On second thoughts, though, it might be more convenient to include the path:

/// (See `LoweringContext::lower_expr()` in `src/librustc/hir/lowering.rs`.)

as then someone can jump straight to the path. (Like in the following.)
https://github.com/rust-lang/rust/blob/f89622a7548f69c76e5c9857e34be568ba322d02/src/librustc_privacy/lib.rs#L1388

varkor reviewed

View reviewed changes

src/librustc_typeck/check/demand.rs Outdated

+                          }
+                      };
+                      let is_range_literal = |span: &Span| {

Member

varkor Oct 3, 2018

As you pointed out, it would be clearer to give this a different name than the function it's in. Maybe span_is_range_literal or something like that?

varkor reviewed

View reviewed changes

src/librustc_typeck/check/demand.rs Outdated

+                      let is_range_literal = |span: &Span| {
+                          // Tell if expression span snippet doesn't look like an explicit
+                          // Range struct or `new()` call.  This is to allow inferring

Member

varkor Oct 3, 2018

"Inferring" has a very specific meaning, so I'd tend to avoid it in favour of something like:

// Check whether a span corresponding to a range is a range literal,
// rather than an explicit struct or `new()` call.

Member

varkor commented Oct 3, 2018

Some very minor comments to follow up, just to make things a little more readable. After this, it all looks good to me!


          Address review comments

611e5c4

Contributor Author

pawroman commented Oct 3, 2018

Addressed them now -- thanks for review and please let me know if anything else.

Member

varkor commented Oct 3, 2018

Great, thanks for fixing this @pawroman!

Collaborator

bors commented Oct 3, 2018

📌 Commit 611e5c4 has been approved by varkor

bors added S-waiting-on-bors and removed S-waiting-on-review labels

pietroalbini added a commit to pietroalbini/rust that referenced this pull request


          Rollup merge of rust-lang#54734 - pawroman:fix_range_borrowing_sugges…

36e7d4d

…tion, r=varkor

Fix range literals borrowing suggestions

Fixes rust-lang#54505. The compiler issued incorrect range borrowing suggestions (missing `()` around borrows of range literals). This was not correct syntax (see the issue for an example).

With changes in this PR, this is fixed for all types of `Range` literals.

Thanks again to @varkor and @estebank for their invaluable help and guidance.

r? @varkor

pietroalbini mentioned this pull request

Rollup of 10 pull requests #54796

Closed

Member

pietroalbini commented Oct 4, 2018

This is failing on wasm.

[00:51:26] ---- [ui] ui/range/issue-54505-no-std.rs stdout ----
[00:51:26] diff of stderr:
[00:51:26] 1	error: `#[panic_handler]` function required, but not found
[00:51:26] 2	
[00:51:26] -	error: language item required, but not found: `eh_personality`
[00:51:26] -	
[00:51:26] 5	error[E0308]: mismatched types
[00:51:26] 6	  --> $DIR/issue-54505-no-std.rs:21:16
[00:51:26] 7	   |
[00:51:26] 
[00:51:26] 74	   = note: expected type `&_`
[00:51:26] 75	              found type `core::ops::RangeToInclusive<{integer}>`
[00:51:26] 76	
[00:51:26] -	error: aborting due to 8 previous errors
[00:51:26] +	error: aborting due to 7 previous errors
[00:51:26] 78	
[00:51:26] 79	For more information about this error, try `rustc --explain E0308`.

bors added S-waiting-on-author and removed S-waiting-on-bors labels

Contributor Author

pawroman commented Oct 4, 2018

Uh oh! Will look into this as soon as I have some spare time.


          Attempt to fix rust-lang#54505 tests for wasm

882e373

Contributor Author

pawroman commented Oct 8, 2018

I have attempted a fix -- unfortunately having trouble getting the toolchain to work with wasm target, so can't test that at the moment. Any help with this would be appreciated. Alternatively, we can lean on the automated CI process to test with wasm target.

Member

varkor commented Oct 8, 2018

Collaborator

bors commented Oct 8, 2018

📌 Commit 882e373 has been approved by varkor

bors added S-waiting-on-bors and removed S-waiting-on-author labels

Collaborator

bors commented Oct 9, 2018

⌛ Testing commit 882e373 with merge 5a7b2dec4ec3ea0c84b272799f81d090dc773263...

Collaborator

bors commented Oct 9, 2018

💔 Test failed - status-appveyor

bors added S-waiting-on-review and removed S-waiting-on-bors labels


          Fix test for windows os

1f7dafb

Contributor Author

pawroman commented Oct 9, 2018

Fixed that test for windows, tested successfully with x86_64-pc-windows-gnu target.

Member

varkor commented Oct 9, 2018

Collaborator

bors commented Oct 9, 2018

📌 Commit 1f7dafb has been approved by varkor

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Collaborator

bors commented Oct 9, 2018

⌛ Testing commit 1f7dafb with merge 1a46ecf...

bors added a commit that referenced this pull request


          Auto merge of #54734 - pawroman:fix_range_borrowing_suggestion, r=varkor

1a46ecf

Fix range literals borrowing suggestions

Fixes #54505. The compiler issued incorrect range borrowing suggestions (missing `()` around borrows of range literals). This was not correct syntax (see the issue for an example).

With changes in this PR, this is fixed for all types of `Range` literals.

Thanks again to @varkor and @estebank for their invaluable help and guidance.

r? @varkor

Collaborator

bors commented Oct 9, 2018

💔 Test failed - status-appveyor

bors added S-waiting-on-review and removed S-waiting-on-bors labels

Contributor Author

pawroman commented Oct 9, 2018 •

edited

Loading

Hmm, the failure appears unrelated at first glance. I'm seeing Permission denied in stderr. https://ci.appveyor.com/project/rust-lang/rust/builds/19375959/job/qb9sd3v1ya4lmm9i#L10798 Any clues?

Member

varkor commented Oct 9, 2018

The error is confusing. It's referencing issue-19100\a.exe, but it doesn't appear to be a test that should be generated ancillary outputs. My inclination would be to rebase on top of master and see if that fixes it (you might as well squash the related "fix test" commits at the same time).

Member

pietroalbini commented Oct 9, 2018

Let's just retry.

@bors retry

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Collaborator

bors commented Oct 9, 2018

⌛ Testing commit 1f7dafb with merge eae47a4...

bors added a commit that referenced this pull request


          Auto merge of #54734 - pawroman:fix_range_borrowing_suggestion, r=varkor

eae47a4

Fix range literals borrowing suggestions

Fixes #54505. The compiler issued incorrect range borrowing suggestions (missing `()` around borrows of range literals). This was not correct syntax (see the issue for an example).

With changes in this PR, this is fixed for all types of `Range` literals.

Thanks again to @varkor and @estebank for their invaluable help and guidance.

r? @varkor

Collaborator

bors commented Oct 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: varkor
Pushing eae47a4 to master...

bors merged commit 1f7dafb into rust-lang:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors