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

use type folder + normalization for MIR assignment type-checking #75419

Closed
wants to merge 2 commits into from

Conversation

RalfJung
Copy link
Member

Fixes #75313
Thanks to @eddyb who did all the hard work, I just implemented his suggestions.^^

@lcnr Unfortunately this loses the type relator, we normalize the entire thing instead. There might be a perf hit.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Aug 11, 2020
@RalfJung
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Aug 11, 2020

⌛ Trying commit 31a0251 with merge 0caf8b37da267b885ad6f55b86dde01b897d7068...

@eddyb
Copy link
Member

eddyb commented Aug 11, 2020

As per #75313 (comment), we likely shouldn't need to land this, once we fix normalization of ty::Opaque.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

This seems fine if perf isn't too bad.

Do we have a minimal example of what breaks if we do not erase all lifetimes before calling normalize_erasing_regions?

@bors
Copy link
Contributor

bors commented Aug 11, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 0caf8b37da267b885ad6f55b86dde01b897d7068 (0caf8b37da267b885ad6f55b86dde01b897d7068)

@rust-timer
Copy link
Collaborator

Queued 0caf8b37da267b885ad6f55b86dde01b897d7068 with parent cbe7c5c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0caf8b37da267b885ad6f55b86dde01b897d7068): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@RalfJung
Copy link
Member Author

Perf seems to be mostly noise?

@lcnr
Copy link
Contributor

lcnr commented Aug 12, 2020

As per #75313 (comment), we likely shouldn't need to land this, once we fix normalization of ty::Opaque.

Do we want to wait for that or just land this as is, can try that change locally and see if it breaks something 🤔

@lcnr
Copy link
Contributor

lcnr commented Aug 12, 2020

opened #75443 for this, let's see if niko finds a problem with that approach

@RalfJung
Copy link
Member Author

opened #75443 for this, let's see if niko finds a problem with that approach

If CI passes there then that's definitely the better solution I'd say. :)

@RalfJung
Copy link
Member Author

#75443 landed, closing in favor of that.

@RalfJung RalfJung closed this Aug 17, 2020
@RalfJung RalfJung deleted the mir-assign-check branch August 17, 2020 12:12
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2020
revert rust-lang#75443, update mir validator

This PR reverts rust-lang#75443 to fix rust-lang#75992 and instead uses rust-lang#75419 to fix rust-lang#75313.

Adapts rust-lang#75419 to correctly deal with unevaluated constants as otherwise some `feature(const_evaluatable_checked)` tests would ICE.

Note that rust-lang#72793 was also fixed by rust-lang#75443, but as that issue only concerns `feature(type_alias_impl_trait)` I deleted that test case for now and would reopen that issue.

rust-lang#75443 may have also allowed some other code to now successfully compile which would make this revert a breaking change after 2 stable versions, but I hope that this is a purely theoretical concern.

See https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/generator.20upvars/near/214617274 for more reasoning about this.

r? `@nikomatsakis` `@eddyb` `@RalfJung`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when compiling gluon on 1.46
6 participants