lint incorrect implied bounds in wfcheck #109763

aliemjay · 2023-03-30T07:59:01Z

Lint version of #109482.

This implements a new implied bounds query that fixes the normalization problems with the previous query, including #106569, #109628 and #109799. While the new implementation allows more code to compile, it also breaks code that relies on #109628 (see the crater run in #109482 for details.) For this reason we keep the legacy implementation and emit a lint when they disagree.

Fixes #109628.

~~This doesn't implement the lint yet. I want to have a perf baseline first.~~
r? @ghost

aliemjay · 2023-03-30T12:40:58Z

@bors try @rust-timer queue

bors · 2023-03-30T12:41:09Z

⌛ Trying commit fb49efa4c195a669c1d518c33e78b3adf9eea34b with merge 67ce1e9f41cb4c34e12f6d249826cce3f073c6c7...

bors · 2023-03-30T12:54:23Z

💔 Test failed - checks-actions

aliemjay · 2023-03-30T13:12:20Z

@bors try

bors · 2023-03-30T13:12:31Z

⌛ Trying commit 364683472a016eb083e00c781635025f54044e49 with merge 9df496919a3bfe6478f619535af0fa752b6d84b5...

bors · 2023-03-30T15:12:44Z

☀️ Try build successful - checks-actions
Build commit: 9df496919a3bfe6478f619535af0fa752b6d84b5 (9df496919a3bfe6478f619535af0fa752b6d84b5)

rust-timer · 2023-03-30T16:46:38Z

Finished benchmarking commit (9df496919a3bfe6478f619535af0fa752b6d84b5): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.5%	[3.2%, 3.8%]	2
Regressions ❌ (secondary)	3.0%	[1.7%, 3.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	3.5%	[3.2%, 3.8%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	1.4%	[1.4%, 1.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.9%	[2.9%, 2.9%]	1

rustbot · 2023-03-31T14:22:07Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

bors · 2023-05-06T23:13:27Z

☀️ Try build successful - checks-actions
Build commit: 7ff4f56db6ea2b678c8cccb37dd6b260df25763c (7ff4f56db6ea2b678c8cccb37dd6b260df25763c)

rust-timer · 2023-05-07T00:29:30Z

Finished benchmarking commit (7ff4f56db6ea2b678c8cccb37dd6b260df25763c): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.4%]	2
Regressions ❌ (secondary)	0.4%	[0.2%, 0.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.4%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.5%	[3.6%, 5.3%]	4
Regressions ❌ (secondary)	3.1%	[2.9%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	2
All ❌✅ (primary)	4.5%	[3.6%, 5.3%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.3%	[0.8%, 2.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[0.8%, 2.0%]	3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 653.481s -> 654.306s (0.13%)

jackh726

Sorry for the long review latency here.

Just a few smaller thoughts, before digging into the logic.

jackh726 · 2023-08-13T01:36:38Z

compiler/rustc_traits/src/implied_outlives_bounds.rs

-            }
-        })
-        .collect()
+) -> impl Iterator<Item = OutlivesBound<'tcx>> {


Can we move this cleanup into a separate commit or PR?

jackh726 · 2023-08-13T01:38:01Z

compiler/rustc_trait_selection/src/traits/wf.rs

    param_env: ty::ParamEnv<'tcx>,
    arg: GenericArg<'tcx>,
-) -> Option<Vec<traits::PredicateObligation<'tcx>>> {
+) -> Vec<traits::PredicateObligation<'tcx>> {


This too can be moved into a separate commit or PR?

jackh726 · 2023-08-13T01:39:07Z

compiler/rustc_middle/src/query/mod.rs

@@ -1828,7 +1828,7 @@ rustc_queries! {
        desc { "normalizing `{}`", goal.value }
    }

-    query implied_outlives_bounds(
+    query implied_outlives_bounds_compat(


Given that it seems like most places still use this, it seems like it would make sense to keep this name the same, and name the new version v2 or something.

jackh726

Finally got through this, changes look good to me. I'm expecting the rebase might require retouching some of this code significantly. If so, I can review. Otherwise, r=me

lcnr · 2023-08-14T07:41:12Z

Before merging, I would like us to look more deeply into #109482 (comment).

rn this lints triggers for bevy with afaict no sensible way to silence the lint. Given that we may be able to avoid it by a sensible and small extension to implied bounds, we may be able to avoid this.

jackh726 · 2023-08-14T12:24:17Z

I think the extension to implied bounds is certainly possible. But I think a sensible extension won't be small and small extension won't be simple.

The sensible extension is to effectively allow types to have implied bounds from trait impls. The simple extension is to change nothing and continue to let this compile.

lcnr · 2023-09-18T14:05:07Z

blocked on rust-lang/types-team#99, unnominating

spastorino · 2023-12-01T13:45:59Z

compiler/rustc_trait_selection/src/traits/outlives_bounds.rs

+                let ty = OpportunisticRegionResolver::new(self).fold_ty(ty);
+                if ty.has_infer() {
+                    // Infer vars can appear only in invalid code. See #110161.
+                    self.tcx.sess.delay_span_bug(DUMMY_SP, "infer vars in implied bounds");


For the record, this line should be removed when/if this is rebased #115559

spastorino · 2023-12-01T21:19:05Z

In case it's needed I was playing with this PR and "rebased" it in https://github.com/spastorino/rust/tree/lint-implied-bounds

lint incorrect implied bounds in wfcheck except for Bevy dependents Rebase of rust-lang#109763 Additionally, special cases Bevy `ParamSet` types to not trigger the lint. Opening for crater r? `@ghost`

…ted-tests, r=jackh726 Add tests related to normalization in implied bounds Getting `@aliemjay's` tests from rust-lang#109763, so we can better track what's going on in every different example. r? `@jackh726`

…ted-tests, r=jackh726 Add tests related to normalization in implied bounds Getting ``@aliemjay's`` tests from rust-lang#109763, so we can better track what's going on in every different example. r? ``@jackh726``

…ted-tests, r=jackh726 Add tests related to normalization in implied bounds Getting ```@aliemjay's``` tests from rust-lang#109763, so we can better track what's going on in every different example. r? ```@jackh726```

Rollup merge of rust-lang#118512 - spastorino:add-implied-bounds-related-tests, r=jackh726 Add tests related to normalization in implied bounds Getting ```@aliemjay's``` tests from rust-lang#109763, so we can better track what's going on in every different example. r? ```@jackh726```

error on incorrect implied bounds in wfcheck except for Bevy dependents Rebase of rust-lang#109763 Additionally, special cases Bevy `ParamSet` types to not trigger the lint. This is tracked in rust-lang#119956. Fixes rust-lang#109628

jackh726 · 2024-02-05T06:03:22Z

Closing since #118553 landed.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 30, 2023

aliemjay force-pushed the lint-implied-bounds-fork branch 2 times, most recently from dcdb22e to fb49efa Compare March 30, 2023 12:39

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 30, 2023

bors 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 Mar 30, 2023

aliemjay force-pushed the lint-implied-bounds-fork branch from fb49efa to 3646834 Compare March 30, 2023 12:58

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 30, 2023

aliemjay force-pushed the lint-implied-bounds-fork branch from 3646834 to 91c7cd2 Compare March 31, 2023 14:12

aliemjay added the A-implied-bounds Area: Implied bounds / inferred outlives-bounds label Mar 31, 2023

aliemjay marked this pull request as ready for review March 31, 2023 14:22

aliemjay force-pushed the lint-implied-bounds-fork branch from 91c7cd2 to 13e0f74 Compare March 31, 2023 14:24

aliemjay added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 31, 2023

aliemjay mentioned this pull request May 6, 2023

interpret region vars as universals in implied bounds query #109388

Closed

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 7, 2023

jackh726 reviewed Aug 13, 2023

View reviewed changes

jackh726 approved these changes Aug 13, 2023

View reviewed changes

jackh726 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 Aug 13, 2023

jackh726 added the I-types-nominated Nominated for discussion during a types team meeting. label Aug 14, 2023

nikomatsakis mentioned this pull request Sep 11, 2023

discuss "incorrect implied bounds in wfcheck" rust-lang/types-team#99

Open

alice-i-cecile mentioned this pull request Sep 14, 2023

Lifetime bounds differ between WorldQuery and SystemParam, making rustc grumpy bevyengine/bevy#9808

Open

lcnr removed the I-types-nominated Nominated for discussion during a types team meeting. label Sep 18, 2023

spastorino reviewed Dec 1, 2023

View reviewed changes

spastorino mentioned this pull request Dec 1, 2023

Add tests related to normalization in implied bounds #118512

Merged

jackh726 mentioned this pull request Dec 2, 2023

error on incorrect implied bounds in wfcheck except for Bevy dependents #118553

Merged

jackh726 closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint incorrect implied bounds in wfcheck #109763

lint incorrect implied bounds in wfcheck #109763

aliemjay commented Mar 30, 2023 •

edited

Loading

aliemjay commented Mar 30, 2023 •

edited

Loading

This comment has been minimized.

bors commented Mar 30, 2023

bors commented Mar 30, 2023

This comment has been minimized.

aliemjay commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

This comment has been minimized.

This comment has been minimized.

rust-timer commented Mar 30, 2023

rustbot commented Mar 31, 2023

bors commented May 6, 2023

This comment has been minimized.

rust-timer commented May 7, 2023

jackh726 left a comment

jackh726 Aug 13, 2023

jackh726 Aug 13, 2023

jackh726 Aug 13, 2023

jackh726 left a comment

lcnr commented Aug 14, 2023

jackh726 commented Aug 14, 2023

lcnr commented Sep 18, 2023

spastorino Dec 1, 2023

spastorino commented Dec 1, 2023

jackh726 commented Feb 5, 2024

lint incorrect implied bounds in wfcheck #109763

lint incorrect implied bounds in wfcheck #109763

Conversation

aliemjay commented Mar 30, 2023 • edited Loading

aliemjay commented Mar 30, 2023 • edited Loading

This comment has been minimized.

bors commented Mar 30, 2023

bors commented Mar 30, 2023

This comment has been minimized.

aliemjay commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

This comment has been minimized.

This comment has been minimized.

rust-timer commented Mar 30, 2023

Overall result: ❌ regressions - no action needed

rustbot commented Mar 31, 2023

bors commented May 6, 2023

This comment has been minimized.

rust-timer commented May 7, 2023

Overall result: ❌ regressions - ACTION NEEDED

jackh726 left a comment

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

jackh726 left a comment

Choose a reason for hiding this comment

lcnr commented Aug 14, 2023

jackh726 commented Aug 14, 2023

lcnr commented Sep 18, 2023

Choose a reason for hiding this comment

spastorino commented Dec 1, 2023

jackh726 commented Feb 5, 2024

aliemjay commented Mar 30, 2023 •

edited

Loading

aliemjay commented Mar 30, 2023 •

edited

Loading