Fix built-in indexing not being used where index type wasn't "obviously" usize by ivanbakel · Pull Request #47167 · rust-lang/rust

ivanbakel · 2018-01-03T23:32:16Z

This PR was made possible thanks to the generous help of @eddyb

Following the example of binary operators, builtin checking for indexing has been moved from the typecheck stage to a writeback stage, after type constraints have been resolved.

rust-highfive · 2018-01-03T23:32:21Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2018-01-03T23:35:31Z

This comment is far too specific IMO, without this logic Index<usize> for [T]impl would infinitely recurse, for example.

eddyb · 2018-01-03T23:35:54Z

We always write this as ty::TyRef

eddyb · 2018-01-03T23:36:46Z

index_ty should be compared to tcx.types.usize.

The is_uint() does that, from my understanding - would a direct comparison work in the case of the index being just literals?

Since you resolved the type, it can't be TyInfer(IntVar(_)) anymore, only an usize. is_uint is pretty useless IMO, and misleading nowadays, when uint typically means "any unsigned integer" not usize.

eddyb · 2018-01-03T23:37:56Z

r? @nikomatsakis

nikomatsakis · 2018-01-04T17:56:24Z

This makes me a bit ... nervous. It seems strange to strip out the &T based purely on the type involved. That is, if we were looking at the adjustment and deciding to strip off a layer of references, that'd be one thing, but I deciding based on the type alone makes me nervous. Can you give some examples of why this line of code was needed?

I'd guess TyError, but we should probably skip doing this at all if there are any errors.

The Unsize adjustment made by the index typechecking code (2252-2257 in this codebase) doesn't have the unsized type as the target, instead taking method.sig.inputs()[0], a &self. This is, to my understanding, the only way for the result of expr_ty_adjusted to result in a TyRef, since otherwise a reference would be deref'd, and you can't do an impl<'a, T> Index<usize> for &'a [T] of some kind.
builtin_index() returns None on TyRef, regardless of the underlying type - I considered changing this instead, but I felt this would confuse the dereferencing logic. Alternatively, I could introduce some kind of fn unsize_type to use in place of the trait argument, which would return TySlice for TyArray, which is the only case I can think of that matters.

Actually, on a second closer look, this just requires using self_ty, which is the correct unsized type, and not method.sigs.inputs()[0], in that adjustment.

Unsizing coercions happen on references and raw/smart pointers, so you first have a borrow to get &[T; N] and then an unsizing of that reference to &[T].

@ivanbakel my point was that I would feel more comfortbale "skipping" the ref if we only did it when the coercion was an Unsize coercion -- i.e., I don't like deciding to skip based on the type, that feels like it could misfire and skip when we didn't expect.

What @eddyb said made me re-examine the behaviour, and this ref skip is always necessary, because it's always introduced in the indexing logic even without an Unsize. I've rewritten the code to reflect that. Let me know if you still have the same concern.

nikomatsakis · 2018-01-04T19:28:43Z

@ivanbakel if you have a build of this branch handy, could you tell me the result of running rustc on this test case:

fnmain(){letmut i = [1,2,3];
i[i[0] - 1] = 0;}

Does it compile? I'm just curious to see if it fixes another random bug. Thanks =)

ivanbakel · 2018-01-05T03:11:30Z

Compiles, and from a little bit of println it does what I'd expect.

bors · 2018-01-06T23:34:02Z

☔ The latest upstream changes (presumably #47235) made this pull request unmergeable. Please resolve the merge conflicts.

Use of builtin indexing is now only checked after types are fully resolved. Types are not correctly checked in case of autoderefs.

Tests match issues opened on Github. Tests would not previously compile and pass.

The adjusted type is now used instead in cases of autoderefs.

If the final type coming out of an Adjust is a ref, it is deconstructed. Also made some formatting changes.

The ref was actually always necessary. This clarifies some incorrect thinking in the original code that might have led to errors in the future to do with unsizing.

nikomatsakis · 2018-01-08T14:08:36Z

+ // So the borrow discard actually happens here
+ a.pop();
+ },
+ _ => {}


Can we add some kind of assertion here? That is, the adjustment we are popping, will it always be a ref adjustment or something similar?

Eh, looking at the other cases, this looks similarly paranoid (or not paranoid). Seems fine.

nikomatsakis

This looks much better. One last question is whether there is some form of assertion we can add that would be helpful.

nikomatsakis · 2018-01-09T12:38:29Z

@bors r+

bors · 2018-01-09T12:38:30Z

📌 Commit 6132806 has been approved by nikomatsakis

bors · 2018-01-09T13:03:24Z

⌛ Testing commit 6132806 with merge 9f9ba8a5bf0182a0009f6fb71c8b5374be3123c0...

bors · 2018-01-09T16:04:17Z

💔 Test failed - status-appveyor

estebank · 2018-01-09T17:31:28Z

@bors retry

bors · 2018-01-09T18:57:15Z

⌛ Testing commit 6132806 with merge 52a9637bf87b98d1110e48c65f7507fc232f8440...

bors · 2018-01-09T21:57:46Z

💔 Test failed - status-appveyor

kennytm · 2018-01-10T12:24:25Z

@bors retry #46903

bors · 2018-01-10T12:29:12Z

⌛ Testing commit 6132806 with merge f62f774...

@eddyb

Fix built-in indexing not being used where index type wasn't "obviously" usize Fixes#33903Fixes#46095 This PR was made possible thanks to the generous help of @eddyb Following the example of binary operators, builtin checking for indexing has been moved from the typecheck stage to a writeback stage, after type constraints have been resolved.

bors · 2018-01-10T15:23:52Z

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

SimonSapin · 2018-01-11T09:04:42Z

Is this change intended to reject during borrow-checking programs that were previously accepted? #47349

nikomatsakis · 2018-01-25T14:30:34Z

Adding relnotes tag because, as @SimonSapin noted, this did cause a regression (#47349), though it appears to be a genuine bug fix. In particular, I would summarize it as:

The borrow checker was sometimes incorrectly permitting overlapping borrows around indexing operations (see #47349). This has been fixed (which also enabled some correct code that used to cause errors (e.g. #33903 and #46095).

rust-highfive assigned estebank Jan 3, 2018

eddyb reviewed Jan 3, 2018
View reviewed changes

Comment threadsrc/librustc_typeck/check/writeback.rs Outdated
eddybJan 3, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always write this as ty::TyRef

eddyb reviewed Jan 3, 2018
View reviewed changes

rust-highfive assigned nikomatsakis and unassigned estebank Jan 3, 2018

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2018

nikomatsakis reviewed Jan 4, 2018
View reviewed changes

nikomatsakis mentioned this pull request Jan 4, 2018
Arithmetic makes borrow checker overly restrictive #29743
Closed

ivanbakel added 7 commits January 7, 2018 01:30

Moved builtin indexing checks to writeback stage
23bc641
Use of builtin indexing is now only checked after types are fully resolved. Types are not correctly checked in case of autoderefs.

Added tests for non-obvious builtin indexing
bf199d4
Tests match issues opened on Github. Tests would not previously compile and pass.

Made eddyb's suggested change
8ec3696
The adjusted type is now used instead in cases of autoderefs.

Fixed unsize changing adjusted type
21ddc18
If the final type coming out of an Adjust is a ref, it is deconstructed. Also made some formatting changes.

Made requested changes
137a63c

Removed trailing whitespace
2133ace

Correct reasoning behind writeback ref removal
6132806
The ref was actually always necessary. This clarifies some incorrect thinking in the original code that might have led to errors in the future to do with unsizing.

ivanbakel force-pushed the builtin_indexing branch from 3ba7d18 to 6132806Compare January 7, 2018 01:31

nikomatsakis reviewed Jan 8, 2018
View reviewed changes

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2018

bors merged commit 6132806 into rust-lang:masterJan 10, 2018

SimonSapin mentioned this pull request Jan 11, 2018
Improve diagnostics to guide users past Borrow checking regression in brotli #47349
Closed

nikomatsakis added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 25, 2018

kennytm mentioned this pull request Mar 4, 2018
Indexing regression in beta #48715
Closed

emilyalbini mentioned this pull request Apr 26, 2018
Can't overload indexing for arrays #49786
Closed

beepster4096 mentioned this pull request Aug 6, 2026
ConstantIndex and Index not flagged as potential aliases #160525
Open

Uh oh!

Uh oh!

Conversation

ivanbakel commented Jan 3, 2018

Uh oh!

rust-highfive commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 4, 2018

Uh oh!

ivanbakel commented Jan 5, 2018

Uh oh!

bors commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 9, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

estebank commented Jan 9, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

kennytm commented Jan 10, 2018

Uh oh!

bors commented Jan 10, 2018

Uh oh!

bors commented Jan 10, 2018

Uh oh!

SimonSapin commented Jan 11, 2018

Uh oh!

nikomatsakis commented Jan 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants