Do not remove closures during `ReplaceBodyWithLoop` pass #72088

marmeladema · 2020-05-10T20:14:37Z

Fixes #71820

rust-highfive · 2020-05-10T20:14:40Z

r? @ecstatic-morse

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

src/librustc_ast/ast.rs

marmeladema · 2020-05-10T21:18:53Z

cc @petrochenkov

src/librustc_interface/util.rs

ecstatic-morse · 2020-05-11T16:00:54Z

What happens if I have code like the following:

fn foo() {
    let x = |()| {};
}

I think the closure still gets removed because it's in a StmtKind::Local. Is this what we want? If not, we should map these to bare StmtKind::Exprs instead.

marmeladema · 2020-05-11T17:44:23Z

What happens if I have code like the following:
fn foo() {
    let x = |()| {};
}
I think the closure still gets removed because it's in a StmtKind::Local. Is this what we want? If not, we should map these to bare StmtKind::Exprs instead.

Yes the closure would probably gets removed, but i guess its the intended behavior of the simplification pass? The problem in my case is a closure with an exported macro inside. I tried various other code layout (useless blocks, nested closures, etc) but was not able to reproduce the bug when applying this change.

I think the real question is whether or not this simplification pass is needed at all? It looks like a weird hack and can imagine a lot of different bugs due to it.

marmeladema · 2020-05-11T17:47:55Z

What happens if I have code like the following:
fn foo() {
    let x = |()| {};
}

So i just tested your example with a macro inside, and indeed, you are right, i can reproduce the bug.

I think the closure still gets removed because it's in a StmtKind::Local. Is this what we want? If not, we should map these to bare StmtKind::Exprs instead.

Sorry, I am not sure i fully understand why you mean. Could you explain a bit more? Or maybe provide pseudo-code / example?

ecstatic-morse · 2020-05-11T17:48:29Z

My point is that this won't fix #71820 if you have the following, since you're not looking at all places an expression could appear in a Stmt.

fn foo() {
    let x = |()| {
        #[macro_export] macro_rules! foo {}
    };
}

Yeah, so I would add an expr() -> Option<&Expr> method to StmtKind as I mentioned above. You'll also look for the Expr in the Local variant of StmtKind. Then change the filter into a filter_map with a body like stmt.expr().map(|expr| Stmt::new(StmtKind::Expr(expr)) (this is just pseudo-code, you'll need to map items to themselves as well).

ecstatic-morse · 2020-05-11T17:59:23Z

I think the real question is whether or not this simplification pass is needed at all? It looks like a weird hack and can imagine a lot of different bugs due to it.

I can't speak to how rustdoc is using this. Perhaps someone else knows? I'd be surprised if leaving in closures caused anything to break, so I think this approach is fine. That said, you should mention why you're doing this in a comment, since this is very subtle.

edit:

Now that I think of it, there are probably more expressions that will introduce a new HirId just as closures do. For example, async blocks probably work like this?

marmeladema · 2020-05-11T18:34:11Z

Now that I think of it, there are probably more expressions that will introduce a new HirId just as closures do. For example, async blocks probably work like this?

Just tried it, and yes, the bug also happens with async block. Basically, any AST node that can contain a macro and that is going to get removed (ie: not an Item node) would trigger the issue.

ecstatic-morse · 2020-05-11T19:29:54Z

So basically we need to make the AST visitor more robust. Specifically, it should only remove AST nodes that contain no nested items. Does this seem feasible? Unfortunately, ReplaceBodyWithLoop is getting further away from doing what it promises. I think we will need to port rustdoc to a hypothetical libsyntax2 to remove it, however. See #43348 for context on why it is needed for now.

marmeladema · 2020-05-11T22:32:21Z

I've improved the implem based on your feedbacks @ecstatic-morse ! Thanks a lot.
Should we create a ticket for the proper fix and merge this PR in the mean time?

Fixes rust-lang#71820

ecstatic-morse · 2020-05-11T23:30:18Z

I'm good with that. I'll take another look when CI is green.

petrochenkov · 2020-05-11T23:36:16Z

I'm still not sure why this issue happened only with macros but not other items.

ecstatic-morse · 2020-05-11T23:55:21Z

@petrochenkov I have very little intuition for how rustdoc works, but perhaps it is because we run this pass after macro expansion but before AST lowering? Maybe inner items that are not macro definitions are never seen by rustdoc but macro definitions are collected somewhere that is eventually processed. I would have assumed that rustdoc had tests for docs on nested items, however.

rust-highfive · 2020-05-11T23:59:46Z

The job x86_64-gnu-llvm-8 of your PR failed (pretty log, 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.

##[section]Starting: Linux x86_64-gnu-llvm-8
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 3'
Agent machine name: 'fv-az578'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200430.2
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200430.2/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.1)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/16f085c6-138e-43de-a0d4-cee5ea3971de.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72088/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/72088/merge:refs/remotes/pull/72088/merge
---
 ---> cb2676f08729
Step 5/8 : ENV RUST_CONFIGURE_ARGS       --build=x86_64-unknown-linux-gnu       --llvm-root=/usr/lib/llvm-8       --enable-llvm-link-shared       --set rust.thin-lto-import-instr-limit=10
 ---> Using cache
 ---> df25ce111862
Step 6/8 : ENV SCRIPT python2.7 ../x.py test --exclude src/tools/tidy &&            python2.7 ../x.py test src/test/mir-opt --pass=build                                   --target=armv5te-unknown-linux-gnueabi &&            python2.7 ../x.py test src/tools/tidy
 ---> 599b9ac96b27
Step 7/8 : ENV NO_DEBUG_ASSERTIONS=1
 ---> Using cache
 ---> 091087e35a36
---
   Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
   Compiling chalk-rust-ir v0.10.0
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling chalk-solve v0.10.0
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
   Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
   Compiling chalk-rust-ir v0.10.0
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling chalk-solve v0.10.0
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
.....................................................i.............................................. 1800/10160
.................................................................................................... 1900/10160
.......................................................................i..i......................... 2000/10160
.................................................................................................... 2100/10160
.............................................................iiiii.................................. 2200/10160
.................................................................................................... 2400/10160
.................................................................................................... 2500/10160
.................................................................................................... 2600/10160
.................................................................................................... 2700/10160
---
.................................................................................................... 5200/10160
.................................................................................................... 5300/10160
........................i........................................................................... 5400/10160
.................i.................................................................................. 5500/10160
........................ii.ii........i...i.......................................................... 5600/10160
.........................................................................i.......................... 5800/10160
.................................................................................................... 5900/10160
....................ii.....................................i........................................ 6000/10160
.................................................................................................... 6100/10160
.................................................................................................... 6100/10160
.................................................................................................... 6200/10160
.................................................................................ii...i..ii......... 6300/10160
.................................................................................................... 6500/10160
.................................................................................................... 6600/10160
.................................................................................................... 6700/10160
.................................................................................................... 6700/10160
..............i..ii................................................................................. 6800/10160
.................................................................................................... 7000/10160
....................................................................i............................... 7100/10160
.................................................................................................... 7200/10160
.................................................................................................... 7300/10160
---
.................................................................................................... 8100/10160
.................................................................................................... 8200/10160
...................................................................................i................ 8300/10160
.................................................................................................... 8400/10160
.....................................iiiiii.iiiii.i................................................. 8500/10160
.................................................................................................... 8700/10160
.................................................................................................... 8800/10160
.................................................................................................... 8900/10160
.................................................................................................... 9000/10160
---
Suite("src/test/codegen") not skipped for "bootstrap::test::Codegen" -- not in ["src/tools/tidy"]
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 188 tests
iiii......i.............ii.i..........i...............................i..i..................i....i.. 100/188
..........i.i.i...iii..iiiiiiiiiiiiiiii........................iii..............ii......

 finished in 5.565
Suite("src/test/codegen-units") not skipped for "bootstrap::test::CodegenUnits" -- not in ["src/tools/tidy"]
Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/assembly") not skipped for "bootstrap::test::Assembly" -- not in ["src/tools/tidy"]
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 9 tests
iiiiiiiii

 finished in 0.149
Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 115 tests
iiiii..i.....i..i...i..i.i.i..i..i..ii....i.i....ii..........iiii.........i.....i..i.......ii.i.ii.. 100/115
...iiii.....ii.

 finished in 15.473
Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
   Doc-tests alloc
error[E0425]: cannot find function `pred` in this scope
    --> /checkout/src/liballoc/collections/btree/set.rs:1135:52
     |
1135 |         let mut mapped_pred = |k: &T, _v: &mut ()| pred(k);
     |                                                    ^^^^ help: you might have meant to use the available field: `self.pred`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.
error: test failed, to rerun pass '--doc'
---
  local time: Mon May 11 23:59:40 UTC 2020
  network time: Mon, 11 May 2020 23:59:40 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72088/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/72088/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (4398) (python)
##[section]Finishing: Finalize Job

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 @rust-lang/infra. (Feature Requests)

marmeladema · 2020-05-12T07:52:20Z

@petrochenkov @ecstatic-morse My understanding is that the bug is triggered because we try to access the HirId of a parent DefId. This operation does not seem that common(ie: specifically getting the parent DefId then getting its HirId). Moreover its triggered by macros because during lowering, for the ones that are exported, they are registered in a specific "exported macros" vector which is processed later on for visibility checks.

marmeladema · 2020-05-12T08:17:06Z

So keeping the closures, even in the StmtKind::Local case might not the right solution after all because rustdoc now fails in liballoc:

let pred = &mut self.pred;
let mut mapped_pred = |k: &T, _v: &mut ()| pred(k);

First let statement is removed, but second is kept and fails to compile because pred does not exist anymore.

Another idea i had, was to monkey patch the parent DefId of exported macros during lowering. I don't really know the implications of such an approach.

ecstatic-morse · 2020-05-14T17:30:52Z

So basically we need to make the AST visitor more robust. Specifically, it should only remove AST nodes that contain no nested items.

@marmeladema This is what I was hinting at with the comment above. The AST visitor approach will only work if you remove all expressions that don't have nested items. Currently, the AST visitor is only handling block expressions, which means you're missing stuff like the single-expression closure above.

Monkey patching DefIds does seem more robust than stripping AST nodes, but I also don't know how you would go about it. My position is that ReplaceBodyWithLoop is already a hack, so we should keep any fixes internal and avoid changing other parts of the compiler just to make it work.

crlf0710 · 2020-05-22T11:55:07Z

@marmeladema Ping from triage. The CI failure needs to be fixed.

marmeladema · 2020-05-23T10:40:04Z

@crlf0710 its broken but i don't know what is the best way to fix yet. Should I move the PR to draft for the time being?

ecstatic-morse · 2020-05-24T18:50:57Z

@marmeladema Do you mind if I take this over?

marmeladema · 2020-05-24T19:23:21Z

Be my guest 👍 For the record, there are some important refactor work going on right now like #72284 that might help fixing this.

ecstatic-morse · 2020-05-24T21:57:47Z

For the record, there are some important refactor work going on right now like #72284 that might help fixing this.

Sweet. I'll wait for that to land.

petrochenkov · 2020-05-25T05:39:46Z

#72284 seems unrelated to this PR.

ecstatic-morse · 2020-06-02T20:07:09Z

@marmeladema Seems like that PR landed. Is this still desirable? Or is there a different approach in the works?

marmeladema · 2020-06-03T08:39:00Z

So #72284 was indeed unrelated, i got confused with another macro def map that exists during lowering .. sorry for the confusion. I haven't time to try any other approach, feel free to pick that up @ecstatic-morse you will probably be more efficient that me at solving that issue :)

jyn514 · 2020-06-07T23:42:40Z

Let me know if I can help with this, #71820 is blocking #73101.

ecstatic-morse · 2020-06-11T22:15:43Z

Superseded by #73103.

rust-highfive assigned ecstatic-morse May 10, 2020

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

lcnr reviewed May 10, 2020

View reviewed changes

src/librustc_ast/ast.rs Outdated Show resolved Hide resolved

marmeladema marked this pull request as ready for review May 10, 2020 21:18

marmeladema requested a review from lcnr May 10, 2020 23:26

lcnr removed their request for review May 11, 2020 13:08

ecstatic-morse reviewed May 11, 2020

View reviewed changes

src/librustc_interface/util.rs Outdated Show resolved Hide resolved

marmeladema force-pushed the fix-issue-71820 branch from 282f47d to f2de13e Compare May 11, 2020 22:24

Do not remove closures during ReplaceBodyWithLoop pass

1941a43

Fixes rust-lang#71820

marmeladema force-pushed the fix-issue-71820 branch from f2de13e to 1941a43 Compare May 11, 2020 22:38

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2020

jyn514 mentioned this pull request Jun 7, 2020

Resolve items for cross-crate imports relative to the original module #73101

Merged

ecstatic-morse added a commit to ecstatic-morse/rust that referenced this pull request Jun 7, 2020

Add rustdoc tests from rust-lang#72088

e319f20

ecstatic-morse mentioned this pull request Jun 9, 2020

Preserve Exprs that have DefIds in ReplaceBodyWithLoop #73103

Closed

ecstatic-morse closed this Jun 11, 2020

marmeladema deleted the fix-issue-71820 branch June 12, 2020 23:09

ecstatic-morse added a commit to ecstatic-morse/rust that referenced this pull request Jun 16, 2020

Add rustdoc tests from rust-lang#72088

2bfdeca

jyn514 pushed a commit to jyn514/rust that referenced this pull request Jun 20, 2020

Add rustdoc tests from rust-lang#72088

ab30797

jyn514 pushed a commit to jyn514/rust that referenced this pull request Jul 15, 2020

Add rustdoc tests from rust-lang#72088

14a8707

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not remove closures during `ReplaceBodyWithLoop` pass #72088

Do not remove closures during `ReplaceBodyWithLoop` pass #72088

marmeladema commented May 10, 2020

rust-highfive commented May 10, 2020

marmeladema commented May 10, 2020

ecstatic-morse commented May 11, 2020 •

edited

Loading

marmeladema commented May 11, 2020

marmeladema commented May 11, 2020

ecstatic-morse commented May 11, 2020 •

edited

Loading

ecstatic-morse commented May 11, 2020 •

edited

Loading

marmeladema commented May 11, 2020

ecstatic-morse commented May 11, 2020 •

edited

Loading

marmeladema commented May 11, 2020

ecstatic-morse commented May 11, 2020

petrochenkov commented May 11, 2020

ecstatic-morse commented May 11, 2020 •

edited

Loading

rust-highfive commented May 11, 2020

marmeladema commented May 12, 2020

marmeladema commented May 12, 2020

ecstatic-morse commented May 14, 2020 •

edited

Loading

crlf0710 commented May 22, 2020

marmeladema commented May 23, 2020

ecstatic-morse commented May 24, 2020

marmeladema commented May 24, 2020

ecstatic-morse commented May 24, 2020

petrochenkov commented May 25, 2020

ecstatic-morse commented Jun 2, 2020

marmeladema commented Jun 3, 2020

jyn514 commented Jun 7, 2020

ecstatic-morse commented Jun 11, 2020

Do not remove closures during ReplaceBodyWithLoop pass #72088

Do not remove closures during ReplaceBodyWithLoop pass #72088

Conversation

marmeladema commented May 10, 2020

rust-highfive commented May 10, 2020

marmeladema commented May 10, 2020

ecstatic-morse commented May 11, 2020 • edited Loading

marmeladema commented May 11, 2020

marmeladema commented May 11, 2020

ecstatic-morse commented May 11, 2020 • edited Loading

ecstatic-morse commented May 11, 2020 • edited Loading

marmeladema commented May 11, 2020

ecstatic-morse commented May 11, 2020 • edited Loading

marmeladema commented May 11, 2020

ecstatic-morse commented May 11, 2020

petrochenkov commented May 11, 2020

ecstatic-morse commented May 11, 2020 • edited Loading

rust-highfive commented May 11, 2020

marmeladema commented May 12, 2020

marmeladema commented May 12, 2020

ecstatic-morse commented May 14, 2020 • edited Loading

crlf0710 commented May 22, 2020

marmeladema commented May 23, 2020

ecstatic-morse commented May 24, 2020

marmeladema commented May 24, 2020

ecstatic-morse commented May 24, 2020

petrochenkov commented May 25, 2020

ecstatic-morse commented Jun 2, 2020

marmeladema commented Jun 3, 2020

jyn514 commented Jun 7, 2020

ecstatic-morse commented Jun 11, 2020

Do not remove closures during `ReplaceBodyWithLoop` pass #72088

Do not remove closures during `ReplaceBodyWithLoop` pass #72088

ecstatic-morse commented May 11, 2020 •

edited

Loading

ecstatic-morse commented May 11, 2020 •

edited

Loading

ecstatic-morse commented May 11, 2020 •

edited

Loading

ecstatic-morse commented May 11, 2020 •

edited

Loading

ecstatic-morse commented May 11, 2020 •

edited

Loading

ecstatic-morse commented May 14, 2020 •

edited

Loading