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

Use track_errors instead of hand rolling #59358

Merged
merged 7 commits into from
Mar 29, 2019

Conversation

JohnTitor
Copy link
Member

Fixes #59215

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2019
@@ -647,10 +647,14 @@ pub fn const_eval_raw_provider<'a, 'tcx>(
"could not evaluate static initializer");
// Ensure that if the above error was either `TooGeneric` or `Reported`
// an error must be reported.
if tcx.sess.err_count() == 0 {
tcx.sess.delay_span_bug(err.span,
let errs = tcx.sess.track_errors(|| {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should put the err.report_as_error(...) call above into the closure. We don't need the err_count call anymore, it's done by track_errors.

The idea was to ensure that calling report_as_error actually reports any errors.

You should then be able to yield the values inside the Err and Ok variants instead of having to carry the reported_err variable around

Copy link
Member Author

Choose a reason for hiding this comment

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

I get the point, removed err_count.

if tcx.sess.err_count() == 0 {
tcx.sess.delay_span_bug(err.span,
let tracked_err = tcx.sess.track_errors(|| {
err.report_as_error(ecx.tcx,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove the report_as_error call above. I'm not sure what types tracked_err has, but the Ok path should give you the same value that you got previously in reported_err. What's the value in the Err part?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please let me ask you two questions.

  1. What should I place instead of report_as_error?
  2. How I check if track_err returns Ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

for 2. you are already checking that via the match below. You can pattern match on the Ok value via Ok(reported_err).

for 1. nothing, you already are calling it inside of the track_errors call, so you are replacing it with the tcx.sess.track_errors(|| { err.report_as_error ....

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, thank you! And what should I do in the Err part?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, that depends on what is returned there. Have a look at the return type and emit an apropriate ErrorHandled enum variant

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed WIP commit. Test will be failed because types are incompatible in match.
But I want to know if my work is correct so far. Could you check this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is correct so far.

In the Ok arm, the return value should be v I believe. The Err pattern can probably be Err(ErrorReported) and the arm value ErrorHandled::Reported (or whatever the correct variant is).

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified as follows:

match reported_err {
    Ok(v) => {
        tcx.sess.delay_span_bug(err.span,
                            &format!("static eval failure did not emit an error: {:#?}",
                            v));
        v
    },
    Err(ErrorReported) => ErrorReported,
}

but () is returned in the Ok arm and rustc::util::common::ErrorReported is returned in the Err arm, so the compile is failed. Any solutions?

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.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.
travis_time:end:0326c964:start=1553406409795269447,finish=1553406484147189549,duration=74351920102
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:13:33]    Compiling rustc_allocator v0.0.0 (/checkout/src/librustc_allocator)
[00:13:49] error[E0308]: match arms have incompatible types
[00:13:49]    --> src/librustc_mir/const_eval.rs:656:29
[00:13:49]     |
[00:13:49] 652 |  /             match reported_err {
[00:13:49] 653 |  |                 Ok(v) => tcx.sess.delay_span_bug(err.span,
[00:13:49]     |  |__________________________-
[00:13:49] 654 | ||                                         &format!("static eval failure did not emit an error: {:#?}",
[00:13:49] 655 | ||                                         v)),
[00:13:49]     | ||___________________________________________- this is found to be of type `()`
[00:13:49] 656 |  |                 Err(err) => err,
[00:13:49]     |  |                             ^^^ expected (), found struct `rustc::util::common::ErrorReported`
[00:13:49] 657 |  |             }
[00:13:49]     |  |_____________- `match` arms have incompatible types
[00:13:49]     = note: expected type `()`
[00:13:49]                found type `rustc::util::common::ErrorReported`
[00:13:49] 
[00:13:50] error: aborting due to previous error
---
travis_time:end:018e2810:start=1553407546471130127,finish=1553407546476492478,duration=5362351
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:15b45a97
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2a1c4b18
travis_time:start:2a1c4b18
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No s

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)

tcx.sess.delay_span_bug(err.span,
let reported_err = tcx.sess.track_errors(|| {
err.report_as_error(ecx.tcx,
"could not evaluate static initializer");
Copy link
Contributor

Choose a reason for hiding this comment

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

The ; here drops the value you'd get for v and makes it ().

Removing the ; here should fix your problem

Copy link
Member Author

@JohnTitor JohnTitor Mar 24, 2019

Choose a reason for hiding this comment

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

Thank you! I dropped ; but the compile was failed. However, the error message was changed.

error[E0308]: mismatched types==================================>    ] 128/137: rustc_mir
   --> src/librustc_mir/const_eval.rs:657:21
    |
657 |                     v
    |                     ^ expected (), found enum `rustc::mir::interpret::ErrorHandled`
    |
    = note: expected type `()`
               found type `rustc::mir::interpret::ErrorHandled`

related code is here:

if tcx.is_static(def_id).is_some() {
    // Ensure that if the above error was either `TooGeneric` or `Reported`
    // an error must be reported.
    let reported_err = tcx.sess.track_errors(|| {
        err.report_as_error(ecx.tcx,
                            "could not evaluate static initializer")
    });
    match reported_err {
        Ok(v) => {
            tcx.sess.delay_span_bug(err.span,
                                &format!("static eval failure did not emit an error: {:#?}",
                                v));
            v
        },
        Err(ErrorReported) => Err(ErrorHandled::Reported),
    }
    reported_err
}

I tried to compile by changing value in the Ok or Err arm, but still failed, hmm... When () are returned in the Ok and Err, another error was occured.

Copy link
Contributor

Choose a reason for hiding this comment

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

The match isn't returning the values written in it, because you have a trailing expression (reported_err) afterwards. Removing that will make the match the trailing expression

Copy link
Member Author

Choose a reason for hiding this comment

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

I got it, modified and pushed.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.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.
travis_time:end:01dbc2c0:start=1553451669458924952,finish=1553451750264122637,duration=80805197685
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:15:28]    Compiling rustc_lint v0.0.0 (/checkout/src/librustc_lint)
[00:15:45] error: unused variable: `ErrorReported`
[00:15:45]    --> src/librustc_mir/const_eval.rs:657:21
[00:15:45]     |
[00:15:45] 657 |                 Err(ErrorReported) => ErrorHandled::Reported,
[00:15:45]     |                     ^^^^^^^^^^^^^ help: consider prefixing with an underscore: `_ErrorReported`
[00:15:45]     = note: `-D unused-variables` implied by `-D warnings`
[00:15:45] 
[00:15:45] error: aborting due to previous error
[00:15:45] 
---
travis_time:end:252e2d4c:start=1553452930727091633,finish=1553452930734893005,duration=7801372
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:08f99725
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:083ae290
travis_time:start:083ae290
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:02f51bec
$ dmesg | grep -i kill

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)

@JohnTitor
Copy link
Member Author

@oli-obk Is there anything I can do?

@tesuji
Copy link
Contributor

tesuji commented Mar 28, 2019

@JohnTitor You might need to rebase on master. Now we have 384 commits in this PR.

@JohnTitor
Copy link
Member Author

@lzutao yes, I'll fix.

@JohnTitor
Copy link
Member Author

JohnTitor commented Mar 28, 2019

@oli-obk Uh, if you allow me, I'd like to delete this branch and re-open PR, sorry...

EDIT: I cleaned up commit log successfully, I'm sorry for confusing you.

@JohnTitor JohnTitor force-pushed the use-track-errors branch 2 times, most recently from 4dc0279 to 169c320 Compare March 28, 2019 19:26
@JohnTitor JohnTitor force-pushed the use-track-errors branch 2 times, most recently from 76d457a to 6c8e3a5 Compare March 28, 2019 19:41
@oli-obk
Copy link
Contributor

oli-obk commented Mar 28, 2019

@bors r+

Thanks for doing this and seeing this through all the changes!

@bors
Copy link
Contributor

bors commented Mar 28, 2019

📌 Commit 0778847 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 29, 2019
bors added a commit that referenced this pull request Mar 29, 2019
Rollup of 11 pull requests

Successful merges:

 - #58019 (Combine all builtin late lints and make lint checking parallel)
 - #59358 (Use `track_errors` instead of hand rolling)
 - #59394 (warn -> deny duplicate match bindings)
 - #59401 (bootstrap: build crates under libtest with -Z emit-stack-sizes)
 - #59423 (Visit path in `walk_mac`)
 - #59468 (musl: build toolchain libs with -fPIC)
 - #59476 (Use `SmallVec` in `TokenStreamBuilder`.)
 - #59496 (Remove unnecessary with_globals calls)
 - #59498 (Use 'write_all' instead of 'write' in example code)
 - #59503 (Stablize {f32,f64}::copysign().)
 - #59511 (Fix missed fn rename in #59284)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Mar 29, 2019
Rollup of 11 pull requests

Successful merges:

 - #58019 (Combine all builtin late lints and make lint checking parallel)
 - #59358 (Use `track_errors` instead of hand rolling)
 - #59394 (warn -> deny duplicate match bindings)
 - #59401 (bootstrap: build crates under libtest with -Z emit-stack-sizes)
 - #59423 (Visit path in `walk_mac`)
 - #59468 (musl: build toolchain libs with -fPIC)
 - #59476 (Use `SmallVec` in `TokenStreamBuilder`.)
 - #59496 (Remove unnecessary with_globals calls)
 - #59498 (Use 'write_all' instead of 'write' in example code)
 - #59503 (Stablize {f32,f64}::copysign().)
 - #59511 (Fix missed fn rename in #59284)

Failed merges:

r? @ghost
@bors bors merged commit 0778847 into rust-lang:master Mar 29, 2019
@JohnTitor JohnTitor deleted the use-track-errors branch March 29, 2019 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants