fix universes in the NLL type tests #98109

nikomatsakis · 2022-06-14T20:28:15Z

In the NLL code, we were not accommodating universes in the
type_test logic.

Fixes #98095.

r? @compiler-errors

This breaks some tests, however, so the purpose of this branch is more explanatory and perhaps to do a crater run.

compiler-errors · 2022-06-14T20:28:50Z

@bors try

bors · 2022-06-14T20:28:59Z

⌛ Trying commit 54d4fee2b42bbaa0073f89a7b979ed1cbb34f34b with merge 363598f22faa9d0e3a12f5b6d2b118bc77f83bf7...

compiler-errors · 2022-06-14T20:32:11Z

marking this as experimental since it probably shouldn't be merged, since it breaks existing UI tests

compiler-errors · 2022-06-14T20:33:34Z

compiler/rustc_borrowck/src/region_infer/mod.rs

+
+            return self.eval_outlives(sup_region, self.universal_regions.fr_static);
+        }
+
        // Both the `sub_region` and `sup_region` consist of the union
        // of some number of universal regions (along with the union
        // of various points in the CFG; ignore those points for


I'm curious if the logic below (let universal_outlives = ...) is necessary due to the added logic above. I don't know enough about borrowck to make a judgement.

I believe it is still relevant. I think that the code below is meant to deal with "top-level" generics declared on the function, where we do have finer-grained outlives data.

In other words, it deals with relationships between universals and variables that are in the same universe.

bors · 2022-06-14T22:04:29Z

☀️ Try build successful - checks-actions
Build commit: 363598f22faa9d0e3a12f5b6d2b118bc77f83bf7 (363598f22faa9d0e3a12f5b6d2b118bc77f83bf7)

the excessive generality becomes annoying later because it wouldn't implement type folding etc

we don't really take other things

The code now accepts `Binder<OutlivesPredicate>` instead of just `OutlivesPredicate` and thus exercises the new, generalized `IfEqBound` codepaths. Note though that we never *produce* Binder<OutlivesPredicate>, so we are only testing a subset of those codepaths that excludes actual higher-ranked outlives bounds.

jackh726 · 2022-06-15T16:07:32Z

src/test/ui/regions/forall-wf-ref-reflexive.rs

+// Test that we consider `for<'a> &'a T: 'a` to be sufficient to prove
+// that `for<'a> &'a T: 'a`.
+//
+// FIXME. Except we don't!


I believe so, certainly a pre-existing one. This feels like an implied bounds situation to me. We ought to be able to know that for<'a> &'a T: 'a is really for<'a> if (WF(&'a T)) { &'a T: 'a }, and -- in that case -- we would be succesful here.

I think @jackh726 means using the // known-bug attribute

Ah, cool. Citing which bug?

compiler/rustc_infer/src/infer/outlives/verify.rs

In the NLL code, we were not accommodating universes in the `type_test` logic. This led to issue 98095.

These were "fixed" as part of switching on NLL but seems to be due to another problem. Preliminary investigation suggests they are both PROBABLY "implied bounds" related.

Co-authored-by: Oli Scherer <github35764891676564198441@oli-obk.de>

Hat-tip: aliemjay

per aliemjay's suggestion

jackh726 · 2022-06-24T04:59:22Z

@bors r+ p=5 rollup=never

We want this in before beta cut, since this fixes a stable-to-nightly unsound regression, caused by NLL stabilization.

There are two known regressions here, but one requires GATs (unstable) and the other has an obvious fix (remove the 'a bound). Given this, it makes sense to land this. We might be able to fix and backport a fix for those; but if not, it's not the end of the world.

I think the code here and adjacent would be a good candidate for a types team deep dive; this feels a bit ad-hoc to me, even if correct (but I have little personal context to the overall design of this logic).

bors · 2022-06-24T04:59:24Z

📌 Commit e7ed8fe has been approved by jackh726

bors · 2022-06-24T07:39:32Z

⌛ Testing commit e7ed8fe with merge d017d59...

bors · 2022-06-24T10:20:11Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing d017d59 to master...

rust-timer · 2022-06-24T11:59:55Z

Finished benchmarking commit (d017d59): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	0.6%	1.9%	31
Regressions 😿 (secondary)	0.9%	2.0%	35
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.6%	1.9%	31

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	4.0%	4.0%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-8.4%	-8.4%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	1.7%	1.8%	3
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.9%	-2.9%	1
All 😿🎉 (primary)	1.7%	1.8%	3

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

oli-obk · 2022-06-24T13:20:58Z

In diesel-check-full, the main regression is

205,739,874  ???:<rustc_middle::ty::context::CtxtInterners>::intern_ty
190,507,805  ???:<alloc::vec::Vec<rustc_middle::ty::sty::Binder<rustc_middle::ty::OutlivesPredicate<rustc_middle::ty::Ty, rustc_middle::ty::sty::Region>>> as alloc::vec::spec_from_iter::SpecFromIter<rustc_middle::ty::sty::Binder<rustc_middle::ty::OutlivesPredicate<rustc_middle::ty::Ty, rustc_middle::ty::sty::Region>>, core::iter::adapters::inspect::Inspect<core::iter::adapters::chain::Chain<core::iter::adapters::filter::Filter<core::iter::adapters::filter_map::FilterMap<core::iter::adapters::copied::Copied<core::slice::iter::Iter<rustc_middle::ty::Predicate>>, <rustc_infer::infer::outlives::verify::VerifyBoundCx>::collect_outlives_from_predicate_list<core::iter::adapters::copied::Copied<core::slice::iter::Iter<rustc_middle::ty::Predicate>>>::{closure

the second one is entirely due to the logic in can_match_erased_ty. There may be some possible gains if we skip the binder first and then erase, but it generally won't help much. Ideally we'd just relate the types while ignoring lifetimes.

rylev · 2022-06-28T15:57:25Z

@oli-obk should we capture this in an issue? I think it's pretty likely that this regression will go unaddressed if we leave it in the comment here.

oli-obk · 2022-06-28T16:07:32Z

This is a soundness fix, the additional checking is necessary for soundness. I don't believe the regression is directly addressable, even if we can do some improvements here.

I'll open a PR for the binder skipping thing to see if it has any effect, but I don't think it'll be significant.

rylev · 2022-06-28T17:00:49Z

Ok I think we can mark this as triaged then.

@rustbot label: +perf-regression-triaged

context: rust-lang/rust#98109

rust-highfive assigned compiler-errors Jun 14, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 14, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2022

compiler-errors added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2022

compiler-errors reviewed Jun 14, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

compiler-errors mentioned this pull request Jun 14, 2022

NLL: unsound verification of higher ranked outlives bounds #98095

Closed

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

nikomatsakis force-pushed the issue-98095 branch from cfd13a9 to 9c863ee Compare June 15, 2022 14:31

nikomatsakis added 6 commits June 15, 2022 11:47

simply the IfEq bound -- we only ever use a region

d203c13

the excessive generality becomes annoying later because it wouldn't implement type folding etc

implement (unused) matching solver

c5ed318

simplify argument type for to take param-ty

e260afc

we don't really take other things

remove compare-fn and use erased-ty

87c2907

improved debug output

10f0f66

nikomatsakis force-pushed the issue-98095 branch from 9c62a87 to d6b3473 Compare June 15, 2022 15:51

jackh726 reviewed Jun 15, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

nikomatsakis added 3 commits June 15, 2022 17:28

fix universes in the NLL type tests

12912b9

In the NLL code, we were not accommodating universes in the `type_test` logic. This led to issue 98095.

mark issue-91139 and issue-92096 as FIXME

ad25ee0

These were "fixed" as part of switching on NLL but seems to be due to another problem. Preliminary investigation suggests they are both PROBABLY "implied bounds" related.

tests H.R. T: 'a bounds proving themselves

b39ba21

nikomatsakis force-pushed the issue-98095 branch from d6b3473 to b39ba21 Compare June 15, 2022 21:28

apply suggestions from oli-obk

e6b630c

Co-authored-by: Oli Scherer <github35764891676564198441@oli-obk.de>

This comment has been minimized.

Sign in to view

nikomatsakis added 4 commits June 23, 2022 17:00

normalize if-eq bounds before testing

c3137d9

Hat-tip: aliemjay

try to clarify confusing comment

d8337ee

remove misleading comment

9118faf

per aliemjay's suggestion

run x.py fmt

e93e105

nikomatsakis mentioned this pull request Jun 23, 2022

for<'a> &'a T: 'a and closures regressed #98437

Open

add regression tests found in crater

e7ed8fe

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 Jun 24, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 24, 2022

bors merged commit d017d59 into rust-lang:master Jun 24, 2022

rustbot added this to the 1.63.0 milestone Jun 24, 2022

bors mentioned this pull request Jun 24, 2022

Shorten def_span for more items. #93967

Merged

oli-obk mentioned this pull request Jun 28, 2022

skip binder before erasing. #98628

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 28, 2022

aliemjay added a commit to aliemjay/snocat that referenced this pull request Jul 31, 2022

avoid incoming breakage of for<'a> &'a T: 'a bounds

d7ccdd8

context: rust-lang/rust#98109

aliemjay mentioned this pull request Jul 31, 2022

avoid incoming breakage of for<'a> &'a T: 'a bounds microsoft/snocat#2

Merged

Dessix pushed a commit to microsoft/snocat that referenced this pull request Oct 10, 2022

avoid incoming breakage of for<'a> &'a T: 'a bounds

2d35c74

context: rust-lang/rust#98109

aliemjay mentioned this pull request Mar 2, 2023

"approximate check" for outlives relations can miss matches #54572

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix universes in the NLL type tests #98109

fix universes in the NLL type tests #98109

nikomatsakis commented Jun 14, 2022 •

edited by jackh726

Loading

compiler-errors commented Jun 14, 2022

bors commented Jun 14, 2022

compiler-errors commented Jun 14, 2022

compiler-errors Jun 14, 2022

nikomatsakis Jun 15, 2022

nikomatsakis Jun 15, 2022

This comment has been minimized.

bors commented Jun 14, 2022

This comment has been minimized.

This comment has been minimized.

This comment was marked as outdated.

jackh726 Jun 15, 2022

nikomatsakis Jun 15, 2022

compiler-errors Jun 16, 2022

nikomatsakis Jun 23, 2022

This comment has been minimized.

This comment has been minimized.

jackh726 commented Jun 24, 2022

bors commented Jun 24, 2022

bors commented Jun 24, 2022

bors commented Jun 24, 2022

rust-timer commented Jun 24, 2022

oli-obk commented Jun 24, 2022

rylev commented Jun 28, 2022

oli-obk commented Jun 28, 2022

rylev commented Jun 28, 2022

fix universes in the NLL type tests #98109

fix universes in the NLL type tests #98109

Conversation

nikomatsakis commented Jun 14, 2022 • edited by jackh726 Loading

compiler-errors commented Jun 14, 2022

bors commented Jun 14, 2022

compiler-errors commented Jun 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Jun 14, 2022

This comment has been minimized.

This comment has been minimized.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jackh726 commented Jun 24, 2022

bors commented Jun 24, 2022

bors commented Jun 24, 2022

bors commented Jun 24, 2022

rust-timer commented Jun 24, 2022

Footnotes

oli-obk commented Jun 24, 2022

rylev commented Jun 28, 2022

oli-obk commented Jun 28, 2022

rylev commented Jun 28, 2022

nikomatsakis commented Jun 14, 2022 •

edited by jackh726

Loading