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

Implement (most of) RFC 1214 #27641

Merged
merged 38 commits into from
Aug 14, 2015
Merged

Conversation

nikomatsakis
Copy link
Contributor

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
Copy link
Contributor

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

@nikomatsakis
Copy link
Contributor Author

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

@@ -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>>),
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing doc comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is temporary anyway, but still.

@arielb1
Copy link
Contributor

arielb1 commented Aug 10, 2015

@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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

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.

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

Choose a reason for hiding this comment

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

no note_obligation_cause there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@nikomatsakis
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

arielb1 commented Aug 11, 2015

@nikomatsakis

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

// regions. This is perhaps not ideal.
self.from_object_ty(ty, data);

// FIXME(#27579) RFC also considers adding trait
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

arielb1 commented Aug 11, 2015

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

@bors
Copy link
Contributor

bors commented Aug 12, 2015

☔ 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.
@nikomatsakis
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Aug 14, 2015

📌 Commit 7f8942c has been approved by nrc

nikomatsakis added a commit to nikomatsakis/nickel.rs that referenced this pull request Aug 14, 2015
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
Copy link
Contributor

Marwes commented Aug 14, 2015

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.

bors added a commit that referenced this pull request Aug 14, 2015
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
Copy link
Contributor

bors commented Aug 14, 2015

⌛ Testing commit 7f8942c with merge e7261f3...

@nikomatsakis
Copy link
Contributor Author

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.

nikomatsakis added a commit to nikomatsakis/nickel.rs that referenced this pull request Aug 14, 2015
… 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.
Ryman pushed a commit to nickel-org/nickel.rs that referenced this pull request Aug 14, 2015
… 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.
Ryman added a commit to nickel-org/nickel.rs that referenced this pull request Aug 14, 2015
bors-servo pushed a commit to servo/tendril that referenced this pull request Aug 15, 2015
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.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/tendril/18)
<!-- Reviewable:end -->
Marwes added a commit to Marwes/combine that referenced this pull request Aug 16, 2015
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 18, 2015
@nikomatsakis nikomatsakis deleted the soundness-rfc-1214 branch March 30, 2016 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants