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

Bail on any found recursion when expanding opaque types #87546

Merged
merged 2 commits into from
Aug 1, 2021

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Jul 28, 2021

Fixes #87450. More of a bandaid because it does not fix the exponential complexity of the type folding used for opaque type expansion.

Fixes rust-lang#87450. More of a bandaid because it does not fix the exponential complexity of the type folding used for opaque type expansion.
@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 @davidtwco (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2021
@hkratz hkratz marked this pull request as ready for review July 28, 2021 13:19
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Could you add the test case from #87450?

@davidtwco davidtwco 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 Jul 30, 2021
@lqd
Copy link
Member

lqd commented Jul 30, 2021

Since this test is exercizing exponential compile times, how long does it take to compile with this PR ? (it would be good not to slow down CI and contributors' tests too much with a huge stress test :)

@hkratz
Copy link
Contributor Author

hkratz commented Jul 30, 2021

Since this test is exercizing exponential compile times, how long does it take to compile with this PR ? (it would be good not to slow down CI and contributors' tests too much with a huge stress test :)

It is pretty fast on the testcase from issue 87450: From >400 seconds with master down to 0.06:

$ rm build/x86_64-unknown-linux-gnu/test/ui/impl-trait/issue-87450/stamp; ./x.py test src/test/ui/impl-trait/issue-87450.rs
[...]
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 12184 filtered out; finished in 0.06s

One would need to add much more recursion to get a significant runtime (i.e. >20 wrap() calls).

@hkratz hkratz requested a review from davidtwco July 30, 2021 19:14
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 30, 2021

📌 Commit 2aa1996 has been approved by davidtwco

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 30, 2021
@bors
Copy link
Contributor

bors commented Jul 31, 2021

⌛ Testing commit 2aa1996 with merge 92f015cefdfc37b37b6e9392a638f1fc9fdf3b7f...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed
thread 'main' panicked at 'assertion failed: status.success()', src/tools/cargotest/main.rs:125:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/cargotest" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools-bin/cargo" "/checkout/obj/build/ct"
expected success, got: exit status: 101
expected success, got: exit status: 101


Build completed unsuccessfully in 0:24:57
make: *** [check-aux] Error 1
Makefile:44: recipe for target 'check-aux' failed

@bors
Copy link
Contributor

bors commented Jul 31, 2021

💔 Test failed - checks-actions

@bors 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 Jul 31, 2021
@hkratz
Copy link
Contributor Author

hkratz commented Jul 31, 2021

error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.
fatal: The remote end hung up unexpectedly

Looks like an intermittent connectivity issue?

@jyn514
Copy link
Member

jyn514 commented Aug 1, 2021

@bors retry

@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 Aug 1, 2021
@bors
Copy link
Contributor

bors commented Aug 1, 2021

⌛ Testing commit 2aa1996 with merge 8d57c0a...

@bors
Copy link
Contributor

bors commented Aug 1, 2021

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 8d57c0a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 1, 2021
@bors bors merged commit 8d57c0a into rust-lang:master Aug 1, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 1, 2021
@hkratz hkratz deleted the issue87450-take-two branch August 4, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

Exponential compile time with recursive opaque type
8 participants