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

Update E0393 to new error format #36114

Merged
merged 1 commit into from
Sep 1, 2016
Merged

Update E0393 to new error format #36114

merged 1 commit into from
Sep 1, 2016

Conversation

zjhmale
Copy link
Contributor

@zjhmale zjhmale commented Aug 29, 2016

Fixes #35632.
Part of #35233.

r? @jonathandturner

and a wired thing is that if i add another label

.span_label(span, &format!("missing reference to `{}`", def.name))
.span_label(span, &format!("because of the default `Self` reference, type parameters must be specified on object types"))

and add a new note in the test case like

trait A<T=Self> {}

fn together_we_will_rule_the_galaxy(son: &A) {}
//~^ ERROR E0393
//~| NOTE missing reference to `T`
//~| NOTE because of the default `Self` reference, type parameters must be specified on object types

it will complain that

running 1 test
test [compile-fail] compile-fail/E0393.rs ... FAILED

failures:

---- [compile-fail] compile-fail/E0393.rs stdout ----

error: /Users/zjh/Documents/rustspace/rust/src/test/compile-fail/E0393.rs:13: unexpected "error": '13:43: 13:44: the type parameter `T` must be explicitly specified [E0393]'

unexpected errors (from JSON output): [
    Error {
        line_num: 13,
        kind: Some(
            Error
        ),
        msg: "13:43: 13:44: the type parameter `T` must be explicitly specified [E0393]"
    }
]

it is a little bit confusing and through the blog post we can use //~^ and //~| to support multiple notes, @jonathandturner am i missing something here?

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonathandturner (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sophiajt
Copy link
Contributor

sophiajt commented Aug 29, 2016

@zjhmale - thanks for the PR. Looks like part of the problem is that you're using .span_label for everything. In the issue, there are a few parts that need to be updated: "E0393 needs a span_label, updated title, and a note, changing it from:"

The span_label, you figured out. The title is the error message title, which you also got. The note is the .note() feature. So instead of:

struct_span_err!(tcx.sess, span, E0393,
                 "the type parameter `{}` must be explicitly specified",
                 def.name)
    .span_label(span, &format!("missing reference to `{}`", def.name))
    .emit();

You can do:

let mut err = struct_span_err!(tcx.sess, span, E0393,
                               "the type parameter `{}` must be explicitly specified",
                               def.name);
err.span_label(span, &format!("missing reference to `{}`", def.name));
err.note(&format!("because of the default `Self` reference, type parameters must be specified on object types"));
err.emit();

@zjhmale
Copy link
Contributor Author

zjhmale commented Aug 30, 2016

@jonathandturner awesome! thx for pointing out note function! and i believe this is freeze now, please review.

@sophiajt
Copy link
Contributor

Great, thanks for updating. We're getting closer. The next step is to pass the tidy checks.

/build/src/librustc_typeck/astconv.rs:522: line longer than 100 chars

You can run these yourself using this command:

python src/bootstrap/bootstrap.py --stage 1 --step check-tidy

@zjhmale
Copy link
Contributor Author

zjhmale commented Aug 30, 2016

@jonathandturner tidied up the code.

@sophiajt
Copy link
Contributor

Great!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 30, 2016

📌 Commit 25c9097 has been approved by jonathandturner

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 30, 2016
Update E0393 to new error format

Fixes rust-lang#35632.
Part of rust-lang#35233.

r? @jonathandturner

and a wired thing is that if i add another label

```rust
.span_label(span, &format!("missing reference to `{}`", def.name))
.span_label(span, &format!("because of the default `Self` reference, type parameters must be specified on object types"))
```

and add a new note in the test case like

```rust
trait A<T=Self> {}

fn together_we_will_rule_the_galaxy(son: &A) {}
//~^ ERROR E0393
//~| NOTE missing reference to `T`
//~| NOTE because of the default `Self` reference, type parameters must be specified on object types
```

it will complain that

```
running 1 test
test [compile-fail] compile-fail/E0393.rs ... FAILED

failures:

---- [compile-fail] compile-fail/E0393.rs stdout ----

error: /Users/zjh/Documents/rustspace/rust/src/test/compile-fail/E0393.rs:13: unexpected "error": '13:43: 13:44: the type parameter `T` must be explicitly specified [E0393]'

unexpected errors (from JSON output): [
    Error {
        line_num: 13,
        kind: Some(
            Error
        ),
        msg: "13:43: 13:44: the type parameter `T` must be explicitly specified [E0393]"
    }
]
```

it is a little bit confusing and through the blog post we can use `//~^` and `//~|` to support multiple notes, @jonathandturner am i missing something here?
@sophiajt
Copy link
Contributor

@bors r-

Looks like there are a few more tests you'll need to update:

  [compile-fail] compile-fail/issue-21950.rs
  [compile-fail] compile-fail/issue-22370.rs
  [compile-fail] compile-fail/issue-22560.rs

When you make the changes, make sure to run the check (there are some tips in the blog post)

python src/bootstrap/bootstrap.py --step check-cfail --stage 1

@zjhmale
Copy link
Contributor Author

zjhmale commented Aug 31, 2016

@jonathandturner fixed these tests! and sorry that i just used python src/bootstrap/bootstrap.py --step check-cfail <name> --stage 1 to check specific test case 😅

@sophiajt
Copy link
Contributor

Hehe, no worries. I do the same thing sometimes.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 31, 2016

📌 Commit 189dee6 has been approved by jonathandturner

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 31, 2016
Update E0393 to new error format

Fixes rust-lang#35632.
Part of rust-lang#35233.

r? @jonathandturner

and a wired thing is that if i add another label

```rust
.span_label(span, &format!("missing reference to `{}`", def.name))
.span_label(span, &format!("because of the default `Self` reference, type parameters must be specified on object types"))
```

and add a new note in the test case like

```rust
trait A<T=Self> {}

fn together_we_will_rule_the_galaxy(son: &A) {}
//~^ ERROR E0393
//~| NOTE missing reference to `T`
//~| NOTE because of the default `Self` reference, type parameters must be specified on object types
```

it will complain that

```
running 1 test
test [compile-fail] compile-fail/E0393.rs ... FAILED

failures:

---- [compile-fail] compile-fail/E0393.rs stdout ----

error: /Users/zjh/Documents/rustspace/rust/src/test/compile-fail/E0393.rs:13: unexpected "error": '13:43: 13:44: the type parameter `T` must be explicitly specified [E0393]'

unexpected errors (from JSON output): [
    Error {
        line_num: 13,
        kind: Some(
            Error
        ),
        msg: "13:43: 13:44: the type parameter `T` must be explicitly specified [E0393]"
    }
]
```

it is a little bit confusing and through the blog post we can use `//~^` and `//~|` to support multiple notes, @jonathandturner am i missing something here?
bors added a commit that referenced this pull request Sep 1, 2016
@bors bors merged commit 189dee6 into rust-lang:master Sep 1, 2016
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.

4 participants