Don't lint for self-recursion when the function can diverge #70822

jonas-schievink · 2020-04-05T19:03:28Z

Fixes #54444

rust-highfive · 2020-04-05T19:03:31Z

r? @ecstatic-morse

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

src/librustc_mir_build/lints.rs

Centril · 2020-04-05T22:48:13Z

src/librustc_mir_build/lints.rs

+    tcx: TyCtxt<'tcx>,
+    body: &Body<'tcx>,
+    def_id: DefId,
+) -> BitSet<BasicBlock> {


Could use more commentary in this function.

I tried, but I'm not sure I fully understand it

ecstatic-morse · 2020-04-06T00:32:32Z

I think this is more complex than it could be. To fix the first issue in #54444, you can suppress the lint if Body::is_cfg_cyclic returns true. This treats any loop as possibly infinite, which I believe this does as well? For the second issue, you just need to properly handle terminators that could unwind by traversing all reachable basic blocks and looking for unwind edges as well as returns. Am I missing something here?

jonas-schievink · 2020-04-06T00:43:51Z

I think checking for cycles in the cfg is not enough - we do want to lint if there's a call before the cycle, but not if it comes after.

ecstatic-morse · 2020-04-06T01:03:54Z

Ah, that's right. In that case you would need to check for back-edges during the traversal, not as a separate step. There's a DFS framework with decent documentation that can differentiate between a cycle and a node with multiple predecessors. You could almost just use it directly, but you need to ignore successors of recursive Call terminators, and that framework works on generic graphs.

Adding an ignore_successors method to

rust/src/librustc_data_structures/graph/iterate/mod.rs

Line 237 in b543afc

pub trait TriColorVisitor<G>

and checking it before pushing successors onto the stack would work:

rust/src/librustc_data_structures/graph/iterate/mod.rs

Lines 204 to 213 in b543afc

    
           if prior_status.is_some() { 
        
               continue; 
        
           } 
        
           // Otherwise, push a `Settled` event for this node onto the stack, then 
        
           // schedule its successors for examination. 
        
           self.stack.push(Event { node, becomes: Settled }); 
        
           for succ in self.graph.successors(node) { 
        
               self.stack.push(Event { node: succ, becomes: Visited }); 
        
           }

I do think it's worth implementing this using forward traversal since we can avoid computing the predecessor graph, but if you're happy with your approach, we could merge it with some more documentation. WDYT?

jonas-schievink · 2020-04-06T10:18:34Z

Oh that's neat, didn't know there was an entire framework for that.

I do think it's worth implementing this using forward traversal since we can avoid computing the predecessor graph

It was already getting computed anyways. I'm not sure what's using it though.

jonas-schievink · 2020-04-06T10:22:32Z

I guess this makes sense since the query returns &'tcx Steal<BodyAndCache<'tcx>>, so not having the predecessors updated means the users of the query would have to do it.

jonas-schievink · 2020-04-06T23:14:36Z

@ecstatic-morse okay, added some more comments. Let me know if there's anything else.

ecstatic-morse

I asked for some clarification/changes in a few places that confused me initially, but I think this is good overall. I reserve the right to try rewriting this to use only forward traversal 😁.

r=me with comments (mine and @Centril's) addressed.

src/librustc_mir_build/lints.rs

jonas-schievink · 2020-04-07T17:32:57Z

@bors r=ecstatic-morse

bors · 2020-04-07T17:32:58Z

📌 Commit b8f416d has been approved by ecstatic-morse

bors · 2020-04-07T17:42:58Z

⌛ Testing commit b8f416d with merge cc6ffab7291b3fb399bd30bd4b5a9fb25634bc6e...

bors · 2020-04-07T17:56:56Z

💔 Test failed - checks-azure

jonas-schievink · 2020-04-08T12:46:33Z

@bors r=ecstatic-morse

bors · 2020-04-08T12:46:35Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2020-04-08T12:46:36Z

📌 Commit b8f416d has been approved by ecstatic-morse

bors · 2020-04-08T12:46:36Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@ghost

Rollup of 7 pull requests Successful merges: - rust-lang#67705 (Use unrolled loop for searching NULL in [u16] on Windows) - rust-lang#70367 (save/restore `pessimistic_yield` when entering bodies) - rust-lang#70822 (Don't lint for self-recursion when the function can diverge) - rust-lang#70868 (rustc_codegen_ssa: Refactor construction of linker arguments) - rust-lang#70896 (Implement Chain with Option fuses) - rust-lang#70916 (Support `#[track_caller]` on functions in `extern "Rust" { ... }`) - rust-lang#70918 (rustc_session: forbid lints override regardless of position) Failed merges: r? @ghost

@jonas-schievink

…nas-schievink Use forward traversal for unconditional recursion lint While reviewing rust-lang#70822, I noted that rust-lang#54444 could be solved without requiring the predecessor graph and without allocating a `Vec<Span>` for every basic block. The unconditional recursion lint is not a performance bottleneck however, so I approved rust-lang#70822 as it was. Nevertheless, I wanted to try implementing my idea using `TriColorDepthFirstSearch`, which is a DFS that can differentiate between [forward/tree edges and backward ones](https://en.wikipedia.org/wiki/Depth-first_search#Output_of_a_depth-first_search). I found this approach more straightforward than the existing one, so I'm opening this PR to see if it is desirable. The pass is now just a DFS across the control-flow graph. We ignore false edges and false unwinds, as well as the successors of recursive calls, just like existing pass does. If we see a back-edge (loop) or a terminator that would cause us to yield control-flow back to the caller (`Return`, `Resume`, etc.), we know that the function does not unconditionally recurse. r? @jonas-schievink

rust-highfive assigned ecstatic-morse Apr 5, 2020

This comment has been minimized.

Sign in to view

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2020

Centril reviewed Apr 5, 2020

View reviewed changes

jonas-schievink added 4 commits April 6, 2020 00:53

Don't lint for self-recursion when the function can diverge

87d859a

Move closure check upwards

9dd18a3

Merge redundant match arms

60e9927

Add some comments and rename variable

dec9078

Add some more comments

b30d906

ecstatic-morse reviewed Apr 7, 2020

View reviewed changes

src/librustc_mir_build/lints.rs Outdated Show resolved Hide resolved

src/librustc_mir_build/lints.rs Outdated Show resolved Hide resolved

Further improve comments

b8f416d

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 Apr 7, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 7, 2020

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 Apr 8, 2020

Centril mentioned this pull request Apr 9, 2020

Rollup of 7 pull requests #70943

Merged

bors merged commit a209b4f into rust-lang:master Apr 9, 2020

ecstatic-morse mentioned this pull request Apr 10, 2020

Use forward traversal for unconditional recursion lint #70973

Merged

jonas-schievink mentioned this pull request Oct 28, 2020

unconditional_recursion lint doesn't work with struct update syntax #78474

Closed

jonas-schievink deleted the curse-of-the-recursion branch October 28, 2020 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't lint for self-recursion when the function can diverge #70822

Don't lint for self-recursion when the function can diverge #70822

jonas-schievink commented Apr 5, 2020

rust-highfive commented Apr 5, 2020

This comment has been minimized.

Centril Apr 5, 2020

jonas-schievink Apr 5, 2020

ecstatic-morse commented Apr 6, 2020

jonas-schievink commented Apr 6, 2020

ecstatic-morse commented Apr 6, 2020 •

edited

Loading

jonas-schievink commented Apr 6, 2020

jonas-schievink commented Apr 6, 2020

jonas-schievink commented Apr 6, 2020

ecstatic-morse left a comment

jonas-schievink commented Apr 7, 2020

bors commented Apr 7, 2020

bors commented Apr 7, 2020

bors commented Apr 7, 2020

jonas-schievink commented Apr 8, 2020

bors commented Apr 8, 2020

bors commented Apr 8, 2020

bors commented Apr 8, 2020

Don't lint for self-recursion when the function can diverge #70822

Don't lint for self-recursion when the function can diverge #70822

Conversation

jonas-schievink commented Apr 5, 2020

rust-highfive commented Apr 5, 2020

This comment has been minimized.

Centril Apr 5, 2020

Choose a reason for hiding this comment

jonas-schievink Apr 5, 2020

Choose a reason for hiding this comment

ecstatic-morse commented Apr 6, 2020

jonas-schievink commented Apr 6, 2020

ecstatic-morse commented Apr 6, 2020 • edited Loading

jonas-schievink commented Apr 6, 2020

jonas-schievink commented Apr 6, 2020

jonas-schievink commented Apr 6, 2020

ecstatic-morse left a comment

Choose a reason for hiding this comment

jonas-schievink commented Apr 7, 2020

bors commented Apr 7, 2020

bors commented Apr 7, 2020

bors commented Apr 7, 2020

jonas-schievink commented Apr 8, 2020

bors commented Apr 8, 2020

bors commented Apr 8, 2020

bors commented Apr 8, 2020

ecstatic-morse commented Apr 6, 2020 •

edited

Loading