stabilize outlives requirements #53793

toidiu · 2018-08-29T17:11:29Z

toidiu · 2018-08-29T17:13:25Z

src/test/ui/lint/lint-ctypes.rs

@@ -10,7 +10,7 @@

 #![deny(improper_ctypes)]
 #![feature(libc)]
-
+#![allow(private_in_public)]


I was unsure why the error changed and why i needed this. Also is there is a better way to handle this?

toidiu · 2018-08-29T17:14:54Z

I forgot to update the documentation. So will be pushing another commit.

killercup · 2018-08-29T19:18:47Z

s/stabalize/stabilize

nikomatsakis

So the main thing is that I think we should make a bigger effort to preserve some of the existing tests rather than deleting them. In particular, they were testing aspects of the WF definition and they just happened to be doing so as part of enum/struct fields. There are other ways to test that same code so we ought to port.

nikomatsakis · 2018-08-29T19:18:37Z

src/librustc_mir/lib.rs

@@ -15,7 +15,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment!
 */

 #![cfg_attr(not(stage0), feature(nll))]
-#![cfg_attr(not(stage0), feature(infer_outlives_requirements))]
+#![cfg_attr(stage0, feature(infer_outlives_requirements))]


Do we need this, really? It seems like we can't be relying on it...

although https://forge.rust-lang.org/stabilization-guide.html says that we want to leave it to compile the stage0 compiler.

src/test/ui/coherence/coherence-inherited-assoc-ty-cycle-err.stderr

nikomatsakis · 2018-08-29T19:41:07Z

src/test/ui/issues/issue-37323.rs

-// except according to those terms.
-
-#![feature(rustc_attrs)]
-#![allow(warnings)]


we can add a // compile-pass comment instead of deleting these tests

nikomatsakis · 2018-08-29T19:43:05Z

src/test/ui/regions/regions-enum-not-wf.rs

-
-enum Ref1<'a, T> {
-    Ref1Variant1(&'a T) //~ ERROR the parameter type `T` may not live long enough
-}


Hmm, I'm not sure about deleting these tests. Maybe we should change them (for now) to use 'static instead of 'a? Seems like it would preserve the original intention of the test .. more or less.

Alternatively, we could add this to the test:

trait Dummy<'a> { type Out; } impl<'a, T> Dummy<'a> for T where T: 'a { type Out = (); } type RequireOutlives<'a, T> = <T as Dummy<'a>>::Out;

and then replace &'a T with RequireOutlives<'a, T>.

This just exploits a weakness in the inference. Kind of horrible though. Maybe move that code over into rfc-2093-infer-outlives directory, too? (e.g., to test the cases where inference is expected to fail)

I suppose in principle we could keep the original test too and make it // compile-pass but...presumably... we added equivalent tests for infer-outlives? Yeah, I see things like src/test/ui/rfc-2093-infer-outlives/enum.rs, which seem pretty similar.

nikomatsakis · 2018-08-29T19:47:48Z

src/test/ui/regions/regions-outlives-nominal-type-enum-type.rs

-        x: T
-    }
-    enum Bar<'a,'b> {
-        F(&'a Foo<&'b i32>) //~ ERROR reference has a longer lifetime


we could .. I think .. test this like so:

trait Trait<'a, 'b> { type Out; } impl<'a, 'b> Trait<'a, 'b> { type Out = &'a Foo<&'b i32>; //~ ERROR reference has a longer lifetime }

src/test/ui/rfc-2093-infer-outlives/enum.rs

nikomatsakis · 2018-08-29T19:54:39Z

src/test/ui/wf/wf-outlives-ty-in-fn-or-trait.rs

-trait Trait<T> { }
-
-struct Foo<'a,T> {
-    f: &'a fn(T),


we can rewrite this using the trait/impl trick like

trait Dummy<'a> { type Out; } impl<'a, T> Dummy<'a> for T { type Out = &'a fn(T); //~ ERROR E0389

nikomatsakis · 2018-08-29T19:54:43Z

src/test/ui/wf/wf-outlives-ty-in-fn-or-trait.rs

-
-struct Bar<'a,T> {
-    f: &'a Trait<T>,
-    //~^ ERROR E0309


nikomatsakis · 2018-08-29T19:54:53Z

src/test/ui/wf/wf-outlives-ty-in-fn-or-trait.rs

-
-// Test that an appearance of `T` in fn args or in a trait object must
-// still meet the outlives bounds. Since this is a new requirement,
-// this is currently only a warning, not a hard error.


this is out of date, I guess, though not your fault :)

src/librustc_typeck/collect.rs

toidiu · 2018-09-01T01:52:31Z

@nikomatsakis believe i addressed all your comments. I added // compile-pass to some test, moved some test to 2093 dir and also used the trait/impl trick to preserve the original tests.

There are some cases where we have now have redundant tests but I am not certain if that is a bad thing.

Docs:
I removed the docs from unstable.
Still need to add docs to reference, book, by example. Will get those merged in their respective repos first.

zackmdavis · 2018-09-02T01:22:05Z

@toidiu It may be worth coordinating this with #53013 (a lint for soon-to-be-unnecessary outlives-bounds), the current version of which introduces a new check for the infer_outlives_requirements feature gate, such that if this lands first, #53013 will have to be revised (to remove 27b2237bde8), and if #53013 lands first, then this will have to be revised. Maybe we should put together a joint PR with both sets of changes?

toidiu · 2018-09-04T18:22:13Z

@zackmdavis I ran into some trouble testing and reproducing the errors locally. Its a short week but I would like to get this ready for merge by end of week.

If however your PR is ready then please go ahead (dont want this to be a blocker) and I can easily rebase.

toidiu · 2018-09-05T04:35:22Z

@nikomatsakis I am getting a cryptic error and dont know how to approach it/reproduce it locally or what exactly is causing the error :(

failures:
/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0309 (line 4762)
 /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0309 (line 4782)
 /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0491 (line 7781)

zackmdavis · 2018-09-05T04:43:38Z

@toidiu I think those are error-index examples (the text that gets displayed when you run --explain E0XXX to get more information about an error code) being run as doctests: 1 2.

nikomatsakis · 2018-09-05T17:47:57Z

@toidiu

to be more specific, on master, the definition for E0309 includes this:

rust/src/librustc/diagnostics.rs

Lines 1240 to 1246 in d8af8b6

    
           ```compile_fail,E0309 
        
           // This won't compile because T is not constrained, meaning the data 
        
           // stored in it is not guaranteed to last as long as the reference 
        
           struct Foo<'a, T> { 
        
               foo: &'a T 
        
           } 
        
           ```

this code no longer generates the error, so we have to rework the example. In fact, I think we should rework the whole section for E0309. In particular, I think that this error can only really arise due to associated types in the structure definition, so maybe we can specialize the text:

The type definition contains some field whose type
requires an outlives annotation. Outlives annotations
(e.g., T: 'a) are used to guarantee that all the data in T is valid
for at least the lifetime 'a. This scenario most commonly
arises when the type contains an associated type reference
like <T as SomeTrait<'a>>::Out, as shown in this example:

// This won't compile because the applicable impl of
// `SomeTrait` (below) requires that `T: 'a`, but the struct does
// not have a matching where-clause.
struct Foo<'a, T> {
    foo: <T as SomeTrait<'a>>::Out
}

trait SomeTrait<'a> {
    type Output;
}

impl<'a, T> SomeTrait<'a> for T
where
    T: 'a,
{ }

Here, the where clause T: 'a that appears on the impl is not known to be
satisfied on the struct. To make this example compile, you have to add
a where-clause like T: 'a to the struct definition:

struct Foo<'a, T>
where
    T: 'a,
{
    foo: <T as SomeTrait<'a>>::Out
}

trait SomeTrait<'a> {
    type Output;
}

impl<'a, T> SomeTrait<'a> for T
where
    T: 'a,
{ }

nikomatsakis · 2018-09-05T17:51:18Z

For E0491, we'll need to make a similar adjustment to the example.

A reference has a longer lifetime than the data it references.

Erroneous code example:

trait SomeTrait<'a> {
    type Output;
}

impl<'a, T> SomeTrait<'a> for T {
    type Output = &'a T; // compile error E0491
}

Here, the problem is that a reference type like &'a T is only valid
if all the data in T outlives the lifetime 'a. But this impl as written
is applicable to any lifetime 'a and any type T -- we have no guarantee
that T outlives 'a. To fix this, you can add a where clause like where T: 'a.

trait SomeTrait<'a> {
    type Output;
}

impl<'a, T> SomeTrait<'a> for T
where
    T: 'a,
{
    type Output = &'a T; // compile error E0491
}

nikomatsakis · 2018-09-05T17:53:16Z

I took the liberty of pasting those descriptions in.

nikomatsakis · 2018-09-04T18:55:55Z

src/test/ui/lint/lint-ctypes.rs

@@ -11,6 +11,8 @@
 #![deny(improper_ctypes)]
 #![feature(libc)]

+#![allow(private_in_public)]


why this change?

The test was failing for me locally until I added this. Although I am wondering if it was simply a bad local setup (same with a few other tests).

I would like to try and get to a passing state and then try to undo this change to see if things still pass.

nikomatsakis · 2018-09-04T18:56:11Z

src/test/ui/no_owned_box_lang_item.rs

@@ -15,10 +15,12 @@
 #![feature(lang_items, box_syntax)]
 #![no_std]

+use core::panic::PanicInfo;


similarly this?

same as above.

I don't quite know why this one changed, but I'm .. less concerned about it. Probably worth figuring out but might have to do with code executing in a somewhat different order than before owing to the new queries.

rust-highfive · 2018-09-06T05:51:25Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:47:32] ....................................................................................................
[00:47:35] ....................................................................................................
[00:47:38] ...............................i....................................................................
[00:47:41] ....................................................................................................
[00:47:43] .................................................................................iiiiiiiii..........
[00:47:49] ..ii................................................................................................
[00:47:52] ....................................................................................................
[00:47:55] .............................................................i......................................
[00:47:58] ....................................................................................................
---
[01:15:05] test /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0658 (line 10938) ... ok
[01:15:05] test /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0689 (line 11021) ... ok
[01:15:05] test /checkout/obj/build/x86_64-unknown-lp test
[01:15:05] Build completed unsuccessfully in 0:31:29
[01:15:05] Makefile:58: recipe for target 'check' failed
[01:15:05] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1919a06e
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

toidiu · 2018-09-06T18:13:02Z

Looks like a passing PR. Going to remove #![allow(private_in_public)] and use core::panic::PanicInfo; to see if we actually need those.

@nikomatsakis

stabilize outlives requirements #44493 r? @nikomatsakis

bors · 2018-09-12T14:13:48Z

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

nnethercote · 2018-09-22T03:01:45Z

This hurt compile speed for unused-warnings by up to 3.5%:
https://perf.rust-lang.org/compare.html?start=8586ec6980462c99a8926646201b2444d8938d29&end=6810f5286b6b91daab06fc3dccb27d8c46f14349&stat=instructions:u

@toidui: any thoughts?

nnethercote · 2018-09-22T06:31:11Z

It also hurt max-rss on incremental builds by up to 10%:
https://perf.rust-lang.org/compare.html?start=8586ec6980462c99a8926646201b2444d8938d29&end=6810f5286b6b91daab06fc3dccb27d8c46f14349&stat=max-rss

toidiu · 2018-09-23T05:29:29Z

@nnethercote this was the first large PR i committed to rust so please excuse my lack of knowledge and procedure. What steps can I take to rectify the degraded performance?

I am also available on rustlang discord and zulip (username: toidiu) is some of this is better for offline discussion.

nnethercote · 2018-09-24T06:24:15Z

@toidiu: if you are on Linux, the best way would be to do Cachegrind runs on two revisions -- the one before your change landed, and the following revision -- and then do a diff. Instructions on how to do a Cachegrind run are here: https://github.com/rust-lang-nursery/rustc-perf/blob/master/collector/README.md

eddyb · 2018-09-24T09:50:38Z

@nikomatsakis or @arielb1 could also look at this - they already have things set up for profiling.

nnethercote · 2018-09-24T22:35:44Z

They could, but I want to encourage more people to become familiar with profiling the compiler. It's not too hard now, if you're on Linux. And if there are ideas on how to make it easier, I'd love to hear them.

toidiu · 2018-09-27T04:20:04Z

@nnethercote

So from what I understand

I need two different repos on the different branches
then build both versions of rustc ./x.py build
then run perf profiler for each beanch

I am wondering if its possible to download the pre-compiled versions of the two branches from somewhere rather than building them locally (takes a long time)?

nnethercote · 2018-09-27T09:19:41Z

So from what I understand
* I need two different repos on the different branches
* then build both versions of rustc ./x.py build
* then run perf profiler for each beanch

Correct!

I am wondering if its possible to download the pre-compiled versions of the two branches from somewhere rather than building them locally (takes a long time)?

Not that I know of.

toidiu · 2018-10-03T00:16:01Z

@nnethercote Btw will this work on a mac? I got the error

 INFO 2018-10-03T00:14:37Z: collector: benchmarking commit Orig (2018-10-03 00:14:37 UTC) for triple x86_64-unknown-linux-gnu
thread 'main' panicked at 'assertion failed: has_perf', collector/src/bin/rustc-perf-collector/execute.rs:306:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

nnethercote · 2018-10-03T00:27:41Z

Benchmarking (with bench_local) won't work on Mac, because it depends on perf. Profiling with Cachegrind will work in theory on Mac, though I haven't tried it myself.

toidiu · 2018-10-04T03:40:11Z

I am getting the following errors on my mac:
cmd:

 RUST_LOG=info ./target/release/collector --output-repo OUT \
profile cachegrind --rustc ~/project/rust/build/x86_64-apple-darwin/stage2/bin/rustc --cargo `which cargo` Outlives

error:

INFO 2018-10-03T01:18:25Z: collector: failed to profile cargo with Cachegrind, recorded: ErrorMessage { msg: "expected success, got exit code: 64\n\nstderr=cp: illegal option -- T\nusage: cp [-R [-H | -L | -P]] [-fi | -n] [-apvX] source_file target_file\n       cp [-R [-H | -L | -P]] [-fi | -n] [-apvX] source_file ... target_directory\n\n\n stdout=" }

nnethercote · 2018-10-05T22:47:44Z

illegal option -- T

I have fixed that in rust-lang/rustc-perf#288.

Unfortunately, it's not enough to get it working, at least on my MacOS 10.12 laptop. Cachegrind starts up ok but doesn't produce an output file, for reasons I haven't yet determined.

…tsakis" This reverts commit 6810f52, reversing changes made to 8586ec6.

toidiu · 2018-10-08T05:21:47Z

Yes ran it locally and also still getting an error. Here is the full output: https://pastebin.com/xhjEKKr5

nnethercote · 2018-10-09T02:12:39Z

assertion failed: has_valgrind

This means you don't have Valgrind installed.

[beta] back out #53793 - stabilize outlives requirements Fixes #54467 for beta, looks like a less risky fix than #54701

toidiu · 2018-10-11T01:34:18Z

I installed Valgrind ran the collector. There were still some errors and eventually the program seemed to stall without making any progress (over a few hours). Also there was no output saved.

@nnethercote I am wondering if there is a path forward where I dont have to rely on my local machine which is not a Linux.

nnethercote · 2018-10-11T02:39:58Z

No path I can see, unfortunately.

arielb1 · 2018-10-13T13:08:53Z

@toidiu

You can rent an AWS and work ssh-ed into it (tip: use screen). I do that when I want a faster computer than my laptop for some things.

toidiu · 2018-10-14T01:43:55Z

I haz procured a Linux. I started running the collector and it seems to be taking a very long time.. been running for multiple hours. Any idea how long it will take to complete?

@nnethercote since you asked before; making it easier to get see and analyze the results might an area for improvement. I imagine that someone somewhere already ran these results to conclude that this PR caused a regression. I wonder if it would be possible to reuse those outputs rather then having to rerun them locally?

rust-highfive assigned nikomatsakis Aug 29, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2018

toidiu commented Aug 29, 2018

View reviewed changes