Implement (most of) RFC 1214 #27641

nikomatsakis · 2015-08-10T19:21:46Z

This PR implements the majority of RFC 1214. In particular, it implements:

the new outlives relation
comprehensive WF checking

For the most part, new code receives warnings, not errors, though 3 regressions were found via a crater run.

There are some deviations from RFC 1214. Most notably:

we still consider implied bounds from fn ret; this intersects other soundness issues that I intend to address in detail in a follow-up RFC. Fixing this without breaking a lot of code probably requires rewriting compare-method somewhat (which is probably a good thing).
object types do not check trait bounds for fear of encountering Self; this was left as an unresolved question in RFC 1214, but ultimately feels inconsistent.

Both of those two issues are highlighted in the tracking issue, #27579. #27579 also includes a testing matrix with new tests that I wrote -- these probably duplicate some existing tests, I tried to check but wasn't quite sure what to look for. I tried to be thorough in testing the WF relation, at least, but would welcome suggestions for missing tests.

r? @nrc (or perhaps someone else?)

eefriedman · 2015-08-10T20:08:24Z

The binary file src/test/compile-fail/wf-trait-associated-type-trait shouldn't be committed.

nikomatsakis · 2015-08-10T20:12:58Z

Note to self: maybe restore the code that bans where clauses w/ no type parameters, even if it's not very useful?

eefriedman · 2015-08-10T20:13:31Z

src/librustc/middle/infer/mod.rs

@@ -188,6 +189,8 @@ pub struct TypeTrace<'tcx> {
 /// See `error_reporting.rs` for more details
 #[derive(Clone, Debug)]
 pub enum SubregionOrigin<'tcx> {
+    RFC1214Subregion(Rc<SubregionOrigin<'tcx>>),


Missing doc comment.

This is temporary anyway, but still.

arielb1 · 2015-08-10T21:37:40Z

@nikomatsakis

I would like to at least prevent where-clauses on non-generic methods - these will be translated anyway and cause ICEs when the obligations can't be fulfilled.

arielb1 · 2015-08-10T22:57:51Z

src/librustc/middle/expr_use_visitor.rs

@@ -273,7 +273,9 @@ enum PassArgs {
 impl<'d,'t,'a,'tcx> ExprUseVisitor<'d,'t,'a,'tcx> {
    pub fn new(delegate: &'d mut Delegate<'tcx>,
               typer: &'t infer::InferCtxt<'a, 'tcx>)
-               -> ExprUseVisitor<'d,'t,'a, 'tcx> {
+               -> ExprUseVisitor<'d,'t,'a,'tcx>
+        where 'tcx: 't // FIXME(#27583) workaround apparent stage0 bug


any progress with the bug? I guess I can play with this after this lands.

@arielb1

any progress with the bug? I guess I can play with this after this lands.

no, I haven't come back to it yet.

nikomatsakis · 2015-08-10T23:15:42Z

I think I will just restore the rules as they were, and write up a separate rfc to try and rationalize them.

Niko

-------- Original message --------

From: arielb1 notifications@github.com

Date:08/10/2015 17:37 (GMT-05:00)

To: rust-lang/rust rust@noreply.github.com

Cc: Niko Matsakis niko@alum.mit.edu

Subject: Re: [rust] Implement (most of) RFC 1214 (#27641)

@nikomatsakis

I would like to at least prevent where-clauses on non-generic methods - these will be translated anyway and cause ICEs when the obligations can't be fulfilled.

—
Reply to this email directly or view it on GitHub.

arielb1 · 2015-08-10T23:21:28Z

src/librustc/middle/traits/error_reporting.rs

+                            if is_warning {
+                                note_obligation_cause(infcx, obligation);
+                            } else if let Some(s) = custom_note {
+                                infcx.tcx.sess.span_note(obligation.cause.span, &s);


no note_obligation_cause there?

@arielb1

no note_obligation_cause there?

Debatable. I was thinking that the custom message plays the role of this note, but I guess they are potentially somewhat orthogonal.

this really improves the error message for various "[u8] does not implement Sized"-s.

nikomatsakis · 2015-08-11T13:28:47Z

Thanks for the suggestions, @eefriedman and @arielb1. I'll be pushing some updates soon. Just debating about whether to switch from ty.walk at the moment. I agree with @arielb1 that enumerating cases is usually more reliable. I'll probably do a hybrid.

nikomatsakis · 2015-08-11T18:38:34Z

@arielb1

this is a rather hot path - could you check the perf impact?

Sure, but a predicate is (I think) POD, so I wouldn't expect this to be hyper expensive. Of course #[derive(Clone)] is not generating optimal code here.

arielb1 · 2015-08-11T18:58:59Z

@nikomatsakis

It is. Could you just make it Copy? (you also need Predicate, TraitPredicate, OutlivesPredicate, ProjectionPredicate, ProjectionTy).

arielb1 · 2015-08-11T19:59:43Z

src/librustc/middle/wf.rs

+                    // regions. This is perhaps not ideal.
+                    self.from_object_ty(ty, data);
+
+                    // FIXME(#27579) RFC also considers adding trait


Couldn't we check the obligations with Self being the object type? i.e. if you have trait Foo<T> where T: Eq<Box<Self>>, Foo<T> wf would require that T: Eq<Box<Self>>. I'm not sure how important is this, because (am I wrong here?) non-supertrait predicates are never elaborated and therefore can be safely ignored (up to #27675 anyway).

@arielb1

Couldn't we check the obligations with Self being the object type?

Interesting thought. I'll have to think it over. It would certainly be nice if so. It may interact with the restrictions on object safety and #27675.

I'm not sure how important is this, because (am I wrong here?) non-supertrait predicates are never elaborated and therefore can be safely ignored

This is true but I have considered changing this (#20671).

arielb1 · 2015-08-11T21:30:25Z

I would also include a test for aturon's bug (#27592), if it is indeed fixed.

bors · 2015-08-12T03:17:16Z

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

after we check casts, because sometimes casts can influence inference, unfortunately. We do re-run `select_all_trait_obligations` during regionck anyhow.

`implicator`. These definitions are also in accordance with RFC 1214 (or more so), and hence somewhat different from the implicator. This commit also modifies the implicator to remove the older rules for projections, which can easily trigger infinite loops.

on more kind of items

nikomatsakis · 2015-08-14T13:34:16Z

So I ran a crater run with a variant of @arielb1's proposed rules for handling projection outlines. In particular, these rules never (I think) introduce overly conservative constraints into the region graph; instead they run the risk of the region graph being underconstrained. I found two new failures versus previous runs, but those seem to be crates that have been newly introduced. One of the failures is due to #27834, which is a pre-existing issue. I have not yet diagnosed the second failure.

nikomatsakis · 2015-08-14T13:51:49Z

OK, after investigating the other failure (combine-1.0.0-beta2), I've found that it is legitimate, caused by a bug fix in this PR. I suspect it is caused by doing more normalization. I'm not sure how to make a warning out of this one, so I'm just going to leave it as an error and submit a patch to the relevant repository.

nikomatsakis · 2015-08-14T13:53:19Z

@bors r=nrc

bors · 2015-08-14T13:53:19Z

📌 Commit 7f8942c has been approved by nrc

the lifetime parameters on request ('a, 'b, 'k) seem to play the following roles: 'a, 'b -- these represent the processing of this individual request. They can vary. 'k -- this represents the lifetime of the server's internal, mutable storage. It is fixed. If you only have two parameters 'x and 'y to supply, then, the correct pattern is `'x, 'x, 'y`, because then `'x` plays the role of the intersection of `'a` and `'b`, but `'y` is pinned to the server's internal storage.

Marwes · 2015-08-14T15:13:02Z

Author of combine here. Got a heads up from @nikomatsakis about this and it should be possible to fix easily. I will upload a fix once I can test it on nightly.

@nrc

This PR implements the majority of RFC 1214. In particular, it implements: - the new outlives relation - comprehensive WF checking For the most part, new code receives warnings, not errors, though 3 regressions were found via a crater run. There are some deviations from RFC 1214. Most notably: - we still consider implied bounds from fn ret; this intersects other soundness issues that I intend to address in detail in a follow-up RFC. Fixing this without breaking a lot of code probably requires rewriting compare-method somewhat (which is probably a good thing). - object types do not check trait bounds for fear of encountering `Self`; this was left as an unresolved question in RFC 1214, but ultimately feels inconsistent. Both of those two issues are highlighted in the tracking issue, #27579. #27579 also includes a testing matrix with new tests that I wrote -- these probably duplicate some existing tests, I tried to check but wasn't quite sure what to look for. I tried to be thorough in testing the WF relation, at least, but would welcome suggestions for missing tests. r? @nrc (or perhaps someone else?)

bors · 2015-08-14T15:26:11Z

⌛ Testing commit 7f8942c with merge e7261f3...

bors · 2015-08-14T17:57:16Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

nikomatsakis · 2015-08-14T18:44:30Z

OK, I believe I have posted PRs for all broken crates, apart from combine, which I wasn't quite sure how best to fix. There are of course many others that received warnings -- I may try to fix some of those too.

… from mw state Correct lifetimes in response to errors found by rust-lang/rust#27641; the lifetime parameters on request ('a, 'b, 'k) seem to play the following roles: 'a, 'b -- these represent the processing of this individual request. They can vary. 'k -- this represents the lifetime of the server's internal, mutable storage. It is fixed. If you only have two parameters 'x and 'y to supply, then, the correct pattern is `'x, 'x, 'y`, because then `'x` plays the role of the intersection of `'a` and `'b`, but `'y` is pinned to the server's internal storage.

… from mw state Correct lifetimes in response to errors found by rust-lang/rust#27641; the lifetime parameters on request ('a, 'b, 'k) seem to play the following roles: 'a, 'b -- these represent the processing of this individual request. They can vary. 'k -- this represents the lifetime of the server's internal, mutable storage. It is fixed. If you only have two parameters 'x and 'y to supply, then, the correct pattern is `'x, 'x, 'y`, because then `'x` plays the role of the intersection of `'a` and `'b`, but `'y` is pinned to the server's internal storage. BREAKING CHANGE: Most type signatures involving `Request` will have broken, to fix code broken by this change, you will need to remove the third lifetime.

Fix region errors uncovered by rust-lang/rust#27641

Update tendril to avoid double borrow. Update tendril to avoid double borrow. This double borrow is detected by the latest Nightly builds, or at least will be once rust-lang/rust#27641 makes it into such a build.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/tendril/18)

As mentioned in rust-lang/rust#27641

rust-highfive assigned nrc Aug 10, 2015

eefriedman reviewed Aug 10, 2015
View reviewed changes

arielb1 reviewed Aug 10, 2015
View reviewed changes

arielb1 reviewed Aug 11, 2015
View reviewed changes

nikomatsakis added 4 commits August 12, 2015 17:57

Don't report a hard error if there are inference failures until

4561607

after we check casts, because sometimes casts can influence inference, unfortunately. We do re-run `select_all_trait_obligations` during regionck anyhow.

Add two new kinds of predicates, WellFormed and ObjectSafe.

b196315

Extend ParameterEnvironment to remember the free_id, and to be usable

ad47bd8

on more kind of items

nikomatsakis mentioned this pull request Aug 14, 2015

Fix region errors uncovered by rust-lang/rust#27641 nickel-org/nickel.rs#260

Closed

bors merged commit 7f8942c into rust-lang:master Aug 14, 2015

This was referenced Aug 14, 2015

Update tendril to avoid double borrow. servo/tendril#18

Merged

Add missing Iterator bound Emerentius/almost_ord#1

Closed

Ryman added a commit to nickel-org/nickel.rs that referenced this pull request Aug 14, 2015

Merge pull request #260 from nikomatsakis/master

4efdb8b

Fix region errors uncovered by rust-lang/rust#27641

Marwes added a commit to Marwes/combine that referenced this pull request Aug 16, 2015

Add missing Clone bounds detected on todays nightly

b706f9d

As mentioned in rust-lang/rust#27641

kevinmehall mentioned this pull request Aug 16, 2015

RFC: Scoped threads, take 2 rust-lang/rfcs#1084

Closed

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 18, 2015

apasel422 mentioned this pull request Oct 24, 2015

Make {Default, From, FromIterator, One, Zero} well-formed #29266

Merged

nikomatsakis deleted the soundness-rfc-1214 branch March 30, 2016 16:17

steffahn mentioned this pull request Apr 23, 2021

Weird error in trait with HRTB for the same trait on Self in a method. Also bad diagnostic with nonsensical suggestion. #84435

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement (most of) RFC 1214 #27641

Implement (most of) RFC 1214 #27641

nikomatsakis commented Aug 10, 2015

eefriedman commented Aug 10, 2015

nikomatsakis commented Aug 10, 2015

eefriedman Aug 10, 2015

arielb1 Aug 10, 2015

arielb1 commented Aug 10, 2015

arielb1 Aug 10, 2015

nikomatsakis Aug 11, 2015

nikomatsakis commented Aug 10, 2015

arielb1 Aug 10, 2015

nikomatsakis Aug 11, 2015

arielb1 Aug 11, 2015

nikomatsakis commented Aug 11, 2015

nikomatsakis commented Aug 11, 2015

arielb1 commented Aug 11, 2015

arielb1 Aug 11, 2015

nikomatsakis Aug 12, 2015

arielb1 commented Aug 11, 2015

bors commented Aug 12, 2015

nikomatsakis commented Aug 14, 2015

nikomatsakis commented Aug 14, 2015

nikomatsakis commented Aug 14, 2015

bors commented Aug 14, 2015

Marwes commented Aug 14, 2015

bors commented Aug 14, 2015

bors commented Aug 14, 2015

nikomatsakis commented Aug 14, 2015

Implement (most of) RFC 1214 #27641

Implement (most of) RFC 1214 #27641

Conversation

nikomatsakis commented Aug 10, 2015

eefriedman commented Aug 10, 2015

nikomatsakis commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Aug 11, 2015

nikomatsakis commented Aug 11, 2015

arielb1 commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Aug 11, 2015

bors commented Aug 12, 2015

nikomatsakis commented Aug 14, 2015

nikomatsakis commented Aug 14, 2015

nikomatsakis commented Aug 14, 2015

bors commented Aug 14, 2015

Marwes commented Aug 14, 2015

bors commented Aug 14, 2015

bors commented Aug 14, 2015

nikomatsakis commented Aug 14, 2015