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

unwrap_usize should at least try to evaluate the underlying constant #59369

Merged
merged 10 commits into from
Aug 5, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 22, 2019

r? @eddyb

fixes #59016

I know that I'm still using ParamEnv wrongly, but that's a preexisting issue not amplified by this PR.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 23, 2019

cc @nikomatsakis this is related to lazy normalization. I could probably run the regular opportunistic normalization on the types (and thus the array lengths inside the array types) obtained in

let expected_ty = self.structurally_resolved_type(pat.span, expected);
but I'm not sure if we'd ever catch all the cases (the linked issue shows other similar ICEs)

src/librustc/ty/sty.rs Outdated Show resolved Hide resolved
src/librustc/ty/sty.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 28, 2019

@eddyb is 1650513 what you had in mind?

@eddyb
Copy link
Member

eddyb commented Mar 28, 2019

I was thinking of tcx.lift_to_global(&substs).unwrap(), but it makes sense if the type also contains inference variables...
The code is confusing, maybe split the ParamEnvAnd before doing anything to it?

But really, you're calling .unwrap_or_else(|| bug!(...)) - my point was to not panic in those cases, but rather... I don't even know what's happening anymore :(

Your error says this is what can't be lifted:

ParamEnvAnd {                                                                                                                       
    param_env: ParamEnv {
        caller_bounds: [
            Binder(
                OutlivesPredicate(
                    '_#0r,
                    '_#1r
                )
            )
        ],
        reveal: UserFacing,
        def_id: None
    },
    value: usize
}

But this can't really exist? Ohhh, you need to convert it to a RevealAll ParamEnv and then call .and(ty) on it, to pass it to layout_of. That will clear all the irrelevant bounds!

src/librustc/ty/sty.rs Outdated Show resolved Hide resolved
src/librustc/ty/sty.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Apr 2, 2019

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

@bors
Copy link
Contributor

bors commented Apr 13, 2019

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

@Dylan-DPC-zz
Copy link

ping from triage @eddyb waiting for your review on this

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 23, 2019

Nope, I still have things to address, sorry about that

@oli-obk oli-obk 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 Apr 23, 2019
@eddyb
Copy link
Member

eddyb commented Apr 24, 2019

This is also waiting on @nikomatsakis for a few questions.

@nikomatsakis nikomatsakis self-assigned this Apr 30, 2019
@pnkfelix
Copy link
Member

pnkfelix commented May 2, 2019

cc #60471 since this is "related to lazy normalization"

@oli-obk
Copy link
Contributor Author

oli-obk commented May 17, 2019

@nikomatsakis this is ready for review again.

The big question related to lazy normalization is whether it's ok that this evalutes (some) constants at the point where someone is trying to act upon the actual value of the constant.

Eager normalization happens in

fn fold_const(&mut self, constant: &'tcx ty::Const<'tcx>) -> &'tcx ty::Const<'tcx> {
and
fn fold_const(&mut self, constant: &'tcx ty::Const<'tcx>) -> &'tcx ty::Const<'tcx> {

EDIT for clarity: this PR does not change eager normalization. We just do lazy normalization of some constants that were never eagerly normalized.

@oli-obk oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2019
@eddyb
Copy link
Member

eddyb commented May 22, 2019

@oli-obk I would expect that for now eager normalization would still resolve at least everything which evaluates successfully, right?
I mean, that is the case for associated type projections, I don't see why consts would be different.

@bors
Copy link
Contributor

bors commented May 25, 2019

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

@wirelessringo
Copy link

Ping from triage, @eddyb @Centril any updates on this? Thanks

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 5, 2019

@bors r=eddyb,nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 5, 2019

📌 Commit bd57498 has been approved by eddyb,nikomatsakis

@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 5, 2019
@bors
Copy link
Contributor

bors commented Aug 5, 2019

⌛ Testing commit bd57498 with merge c471519...

bors added a commit that referenced this pull request Aug 5, 2019
`unwrap_usize` should at least try to evaluate the underlying constant

r? @eddyb

fixes #59016

I know that I'm still using `ParamEnv` wrongly, but that's a preexisting issue not amplified by this PR.
@bors
Copy link
Contributor

bors commented Aug 5, 2019

☀️ Test successful - checks-azure
Approved by: eddyb,nikomatsakis
Pushing c471519 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2019
@bors bors merged commit bd57498 into rust-lang:master Aug 5, 2019
tesuji added a commit to tesuji/rust-clippy that referenced this pull request Aug 6, 2019
tesuji added a commit to tesuji/rust-clippy that referenced this pull request Aug 6, 2019
tesuji added a commit to tesuji/rust-clippy that referenced this pull request Aug 6, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 7, 2019
@oli-obk oli-obk deleted the unwrap_usICE branch March 16, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lazy-normalization Area: lazy normalization (tracking issue: #60471) 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.

Compiler panic when using a slice pattern
10 participants