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

Improve diagnostics for E0282 #24966

Merged
merged 4 commits into from May 7, 2015
Merged

Improve diagnostics for E0282 #24966

merged 4 commits into from May 7, 2015

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda commented Apr 29, 2015

The error message was misleading, so I adjusted it, and I also added the long diagnostics for this error (resolves one point in #24407).

I was unsure about how to phrase the error message. Is “generic parameter binding” the correct term for this?

The error can also occur in cases where a type annotation will not help.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

`VecDeque` among others. Consider the following snippet:

```
let x = (1_i32 .. 10).collect();
Copy link
Member

Choose a reason for hiding this comment

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

I think throughout these examples it should be safe to leave off the _i32 annotation, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but then the type of x: Vec<_> depends on the integer literal default, which is a feature interacting here that is not really related to the error. Maybe we could use an other type like char to avoid defaults altogether, but is there a good and simple way to get an iterator there that is not silly? (E.g. let v = vec!('a', 'b', 'c'); v.iter().collect::<Vec<_>>() seems like a silly example to me.)

Copy link
Member

Choose a reason for hiding this comment

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

If you're still inclined to use something like char, there is the option of starting with a string and iterating over its chars, like this:

    const HELLO: &'static str = "hello";
    let v: Vec<_> = HELLO.chars().rev().collect();
    println!("v: {:?}", v);

Copy link
Member

Choose a reason for hiding this comment

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

and if you did that, you could avoid talking about VecDeque; you could just say that Vec<char> and String are suitable candidates:

    const HELLO: &'static str = "hello";
    let s: String = HELLO.chars().skip(1).collect();
    println!("Bert says, {:?}", s);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pnkfelix Yes, that is much better!

@alexcrichton
Copy link
Member

Awesome, thanks @ruud-v-a!

The new example uses a `char` iterator instead of `i32`, to avoid interplay
between type inference and the default type for integer literals.
@nikomatsakis
Copy link
Contributor

@bors rollup r+

@bors
Copy link
Contributor

bors commented May 5, 2015

📌 Commit 414dfb1 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

This looks nice.

@bors
Copy link
Contributor

bors commented May 5, 2015

⌛ Testing commit 414dfb1 with merge a65d3f9...

@bors
Copy link
Contributor

bors commented May 5, 2015

💔 Test failed - auto-mac-64-nopt-t

@@ -290,7 +290,7 @@ pub fn maybe_report_ambiguity<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
{
span_err!(infcx.tcx.sess, obligation.cause.span, E0282,
"unable to infer enough type information about `{}`; \
type annotations required",
type annotations or generic parameter binding required",
Copy link
Member

Choose a reason for hiding this comment

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

@ruud-v-a with a change like this, you need to update the test suite to reflect the change to the expected error message.

I highly recommend you do a local make check-stage1-cfail to catch these, and maybe make check-stage1-cfail-full as well ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads-up. I did run make check (on Windows), isn’t it supposed to run all tests? Anyhow, I updated the compile-fail tests and I am running the tests (on Linux) at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

maybe I misinterpreted the log files, but it certainly seemed like the compile-fail tests did need updating. If you did not see failures on a windows make check, then that is quite troubling...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pnkfelix It were definitely those tests that failed. To be sure, I ran make check on 414dfb1 on my Windows box again. There were indeed failures, I missed them the first time. I did see ... FAILED scroll by this time, but the check does not stop on failure. The last few lines of output are these:

test [run-pass] run-pass/xcrate-unit-struct.rs ... ok
test [run-pass] run-pass/zero-size-type-destructors.rs ... ok
test [run-pass] run-pass/zero_sized_subslice_match.rs ... ok

test result: ok. 2034 passed; 0 failed; 33 ignored; 0 measured

make: *** wait: No child processes.  Stop.

Last time I only checked the last line, and I assumed 0 failed meant everything was good. The summary of compile-fail turned out to be hidden 2000 lines earlier between the rest of the test output.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, okay, that is a little bit more comforting, but still a little worrisome. Maybe this is something going wrong with our return codes on Windows, causing the make to miss the fact that the run errored?

@pnkfelix
Copy link
Member

pnkfelix commented May 5, 2015

@ruud-v-a now make tidy is failing. :)

@ruuda
Copy link
Contributor Author

ruuda commented May 5, 2015

Now make tidy is happy again, but make check-stage1-cfail fails for the files that I linewrapped. What am I doing wrong? Edit: It is a different set of files that fails, I edited the wrong files. Edit 2: The rebase below should pass.

@pnkfelix
Copy link
Member

pnkfelix commented May 6, 2015

@bors r+ 4b8098b rollup

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 7, 2015
The error message was misleading, so I adjusted it, and I also added the long diagnostics for this error (resolves one point in rust-lang#24407).

I was unsure about how to phrase the error message. Is “generic parameter binding” the correct term for this?
steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 7, 2015
The error message was misleading, so I adjusted it, and I also added the long diagnostics for this error (resolves one point in rust-lang#24407).

I was unsure about how to phrase the error message. Is “generic parameter binding” the correct term for this?
bors added a commit that referenced this pull request May 7, 2015
@bors bors merged commit 4b8098b into rust-lang:master May 7, 2015
@ruuda ruuda deleted the explain branch November 30, 2016 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants