More general `on_unimplemented`, with uses in `Try` #44191

arielb1 · 2017-08-30T21:14:39Z

Allow on_unimplemented directives to specify both the label and the primary message of the trait error, and allow them to be controlled by flags - currently only to be desugaring-sensitive.

e.g.

#[rustc_on_unimplemented(
    on(all(direct, from_desugaring="?"),
        message="the `?` operator can only be used in a \
        function that returns `Result` \
        (or another type that implements `{Try}`)",
        label="cannot use the `?` operator in a function that returns `{Self}`"),
)]

r? @nikomatsakis

arielb1 · 2017-08-30T21:16:36Z

cc @estebank @rust-lang/docs are there any more things you want in on_unimplemented?

estebank · 2017-08-30T22:09:11Z

src/librustc/traits/on_unimplemented.rs

+                    Position::ArgumentIs(_) => {
+                        span_err!(tcx.sess, span, E0231,
+                                  "only named substitution \
+                                   parameters are allowed");


Add a note including all the possible valid named parameters?

That's preexisting.

estebank · 2017-08-30T22:09:55Z

src/librustc_typeck/check/mod.rs

+    let item_def_id = tcx.hir.local_def_id(item.id);
+    // an error would be reported if this fails.
+    let _ = traits::OnUnimplementedDirective::of_item(
+        tcx, trait_def_id, item_def_id);


Doesn't fit in <100?

it does. this just went through a few revisions.

estebank · 2017-08-30T22:10:27Z

src/librustc_typeck/diagnostics.rs

@@ -4690,7 +4646,6 @@ register_diagnostics! {
    E0224, // at least one non-builtin train is required for an object type
    E0227, // ambiguous lifetime bound, explicit lifetime bound required
    E0228, // explicit lifetime bound required
-    E0231, // only named substitution parameters are allowed


Don't these need to be commented out instead of deleted? I don't know.

Moved to librustc/diagnostics, not deleted.

estebank · 2017-08-30T22:18:37Z

src/test/ui/suggestions/try-operator-on-main.stderr

   |     ---------------------------
   |     |
-   |     the `?` operator can only be used in a function that returns `Result` (or another type that implements `std::ops::Try`)
+   |     the trait `std::ops::Try` is not implemented for `()`


I am worried that the error text is less prominent than the label, so moving the more relevant text to the error message makes it less useful for newbies. But this might just be an unfounded personal reaction.

Maybe adding an optional label attribute to rustc_on_unimplemented for text to be displayed in the label would be reasonable? (If I understood the code above correctly, the feature is already there, it just needs to be changed in the trait, right?)

estebank · 2017-08-30T22:19:58Z

src/test/ui/on-unimplemented/bad-annotation.stderr

+  --> $DIR/bad-annotation.rs:46:39
+   |
+46 | #[rustc_on_unimplemented(message="x", message="y")]
+   |                                       ^^^^^^^^^^^ expected value here


Should we handle duplicated values with a special label pointing at the first one encountered?

I didn't bother with precise error reporting here - this is an unstable feature that is going to be revised.

arielb1 · 2017-08-31T10:06:44Z

addressed

nikomatsakis · 2017-08-31T12:48:57Z

Ah, this is a neat idea! :) I like the idea of improving on_unimplemented further. (As an irrelevant aside, remind me to try and write-up some of my thoughts about end-user-customizable error reporting, which would supercede this sort of thing I think.)

Anyway, this seems good, but to truly handle the ? errors correctly, though, we would need to differentiate the two uses of Try that occur within the ? operator, as described in this comment. In particular, we want to distinguish between foo? where the type of foo does not implement Try and the error you are testing here, which arises from the Try::from_err() call.

Put another way, can you add the test case from #43984?

arielb1 · 2017-08-31T13:39:38Z

The test case from #43984 is present, it's the other one that isn't

So the problem is basically to distinguish:

fn main() {
    Ok(4)?; //~ ERROR the `?` operator can only be used in a function that returns `Result` (or another type that implements `std::ops::Try`)
}

fn foo() -> Result<(), ()> {
    ()?; //~ ERROR <want something else>
    Ok(())
}

arielb1 · 2017-08-31T13:39:47Z

I think I can arrange that, but I need to go AFK.

nikomatsakis · 2017-08-31T13:50:43Z

@arielb1 right, that is the two cases to distinguish. The test case I was referring to included:

fn source() -> u32 {
    3u32?
}

with the expected error message:

error[E0277]: the trait bound `u32: std::ops::Try` is not satisfied
  --> $DIR/try-unimplemented.rs:17:5
   |
17 |     3u32?
   |     ^^^^^ `?` cannot be applied to a value of type `u32`
   |
   = help: the trait `std::ops::Try` is not implemented for `u32`
   = note: required by `std::ops::Try::into_result`

I think it's a bit debatable what the "main part" of that error should be, but anyway.

nikomatsakis · 2017-08-31T13:51:07Z

@arielb1 we could also do it in a follow-up, that'd be ok w/ me (and/or maybe @huntiep wants to hack on it, I don't know)

arielb1 · 2017-08-31T18:48:13Z

Special Try case now done! We can iterate more on other special cases, but it should be fairly easy.

estebank · 2017-08-31T19:06:33Z

src/test/ui/suggestions/try-operator-on-main.rs

    std::fs::File::open("foo")?;

+    // a non-`Try` type on a `Try` fn


a non-`Try` type on a non-`Try` fn

estebank · 2017-08-31T19:11:04Z

src/test/ui/suggestions/try-operator-on-main.stderr

   |     ^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Try` is not implemented for `()`
   |
   = note: required by `try_trait_generic`

-error: aborting due to 2 previous errors
+error[E0277]: the trait bound `(): std::ops::Try` is not satisfied


It would be neat to also add a custom error message for this case along the lines of

the `?` operator can only be applied to elements that implement the trait `std::ops::Try`

I think now that we have this feature one of the docs guys should use it (or variants of it) in all the traits.

estebank · 2017-08-31T19:11:29Z

src/librustc/traits/error_reporting.rs

            flags.push(("from_desugaring", None));
-            flags.push(("from_desugaring", Some(&*s)));
+            flags.push(("from_desugaring", Some(&*desugaring)));


Use .as_ref?

I personally use &*x instead of .as_ref().

nikomatsakis · 2017-08-31T19:04:49Z

src/libcore/ops/try.rs

@@ -21,7 +21,9 @@
                                     (or another type that implements `{Try}`)")]
 #[cfg_attr(not(stage0),
           rustc_on_unimplemented(
-               on(all(direct, from_desugaring="?"),
+               on(all(
+                   any(from_method="from_error", from_method="from_ok"),


nikomatsakis · 2017-08-31T20:13:28Z

src/test/ui/suggestions/try-operator-on-main.stderr

   |     in this macro invocation
   |
   = help: the trait `std::ops::Try` is not implemented for `()`
   = note: required by `std::ops::Try::from_error`

-error: aborting due to previous error
+error[E0277]: the `?` operator can only be applied to values that implement `std::ops::Try`


nikomatsakis · 2017-08-31T20:13:42Z

@bors r+

bors · 2017-08-31T20:13:43Z

📌 Commit 0738768 has been approved by nikomatsakis

estebank · 2017-09-01T18:12:35Z

There's a bizarre error when running Doctest: bootstrap.RustBuild.program_config:

[00:02:13] ======================================================================
[00:02:13] FAIL: program_config (bootstrap.RustBuild)
[00:02:13] Doctest: bootstrap.RustBuild.program_config
[00:02:13] ----------------------------------------------------------------------
[00:02:13] Traceback (most recent call last):
[00:02:13]   File "/usr/lib/python2.7/doctest.py", line 2226, in runTest
[00:02:13]     raise self.failureException(self.format_failure(new.getvalue()))
[00:02:13] AssertionError: Failed doctest test for bootstrap.RustBuild.program_config
[00:02:13]   File "/checkout/src/bootstrap/bootstrap.py", line 519, in program_config
[00:02:13] 
[00:02:13] ----------------------------------------------------------------------
[00:02:13] File "/checkout/src/bootstrap/bootstrap.py", line 527, in bootstrap.RustBuild.program_config
[00:02:13] Failed example:
[00:02:13]     cargo_path.rstrip(".exe") == os.path.join("/tmp/rust",
[00:02:13]     "bin", "cargo")
[00:02:13] Expected:
[00:02:13]     True
[00:02:13] Got:
[00:02:13]     False

frewsxcv · 2017-09-02T14:43:03Z

@estebank I'm pretty sure that error (which isn't actually a hard error) happens on all builds, regardless of whether the build is successful or not.

The actual error is in the Building stage1 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) section:

[00:18:17] error: variable does not need to be mutable
[00:18:17]    --> /checkout/src/libsyntax/attr.rs:598:65
[00:18:17]     |
[00:18:17] 598 | pub fn eval_condition<F>(cfg: &ast::MetaItem, sess: &ParseSess, mut eval: &mut F)
[00:18:17]     |                                                                 ^^^^^^^^
[00:18:17]     |
[00:18:17] note: lint level defined here
[00:18:17]    --> /checkout/src/libsyntax/lib.rs:21:9
[00:18:17]     |
[00:18:17] 21  | #![deny(warnings)]
[00:18:17]     |         ^^^^^^^^
[00:18:17]     = note: #[deny(unused_mut)] implied by #[deny(warnings)]
[00:18:17] 
[00:18:17] error: aborting due to previous error
[00:18:17] 
[00:18:17] error: Could not compile `syntax`.

frewsxcv · 2017-09-02T14:43:15Z

@bors r-

@huntiep

Maybe I should allow error messages to check the *specific* desugaring? Thanks @huntiep for the idea!

arielb1 · 2017-09-03T15:22:48Z

@bors r+

bors · 2017-09-03T15:22:49Z

📌 Commit 291b4ed has been approved by arielb1

arielb1 · 2017-09-03T16:04:50Z

@bors r-

arielb1 · 2017-09-03T16:04:55Z

@bors r=nikomatsakis

bors · 2017-09-03T16:04:56Z

📌 Commit 291b4ed has been approved by nikomatsakis

bors · 2017-09-03T16:25:52Z

⌛ Testing commit 291b4ed with merge c8642da...

@nikomatsakis

More general `on_unimplemented`, with uses in `Try` Allow `on_unimplemented` directives to specify both the label and the primary message of the trait error, and allow them to be controlled by flags - currently only to be desugaring-sensitive. e.g. ```Rust #[rustc_on_unimplemented( on(all(direct, from_desugaring="?"), message="the `?` operator can only be used in a \ function that returns `Result` \ (or another type that implements `{Try}`)", label="cannot use the `?` operator in a function that returns `{Self}`"), )] ``` r? @nikomatsakis

bors · 2017-09-03T18:57:18Z

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

rust-highfive assigned nikomatsakis Aug 30, 2017

arielb1 mentioned this pull request Aug 30, 2017

Improve Try error messages #43984

Closed

estebank reviewed Aug 30, 2017

View reviewed changes

estebank reviewed Aug 31, 2017

View reviewed changes

arielb1 force-pushed the on-unimplemented-label branch 2 times, most recently from 264bc78 to 0738768 Compare August 31, 2017 19:53

nikomatsakis reviewed Aug 31, 2017

View reviewed changes

nikomatsakis approved these changes Aug 31, 2017

View reviewed changes

shepmaster added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 1, 2017

arielb1 force-pushed the on-unimplemented-label branch from 0738768 to 9826f2d Compare September 3, 2017 09:44

arielb1 added 6 commits September 3, 2017 13:10

refactor and centralize on_unimplemented parsing

243aa12

move the on_unimplemented logic to its own file

cf07ebd

implement improved on_unimplemented directives

6866aea

enable desugaring-sensitive error messages and use them in Try

02b3ae6

Maybe I should allow error messages to check the *specific* desugaring? Thanks @huntiep for the idea!

address review comments

6dec953

on_unimplemented: add method-name checks and use them in Try

efa09ea

add error message for the other case too

291b4ed

arielb1 force-pushed the on-unimplemented-label branch from 9826f2d to 291b4ed Compare September 3, 2017 10:11

bors merged commit 291b4ed into rust-lang:master Sep 3, 2017

		std::fs::File::open("foo")?;

		// a non-`Try` type on a `Try` fn

More general on_unimplemented, with uses in Try #44191

More general on_unimplemented, with uses in Try #44191

Conversation

arielb1 commented Aug 30, 2017 • edited

arielb1 commented Aug 30, 2017 • edited

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 commented Aug 31, 2017

nikomatsakis commented Aug 31, 2017

arielb1 commented Aug 31, 2017

arielb1 commented Aug 31, 2017 • edited

nikomatsakis commented Aug 31, 2017

nikomatsakis commented Aug 31, 2017

arielb1 commented Aug 31, 2017

estebank Aug 31, 2017 • edited

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

nikomatsakis commented Aug 31, 2017

bors commented Aug 31, 2017

estebank commented Sep 1, 2017

frewsxcv commented Sep 2, 2017

frewsxcv commented Sep 2, 2017

arielb1 commented Sep 3, 2017

bors commented Sep 3, 2017

arielb1 commented Sep 3, 2017

arielb1 commented Sep 3, 2017

bors commented Sep 3, 2017

bors commented Sep 3, 2017

bors commented Sep 3, 2017

More general `on_unimplemented`, with uses in `Try` #44191

More general `on_unimplemented`, with uses in `Try` #44191

arielb1 commented Aug 30, 2017 •

edited

arielb1 commented Aug 30, 2017 •

edited

arielb1 commented Aug 31, 2017 •

edited

estebank Aug 31, 2017 •

edited