[WIP] Warnings on long running constant evaluations #50859

csmoe · 2018-05-18T12:19:48Z

oli-obk

Would you be so kind as to also add a bunch of tests that show various use cases of the lint with different lint levels and across const fn calls with different lint levels?

oli-obk · 2018-05-18T12:28:34Z

src/librustc_mir/interpret/step.rs

+                    .next()
+                    .expect("some part of a failing const eval must be local");
+
+            self.tcx.struct_span_lint_node(


So this alone won't cause const eval to abort. You should be able to run

tcx.lint_level_at_node(lint, id).0 == Level::Deny

to figure out whether the lint is denied.

This means you need to do that check for all stack frames (and not just the first local one), then report the lint at the most extreme level (forbid/deny) and if it is either forbid or deny, return a constant evaluation error here.

if all lint levels are allow or warn, continue the evaluation.

N.B. There exists more than a single error lint level (e.g. Level::Fatal iirc).

oli-obk · 2018-05-18T12:30:33Z

src/librustc_mir/interpret/step.rs

+                    .stack()
+                    .iter()
+                    .rev()
+                    .filter_map(|frame| self.tcx.hir.as_local_node_id(frame.instance.def_id()))


This should also filter out all frames that are Allow, because the lint won't get reported there anyway, but if some frame wants to report the lint, then it should.

This means there might be no frames that we report anything on. In that case just don't emit the lint and only reset the terminators_remaining counter.

@oli-obk thank you, the suggestions will be addressed soon.

oli-obk · 2018-05-18T12:48:22Z

Actually, that travis failure makes me think... Maybe we should be using the report method here, so the lint prints out the entire stack trace. This should make the user experience related to debugging such cases much better.

This means that the changes described above should be done inside the report method, and it should also get a new argument: the lint ID (so we can distinguish between const_err and this new lint).

TimNN · 2018-05-22T18:29:42Z

@csmoe: It looks like your recent push didn't include any changes except the lint declaration, was that intentional?

csmoe · 2018-05-24T13:58:56Z

@TimNN That isn't all, I'm working on this, once it's done, I'd ping for review.

TimNN · 2018-05-24T14:08:49Z

@csmoe No problem, take your time! I was just surprised since the PR history indicates that earlier versions already contained code changes.

shepmaster · 2018-06-02T14:21:00Z

Ping from triage, @csmoe ! We haven't heard from you in a while; are you still working on this?

bors · 2018-06-06T14:11:06Z

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

pietroalbini · 2018-06-18T08:59:47Z

Ping from triage @csmoe! It's been a while since we heard from you, will you have time to work on this again?

pietroalbini · 2018-06-25T08:28:37Z

Thank you for this PR @csmoe! Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

rust-highfive assigned oli-obk May 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2018

oli-obk requested changes May 18, 2018

View reviewed changes