Uh oh!
There was an error while loading. Please reload this page.
Don't use lift to detect local types - #61871
Conversation
rust-highfive
commented
Jun 15, 2019
The job Click to expand the log.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-highfive
commented
Jun 15, 2019
The job Click to expand the log.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 |
Zoxc
commented
Jun 15, 2019
@bors try |
bors
commented
Jun 15, 2019
⌛ Trying commit 90fbfd7095ad0b022adb968bbc72ce51b55e2467 with merge 3e509e149d6644da28c5e2b642d164845e724047... |
There was a problem hiding this comment.
cc @rust-lang/compiler I think it might be better if we stop referring to these as "local values" and instead use something more like .has_infer_vars()?
There was a problem hiding this comment.
has_infer_types already exist and is a different property.
There was a problem hiding this comment.
Hmm, if self.infcx is None, this case should be impossible - it's plausible this code is unnecessarily defensive and lift_to_global's result could've been unwrap'd all along.
There was a problem hiding this comment.
I can't wait for this to use erasure and canonicalization instead (@oli-obk and @nikomatsakis are working on that, I think?).
There was a problem hiding this comment.
@rust-lang/wg-traits This is scary, can we do a crater run with a delay_span_bug in here?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Is it really that expensive? Anyway it should be impossible, given a correct Resolver.
There was a problem hiding this comment.
I don't know how expensive these are. They're not free though, so I'd prefer to not have them for release builds.
And yes, it should be impossible, that's why it's an assertion =P
There was a problem hiding this comment.
Hmm, can we do a crater run with all of these assertions turned on by default, to confirm they're very likely never triggered, and then remove them?
There was a problem hiding this comment.
They're effectively turned on now, and they have caught at least one bug #46184.
I do think we should have these assertions somewhere though, to ensure that no infer types leaks out from typeck. I could just use debug_assert here instead of span_bug to make them more slim.
There was a problem hiding this comment.
I'd prefer if one of two things happened here (and in other places match-ing over lift_to_global results):
- land a PR on master replacing the
matches withif lets (you can just r=me that if you want) - keep the
matchand either useOption::filteror justmatchon theboolpredicate
That is, I would prefer if indentation didn't change (and it only does because of the match -> if change).
There was a problem hiding this comment.
Why do you not want indention to change? I like less rightward drift, and I definitely don't like matching on bools.
There was a problem hiding this comment.
It's fine to change, I just want to be able to read the diff without mentally pretending nothing changed without checking, where indentation is involved.
There was a problem hiding this comment.
I changed the indentation to reduce the diffs in this PR.
There was a problem hiding this comment.
@eddyb you can append ?w=1 to the diff url to have whitespace insensitive diffs https://github.com/rust-lang/rust/pull/61871/files?w=1
There was a problem hiding this comment.
Nice. I also reordered some branches which that can't deal with though =P
There was a problem hiding this comment.
Same as the other match -> if change.
a35c705 to
fabe708Comparebors
commented
Jun 19, 2019
☔ The latest upstream changes (presumably #61945) made this pull request unmergeable. Please resolve the merge conflicts. |
rust-highfive
commented
Jun 19, 2019
The job Click to expand the log.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 |
eddyb
commented
Jul 1, 2019
@bors r+ |
bors
commented
Jul 1, 2019
📌 Commit 8465daf has been approved by |
bors
commented
Jul 1, 2019
⌛ Testing commit 8465daf with merge ebe126b40db5acb422774453b0d9a2d821bb3755... |
emilyalbini
commented
Jul 1, 2019
@bors retry Yielding priority to the Azure migration. |
Don't use lift to detect local types This overlaps with rust-lang#61392. r? @eddyb
bors
commented
Jul 2, 2019
bors
commented
Jul 2, 2019
☀️ Test successful - checks-azure, checks-travis, status-appveyor |
This overlaps with #61392.
r? @eddyb