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

Do not ICE when combining unsized locals and async #64527

Closed
wants to merge 1 commit into from

Conversation

estebank
Copy link
Contributor

Fix #61335.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2019
}
}
}
}
Err(LayoutError::Unsized(_ty)) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add an if ty.is_generator() here, which would ensure the correct error is emitted, and just ICE in any other (in theory unreachable) case.

}
}
}
}
Err(LayoutError::Unsized(_ty)) => {
let sp = statement.source_info.span; // async fn block
self.tcx.sess.struct_span_err(
Copy link
Contributor

@Centril Centril Sep 16, 2019

Choose a reason for hiding this comment

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

Uhm; const_prop is too my knowledge an optimization transform not necessary for correctness so why are we emitting an error here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I think it's totally fine to just keep doing what the old code did and ignore errors. They may be occuring because we're in a generic function and may be bogus for specific concrete types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we close this PR then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well; we cannot merge this PR as-is I think but maybe we can morph the PR into something else. You can always open a new one tho... up to you. :)

@Centril Centril added the F-unsized_locals `#![feature(unsized_locals)]` label Sep 16, 2019
@rust-highfive
Copy link
Collaborator

The job mingw-check 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.
2019-09-16T23:05:06.9297783Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-16T23:05:06.9489989Z ##[command]git config gc.auto 0
2019-09-16T23:05:06.9562880Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-16T23:05:06.9621225Z ##[command]git config --get-all http.proxy
2019-09-16T23:05:06.9766400Z ##[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/64527/merge:refs/remotes/pull/64527/merge
---
2019-09-16T23:14:09.3306147Z     Checking rustc_mir v0.0.0 (/checkout/src/librustc_mir)
2019-09-16T23:14:09.4901056Z error: this file contains an un-closed delimiter
2019-09-16T23:14:09.4902525Z    --> src/librustc_mir/transform/const_prop.rs:824:3
2019-09-16T23:14:09.4904053Z     |
2019-09-16T23:14:09.4904635Z 651 | impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
2019-09-16T23:14:09.4905236Z     |                                                                   - un-closed delimiter
2019-09-16T23:14:09.4905641Z ...
2019-09-16T23:14:09.4906147Z 690 |                                 if self.should_const_prop() {
2019-09-16T23:14:09.4906879Z     |                                                             - this delimiter might not be properly closed...
2019-09-16T23:14:09.4907665Z 696 |                             }
2019-09-16T23:14:09.4908158Z     |                             - ...as it matches this but it has different indentation
2019-09-16T23:14:09.4908499Z ...
2019-09-16T23:14:09.4908879Z 824 | }
2019-09-16T23:14:09.4908879Z 824 | }
2019-09-16T23:14:09.4909249Z     |   ^
2019-09-16T23:14:09.4909389Z 
2019-09-16T23:14:09.4928369Z error: expected one of `.`, `;`, `?`, `}`, or an operator, found `=>`
2019-09-16T23:14:09.4929713Z     |
2019-09-16T23:14:09.4929713Z     |
2019-09-16T23:14:09.4930176Z 700 |                 Err(LayoutError::Unsized(_ty)) => {
2019-09-16T23:14:09.4930725Z     |                                                ^^ expected one of `.`, `;`, `?`, `}`, or an operator here
2019-09-16T23:14:09.4951336Z error: unexpected `self` parameter in function
2019-09-16T23:14:09.4951939Z    --> src/librustc_mir/transform/const_prop.rs:715:9
2019-09-16T23:14:09.4952355Z     |
2019-09-16T23:14:09.4952914Z 715 |         &mut self,
---
2019-09-16T23:14:11.5114772Z == clock drift check ==
2019-09-16T23:14:11.5134534Z   local time: Mon Sep 16 23:14:11 UTC 2019
2019-09-16T23:14:11.6657494Z   network time: Mon, 16 Sep 2019 23:14:11 GMT
2019-09-16T23:14:11.6658014Z == end clock drift check ==
2019-09-16T23:14:12.4254934Z ##[error]Bash exited with code '1'.
2019-09-16T23:14:12.4286848Z ##[section]Starting: Checkout
2019-09-16T23:14:12.4288502Z ==============================================================================
2019-09-16T23:14:12.4288568Z Task         : Get sources
2019-09-16T23:14:12.4288609Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@@ -137,6 +137,7 @@ impl<'tcx> ConstEvalErr<'tcx> {
) -> Result<DiagnosticBuilder<'tcx>, ErrorHandled> {
let must_error = match self.error {
err_inval!(Layout(LayoutError::Unknown(_))) |
err_inval!(Layout(LayoutError::Unsized(_))) |
Copy link
Member

Choose a reason for hiding this comment

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

An unsized type isnt generic, so returning TooGeneric doesnt make sense.

Copy link
Member

Choose a reason for hiding this comment

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

But that's also why this entire approach looks odd... a generic type will eventually be monomorphized, so that's a program that cannot be executed now but can be executed later. But if the type is unsized now it will always be unsized, right, so why does it make any sense to delay the error?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea that should just be reported via the machinery below, which should simplify the rest

@bors
Copy link
Contributor

bors commented Sep 28, 2019

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

@JohnCSimon JohnCSimon 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 Oct 5, 2019
@JohnCSimon
Copy link
Member

Ping from triage
@estebank Can you please address the merge conflicts and address the comments?
CC: @matthewjasper
Thanks.

@JohnCSimon
Copy link
Member

Pinging again from triage
@estebank Can you please address the merge conflicts and address the comments?
CC: @matthewjasper
Thanks.

@Centril Centril added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2019
@Centril
Copy link
Contributor

Centril commented Oct 16, 2019

This hasn't seen any movement for some time so I'm going to close it out as inactive.
As always, feel free to reopen whenever (but ideally with a more sound approach ^^).

@Centril Centril closed this Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-unsized_locals `#![feature(unsized_locals)]` S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE when combining unsized locals and async
9 participants