detect extra region requirements in impls #37167

nikomatsakis · 2016-10-14T14:36:24Z

The current "compare method" check fails to check for the "region obligations" that accrue in the fulfillment context. This branch switches that code to create a FnCtxt so that it can invoke the regionck code. Previous crater runs (I haven't done one with the latest tip) have found some small number of affected crates, so I went ahead and introduced a warning cycle. I will kick off a crater run with this branch shortly.

This is a [breaking-change] because previously unsound code was accepted. The crater runs also revealed some cases where legitimate code was no longer type-checking, so the branch contains one additional (but orthogonal) change. It improves the elaborator so that we elaborate region requirements more thoroughly. In particular, if we know that &'a T: 'b, we now deduce that T: 'b and 'a: 'b.

I invested a certain amount of effort in getting a good error message. The error message looks like this:

error[E0276]: impl has stricter requirements than trait
  --> traits-elaborate-projection-region.rs:33:5
   |
21 |     fn foo() where T: 'a;
   |     --------------------- definition of `foo` from trait
...
33 |     fn foo() where U: 'a { }
   |     ^^^^^^^^^^^^^^^^^^^^^^^^ impl has extra requirement `U: 'a`
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #18937 <https://github.com/rust-lang/rust/issues/18937>
note: lint level defined here
  --> traits-elaborate-projection-region.rs:12:9
   |
12 | #![deny(extra_requirement_in_impl)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^

Obviously the warning only prints if this is a new error (that resulted from the bugfix). But all existing errors that fit this description are updated to follow the general template. In order to get the lint to preserve the span-labels and the error code, I separate out the core Diagnostic type (which encapsulates the error code, message, span, and children) from the DiagnosticBuilder (which layers on a Handler that can be used to report errors). I also extended add_lint with an alternative add_lint_diagnostic that takes in a full diagnostic (cc @jonathandturner for those changes). This doesn't feel ideal but feels like it's moving in the right direction =).

r? @pnkfelix
cc @arielb1

Fixes #18937

arielb1 · 2016-10-14T18:01:04Z

src/librustc_typeck/check/compare_method.rs

+                                                                   trait_param_env,
+                                                                   normalize_cause.clone());
+
+    tcx.infer_ctxt(None, Some(trait_param_env), Reveal::NotSpecializable).enter(|infcx| {


indentation? the previous 20 or so lines should be de-indented because they are no longer within the infcx.

arielb1 · 2016-10-14T18:04:29Z

src/librustc_errors/diagnostic_builder.rs

+/// it easy to declare such methods on the builder.
+macro_rules! forward {
+    // Forward pattern for &self -> &Self
+    (pub fn $n:ident(&self, $($name:ident: $ty:ty),*) -> &Self) => {


nit: not sure a non-trivial macro is better than copy-paste here.

seconded @arielb1 's comment... feels like it makes the code a little harder to read

(i'm fine with the macro personally, but I can see that given the simplicity of these method bodies, this battle may not be worth fighting.)

Hmm. So I was going to remove the macro but hesitated for two reasons:

I consider this a fairly trivial macro. =) But I can see that if you aren't familiar with macros you might not feel that way. And I was disappointed that I wound up requiring the three arms.

The macro gives me an obvious place to put some documentation for why this forwarding is needed at all. Without the macro, I'd probably be inclined to just remove it, or else I have to just leave it dangling in the middle of the impl, where people are unlikely to see it.

Thoughts?

arielb1 · 2016-10-14T18:10:43Z

src/librustc/lint/context.rs

 }

-impl EarlyLintMessage for String {


nit: merge this and the previous commit?

arielb1 · 2016-10-14T18:14:30Z

src/librustc/traits/error_reporting.rs

@@ -866,6 +866,11 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                err.note(&format!("required so that reference `{}` does not outlive its referent",
                                  ref_ty));
            }
+            ObligationCauseCode::ObjectTypeBound(object_ty, region) => {


Test for this error message?

arielb1 · 2016-10-14T18:16:26Z

src/librustc/infer/mod.rs

+        }
+    }
+
+    pub fn from_cause<F>(cause: &traits::ObligationCause<'tcx>,


nit: from_obligation_cause? everything is some sort of a cause.

arielb1 · 2016-10-14T18:56:37Z

src/librustc_typeck/check/compare_method.rs

+                                     trait_item_span: Option<Span>,
+                                     old_broken_mode: bool) {
+    debug!("compare_impl_method(impl_trait_ref={:?})",
+           impl_trait_ref);


2 duplicate calls to debug!?

I would also prefer the lines below this to be refactored out to 4 or so functions.

heh, at first I was going to say that having two debug! calls isn't a sin, but then I realized that @arielb1 is literally pointing out that the payload itself (impl_trait_ref) is duplicated in both.

arielb1 · 2016-10-14T19:10:18Z

src/librustc_typeck/check/compare_method.rs

+            infcx.resolve_regions_and_report_errors(&free_regions, impl_m_body_id);
+        } else {
+            let fcx = FnCtxt::new(&inh, tcx.types.err, impl_m_body_id);
+            fcx.regionck_item(impl_m_body_id, impl_m_span, &[]);


~~This also calls relate_free_regions from the impl. However, the liberated LBRs here come from the trait, so this is quite bogus, and even if it wasn't, you must avoid cases such as~~

trait Foo { fn foo<'a, 'b>(x: &'a &'b u32, y: &'a &'b ()); } impl Foo for () { fn foo<'a, 'b>(x: &'b &'a u32, y: &'a &'b ()); }

~~Where the impl function's implied bounds tell you that 'a = 'b, which bypasses the subtype check.~~

Ignore this - I misread regionck_item. You might want to assume that the trait fn is WF, through.

arielb1 · 2016-10-14T19:28:58Z

The type-checking section LGTM modulo nits. I'll like @jonathandturner to look at the diagnostics section.

sophiajt · 2016-10-14T20:07:48Z

@arielb1 Had a similar nit but that aside diagnostics looks good.

arielb1 · 2016-10-17T19:06:07Z

Tidy is merciless

/build/src/librustc/infer/error_reporting.rs:334: line longer than 100 chars

/build/src/librustc_errors/diagnostic.rs: incorrect license

/build/src/librustc_errors/diagnostic_builder.rs: incorrect license

pnkfelix · 2016-10-18T10:20:52Z

src/test/compile-fail/issue-18937.rs

+}
+
+impl<'a> A<'a> for B {
+    fn foo<F>(&mut self, f: F) //~ ERROR parameter type `F` may not live long enough


hmm, does the error message here end up blaming the mismatch in lifetime bounds between the trait declaration and its implementation? I cannot tell from the portion of the diagnostic given here.

pnkfelix · 2016-10-18T10:23:47Z

src/librustc/traits/util.rs

@@ -1,3 +1,4 @@
+


delete this extraneous blank line

pnkfelix · 2016-10-18T10:24:47Z

src/librustc/traits/util.rs

+                // `'a: 'b`.
+
+                // Ignore `for<'a> T: 'a` -- we might in the future
+                // consider this as evidence that `Foo: 'static`, but


Do you mean "Foo" here, or "T: 'static" ?

pnkfelix · 2016-10-18T10:34:36Z

src/librustc/infer/error_reporting.rs

+                        // usually the changes we suggest would
+                        // actually have to be applied to the *trait*
+                        // method (and it's not clear that the trait
+                        // method is even under the user's control).


(and it's not clear that the trait method is even under the user's control)

can we not apply some heuristics here? For example, we could detect the scenario when the trait is defined in the same crate?

pnkfelix · 2016-10-18T10:50:21Z

src/test/compile-fail/issue-14853.rs

@@ -20,7 +20,7 @@ struct X { data: u32 }

 impl Something for X {
    fn yay<T: Str>(_:Option<X>, thing: &[T]) {
-    //~^ ERROR the requirement `T: Str` appears on the impl method
+    //~^ ERROR E0276


Are all of these scenarios covered by the ui tests?

If so, should we just delete these compile-fail tests?

And if not, then I as a test reviewer personally prefer keeping the longer messages, as it gives me a better chance of divining what the original intent was from the test author.

(but I know that @nikomatsakis and i may simply fundamentally differ in our philosophies here.)

I moved them to ui tests.

pnkfelix · 2016-10-18T10:51:41Z

These changes look fine to me; r=me assuming you address the nits pointed out by @arielb1

bors · 2016-10-19T09:55:11Z

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

arielb1 · 2016-10-20T18:08:36Z

---- Rust_Compiler_Error_Index_55 stdout ----
    error: only foreign functions are allowed to be variadic
 --> <anon>:8:18
  |
8 | fn foo(x: u8, ...) {}
  |                  ^

error: aborting due to previous error

thread 'Rust_Compiler_Error_Index_55' panicked at 'Box<Any>', src/librustc_errors/lib.rs:461
thread 'Rust_Compiler_Error_Index_55' panicked at 'Some expected error codes were not found: ["E0045"]', src/librustdoc/test.rs:293

?

nikomatsakis · 2016-10-20T19:51:58Z

@arielb1 wtf. That is very strange. Locally I also see a bunch of "parse-fail" test failures, which I assume is some kind of misconfiguration on my part.

pnkfelix · 2016-10-27T20:38:06Z

ping @nikomatsakis any idea what's going on with travis on this PR?

bors · 2016-10-29T16:01:48Z

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

nikomatsakis · 2016-10-31T18:43:06Z

@pnkfelix not sure, but I think it must have to do with some of the changes I made to adjusting the error count. I've been meaning to figure this out -- hopefully I'll find some time today.

nikomatsakis · 2016-10-31T19:16:16Z

@pnkfelix so I think I tracked down the problem -- or at least I tracked down a problem I see locally. Locally, some of the parse-fail tests are failing. We used to print this:

error: a `where` clause must have at least one predicate in it
  --> /home/nmatsakis/versioned/rust-7/src/test/parse-fail/where-clauses-no-bounds-or-predicates.rs:13:36
   |
13 | fn equal1<T>(_: &T, _: &T) -> bool where {
   |                                    ^^^^^

error: each predicate in a `where` clause must have at least one bound in it
  --> /home/nmatsakis/versioned/rust-7/src/test/parse-fail/where-clauses-no-bounds-or-predicates.rs:18:42
   |
18 | fn equal2<T>(_: &T, _: &T) -> bool where T: {
   |                                          ^^

error: aborting due to previous error

But we now print this:

error: a `where` clause must have at least one predicate in it
  --> /home/nmatsakis/versioned/rust-7/src/test/parse-fail/where-clauses-no-bounds-or-predicates.rs:13:36
   |
13 | fn equal1<T>(_: &T, _: &T) -> bool where {
   |                                    ^^^^^

error: aborting due to previous error

Note that in both cases it ends with "error: aborting due to previous error" (i.e., the singular).

I think what caused this difference is how I changed the mechanism to adjust the error count. Consider the diff for Handler::span_err():

     pub fn span_err<S: Into<MultiSpan>>(&self, sp: S, msg: &str) {
         self.emit(&sp.into(), msg, Error);
-        self.bump_err_count();
         self.panic_if_treat_err_as_bug();
     }

I changed it so that we bump the error count in the emit() method -- but only if the error hasn't been canceled in the meantime. Before, we used to bump the error (in struct_span_err) and then, when an error was cancelled, decrement the error back down. That made my accounting go awry in the new lint code, which sometimes stores errors for later. It also seemed unnecessarily convoluted. So I changed it to just increment the error count when errors are actually emitted (so long as they haven't been canceled).

I think this also fixed a bug in the existing parser, which changes the output here. In particular, the emit routine also checks a flag called continue_parse_after_error:

    pub fn emit(&self, msp: &MultiSpan, msg: &str, lvl: Level) {
        if lvl == Warning && !self.can_emit_warnings {
            return;
        }
        let mut db = DiagnosticBuilder::new(self, lvl, msg);
        db.set_span(msp.clone());
        db.emit();
        if !self.continue_after_error.get() { // (** HERE **)
            self.abort_if_errors();
        }
    }

Now, in the old code, the error count had not been bumped at this point -- it got bumped after emit() returned. So effectively we were aborting on the 2nd error, not the first one. This is why you see the misleading error message using the singular in the old output, even though two errors are displayed.

So I would argue that this is a bugfix (cc @nrc) in the case of the parse-fail tests. I'm not sure if this explains the other tests -- I'm going to patch the reference files locally and see what happens, maybe travis is running the tests in a funny order or something.

nikomatsakis · 2016-10-31T19:21:14Z

Actually I'll add -Z continue-parse-after-error, which seems to resolve the problem.

gets more comprehensive coverage in `ui`

The new handling fixed a latent bug in the parser error handling where it would only abort after the second error (when configured to stop after the first error). This is because the check for `error_count != 0` was occuring before the increment. Since the increment is tied to the `emit()` call now this no longer occurs.

The old parse code kept going even though it wasn't supposed to, leading to an E0045 ("feature not allowed on beta") printout

nikomatsakis · 2016-11-01T18:48:36Z

This now builds locally -- or did before rebase. Let's see what travis has to say.

nikomatsakis · 2016-11-01T21:21:02Z

@bors r=pnkfelix

bors · 2016-11-01T21:21:06Z

📌 Commit 4501e5a has been approved by pnkfelix

bors · 2016-11-02T18:57:30Z

⌛ Testing commit 4501e5a with merge 0717508...

bors · 2016-11-02T21:29:30Z

💔 Test failed - auto-win-msvc-64-opt

alexcrichton · 2016-11-02T22:14:10Z

@bors: retry

On Wed, Nov 2, 2016 at 2:29 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt/builds/6019

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#37167 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95JkosIh2Zniw1f3JH0wAWSQdu0Maks5q6QC9gaJpZM4KXFF2
.

bors · 2016-11-03T12:58:55Z

⌛ Testing commit 4501e5a with merge 9dfaf1e...

bors · 2016-11-03T13:11:57Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

sophiajt · 2016-11-03T13:30:00Z

@bors: retry

bors · 2016-11-03T13:30:55Z

⌛ Testing commit 4501e5a with merge 135b76e...

bors · 2016-11-03T13:37:47Z

💔 Test failed - auto-mac-64-opt-rustbuild

alexcrichton · 2016-11-03T15:50:48Z

@bors: retry

bors · 2016-11-04T14:20:45Z

⌛ Testing commit 4501e5a with merge ccfc38f...

@pnkfelix

detect extra region requirements in impls The current "compare method" check fails to check for the "region obligations" that accrue in the fulfillment context. This branch switches that code to create a `FnCtxt` so that it can invoke the regionck code. Previous crater runs (I haven't done one with the latest tip) have found some small number of affected crates, so I went ahead and introduced a warning cycle. I will kick off a crater run with this branch shortly. This is a [breaking-change] because previously unsound code was accepted. The crater runs also revealed some cases where legitimate code was no longer type-checking, so the branch contains one additional (but orthogonal) change. It improves the elaborator so that we elaborate region requirements more thoroughly. In particular, if we know that `&'a T: 'b`, we now deduce that `T: 'b` and `'a: 'b`. I invested a certain amount of effort in getting a good error message. The error message looks like this: ``` error[E0276]: impl has stricter requirements than trait --> traits-elaborate-projection-region.rs:33:5 | 21 | fn foo() where T: 'a; | --------------------- definition of `foo` from trait ... 33 | fn foo() where U: 'a { } | ^^^^^^^^^^^^^^^^^^^^^^^^ impl has extra requirement `U: 'a` | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #18937 <#18937> note: lint level defined here --> traits-elaborate-projection-region.rs:12:9 | 12 | #![deny(extra_requirement_in_impl)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ ``` Obviously the warning only prints if this is a _new_ error (that resulted from the bugfix). But all existing errors that fit this description are updated to follow the general template. In order to get the lint to preserve the span-labels and the error code, I separate out the core `Diagnostic` type (which encapsulates the error code, message, span, and children) from the `DiagnosticBuilder` (which layers on a `Handler` that can be used to report errors). I also extended `add_lint` with an alternative `add_lint_diagnostic` that takes in a full diagnostic (cc @jonathandturner for those changes). This doesn't feel ideal but feels like it's moving in the right direction =). r? @pnkfelix cc @arielb1 Fixes #18937

bors · 2016-11-04T17:40:28Z

rust-highfive assigned pnkfelix Oct 14, 2016

arielb1 reviewed Oct 14, 2016

View reviewed changes

src/librustc/lint/context.rs

}

impl EarlyLintMessage for String {

Copy link

Contributor

arielb1 Oct 14, 2016

Choose a reason for hiding this comment

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

nit: merge this and the previous commit?

arielb1 reviewed Oct 14, 2016

View reviewed changes

pnkfelix reviewed Oct 18, 2016

View reviewed changes

src/librustc/traits/util.rs

@@ -1,3 +1,4 @@

Copy link

Member

pnkfelix Oct 18, 2016

Choose a reason for hiding this comment

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

delete this extraneous blank line

pnkfelix reviewed Oct 18, 2016

View reviewed changes

nikomatsakis force-pushed the jroesch-issue-18937 branch from fb0ee77 to 39b3944 Compare October 19, 2016 23:42

nikomatsakis force-pushed the jroesch-issue-18937 branch from 39b3944 to 82ab686 Compare October 21, 2016 17:19

jroesch and others added 3 commits November 1, 2016 14:04

introduce fn-ctxt so we can invoke regionck code

f3cc374

move outlives_components onto tcx

ce340af

elaborate T: 'a dependencies

e77cc9c

nikomatsakis added 4 commits November 1, 2016 14:08

move compile-fail tests to ui tests

fa9ebfc

gets more comprehensive coverage in `ui`

pacify the mercilous tidy

ad46ad6

remove erroneous E0045 annotation

4501e5a

The old parse code kept going even though it wasn't supposed to, leading to an E0045 ("feature not allowed on beta") printout

nikomatsakis force-pushed the jroesch-issue-18937 branch from cdb7195 to 4501e5a Compare November 1, 2016 18:47

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 2, 2016

brson mentioned this pull request Nov 3, 2016

Methods in impls allowed more restrictive lifetime bounds than in the trait. #18937

Closed

bors merged commit 4501e5a into rust-lang:master Nov 4, 2016

This was referenced Nov 4, 2016

[3/n] rustc: unify and simplify managing associated items. #37402

Merged

Group unused import warnings per import list #37456

Merged

bluss mentioned this pull request Nov 26, 2016

Serialization performance regression #38021

Closed

nikomatsakis deleted the jroesch-issue-18937 branch April 14, 2017 10:13

detect extra region requirements in impls #37167

detect extra region requirements in impls #37167

Conversation

nikomatsakis commented Oct 14, 2016 • edited Loading

arielb1 Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

arielb1 Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

arielb1 commented Oct 14, 2016 • edited Loading

sophiajt commented Oct 14, 2016

arielb1 commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Oct 18, 2016

bors commented Oct 19, 2016

arielb1 commented Oct 20, 2016

nikomatsakis commented Oct 20, 2016

pnkfelix commented Oct 27, 2016

bors commented Oct 29, 2016

nikomatsakis commented Oct 31, 2016

nikomatsakis commented Oct 31, 2016

nikomatsakis commented Oct 31, 2016

nikomatsakis commented Nov 1, 2016

nikomatsakis commented Nov 1, 2016

bors commented Nov 1, 2016

bors commented Nov 2, 2016

bors commented Nov 2, 2016

alexcrichton commented Nov 2, 2016

bors commented Nov 3, 2016

bors commented Nov 3, 2016

sophiajt commented Nov 3, 2016

bors commented Nov 3, 2016

bors commented Nov 3, 2016

alexcrichton commented Nov 3, 2016

bors commented Nov 4, 2016

bors commented Nov 4, 2016

nikomatsakis commented Oct 14, 2016 •

edited

Loading

arielb1 Oct 14, 2016 •

edited

Loading

arielb1 Oct 14, 2016 •

edited

Loading

arielb1 Oct 14, 2016 •

edited

Loading

arielb1 commented Oct 14, 2016 •

edited

Loading