compare_impl_item: remove trivial bounds

[lcnr]

fixes #108544, alternative to #109356

whenever we instantiate a param_env with non-identity substs it's very easy to end up with trivial bounds which can cause the trait solver to break :<

still needs comments n stuff but I wanted to get this PR up today.

r? types cc @compiler-errors @jackh726

[compiler-errors]

@bors try @rust-timer queue

we may need to add some short-circuiting in the trivial pred fn

[bors]

⌛ Trying commit 726eb29ca9eed5bf124f9f92c72abe4a48ffa4cc with merge a370a8bbe1b1266b2436ce1b9a40099992c8b8e5...

[lcnr]

wow, i hate the old trait solver. why do we get overflow here :<

[bors]

💔 Test failed - checks-actions

[compiler-errors]

why do we get overflow here :<

This actually happens a lot when evaluating method bounds in empty param-envs for some reason 😅 I've seen it in other PRs that attempt to filter thru unused bounds (e.g. in rustdoc)

[rustbot]

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

[lcnr]

why do we get overflow here :<

This actually happens a lot when evaluating method bounds in empty param-envs for some reason sweat_smile I've seen it in other PRs that attempt to filter thru unused bounds (e.g. in rustdoc)

that's annoying. solving it by using TraitQueryMode::Canonical 😅 can probably do the same in rustdoc. While we should remain careful with where we're using it, it seems unproblematic here.

[lcnr]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 2538c7aac8e66b90bb8ce918048e4a47d32128ec with merge c5a9b52fc828502976cf29d7dca61a663cd88100...

[lcnr]

using the empty environment here is suspect because the predicate is probably not well formed, but that shouldn't cause issues apart from considering fewer predicates to be trivial.

i guess we could keep all T: Trait bounds or something for this check, but it feels good enough for now. we can also add the implied outlives bounds for the predicate to be well formed to the environment, but 🤷 can't motivate myself to actually write a test where that's relevant

[bors]

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

[bors]

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

[rust-timer]

Finished benchmarking commit (c5a9b52fc828502976cf29d7dca61a663cd88100): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	21.0%	[0.6%, 52.0%]	8
Regressions ❌ (secondary)	0.3%	[0.3%, 0.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.3%, -0.9%]	6
All ❌✅ (primary)	21.0%	[0.6%, 52.0%]	8

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)	2.4%	[2.0%, 3.0%]	6
Regressions ❌ (secondary)	1.6%	[0.9%, 2.1%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-1.6%, -0.5%]	4
All ❌✅ (primary)	2.4%	[2.0%, 3.0%]	6

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)	13.5%	[9.6%, 27.3%]	7
Regressions ❌ (secondary)	0.7%	[0.5%, 1.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-1.2%, -0.4%]	3
All ❌✅ (primary)	13.5%	[9.6%, 27.3%]	7

[lcnr]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 901c81695fb451b3113a79a6f12ea70ff5b0b1bf with merge 14ac41b09e3bb31661451c52b5ed0b7994fcb604...

[bors]

☀️ Try build successful - checks-actions
Build commit: 14ac41b09e3bb31661451c52b5ed0b7994fcb604 (14ac41b09e3bb31661451c52b5ed0b7994fcb604)

[rust-timer]

Finished benchmarking commit (14ac41b09e3bb31661451c52b5ed0b7994fcb604): comparison URL.

Overall result: ✅ improvements - 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
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-2.1%, -0.4%]	15
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-2.1%, -0.4%]	15

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.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

[compiler-errors]

r=me, fcp needed per discussion

[jackh726]

Where was it mentioned that this needs FCP? I'm not sure that it does, given that this should just be a bug fix for the lint.

Though, I don't want to land this until we've resolved the discussion in #109356 (#109356 (comment)).

[compiler-errors]

@jackh726: lcnr said that they wanted to do an FCP on this (though I may be misremembering our convo earlier today).

[lcnr]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 97c5793 with merge 7262b3ee56b5293bbc413e007d1365cea78b5fa0...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:8f4b7f84864484a7bf31766abe9204da3cbe65b3)
Download action repository 'rust-lang/simpleinfra@master' (SHA:976c0fd7f953d1b34bea4ff6f03b3baa6d7a705f)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180119 sha256=9fa76c45f3193f307e02ea67d1a48cfe7ef2b854a074b766938a88e046bc7887
  Stored in directory: /tmp/pip-ephem-wheel-cache-kc70tj1l/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully built c32c7be5f65e
Successfully tagged rust-ci:latest
Built container sha256:c32c7be5f65e08e62d86bc68993c1a28222c218d679c974464742538d710ed69
Uploading finished image to https://ci-caches.rust-lang.org/docker/5a83b1174027be8472df586f36f1b4e77c39a20882cc84831c17b85c3cc5731b4e272c71c37dd18e09cacea39bf89c1b3226a304723983ec1270656c20627ec9
upload failed: - to s3://rust-lang-ci-sccache2/docker/5a83b1174027be8472df586f36f1b4e77c39a20882cc84831c17b85c3cc5731b4e272c71c37dd18e09cacea39bf89c1b3226a304723983ec1270656c20627ec9 Unable to locate credentials
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
    Finished release [optimized] target(s) in 13.21s
fmt check
tidy check
tidy: Skipping binary file check, read-only filesystem
##[error]tidy error: /checkout/compiler/rustc_trait_selection/src/traits/engine.rs:30: 6-line comment block with odd number of backticks
tidy error: /checkout/tests/ui/implied-bounds/issue-108544-1.rs: leading newline
some tidy checks failed

[rust-log-analyzer]

The job dist-x86_64-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] rustc_fs_util test:false 0.132
error[E0276]: impl has stricter requirements than trait
   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/lib.rs:471:12
    |
471 |           F: FnMut(T) -> U,
    |              ^^^^^^^^^^^^^ impl has extra requirement `F: FnMut<(T,)>`
   ::: /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/functional.rs:48:5
    |
    |
48  | /     fn map<U, F>(self, f: F) -> MappedSequence<Self, T, U>
49  | |     where
50  | |         Self: MappedGenericSequence<T, U>,
51  | |         Self::Length: ArrayLength<U>,
52  | |         F: FnMut(Self::Item) -> U,
    | |__________________________________- definition of `map` from trait
error[E0276]: impl has stricter requirements than trait
   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/lib.rs:471:24
    |
471 |           F: FnMut(T) -> U,
471 |           F: FnMut(T) -> U,
    |                          ^ impl has extra requirement `F: FnOnce<(T,)>`
   ::: /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/functional.rs:48:5
    |
    |
48  | /     fn map<U, F>(self, f: F) -> MappedSequence<Self, T, U>
49  | |     where
50  | |         Self: MappedGenericSequence<T, U>,
51  | |         Self::Length: ArrayLength<U>,
52  | |         F: FnMut(Self::Item) -> U,
    | |__________________________________- definition of `map` from trait
error[E0276]: impl has stricter requirements than trait
   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/lib.rs:495:12
    |
    |
495 |           F: FnMut(T, Rhs::Item) -> U,
    |              ^^^^^^^^^^^^^^^^^^^^^^^^ impl has extra requirement `F: FnMut<(T, <Rhs as IntoIterator>::Item)>`
   ::: /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/functional.rs:63:5
    |
    |
63  | /     fn zip<B, Rhs, U, F>(self, rhs: Rhs, f: F) -> MappedSequence<Self, T, U>
64  | |     where
65  | |         Self: MappedGenericSequence<T, U>,
66  | |         Rhs: MappedGenericSequence<B, U, Mapped = MappedSequence<Self, T, U>>,
67  | |         Self::Length: ArrayLength<B> + ArrayLength<U>,
68  | |         Rhs: GenericSequence<B, Length = Self::Length>,
69  | |         F: FnMut(Self::Item, Rhs::Item) -> U,
    | |_____________________________________________- definition of `zip` from trait
error[E0276]: impl has stricter requirements than trait
   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/lib.rs:495:35
    |
    |
495 |           F: FnMut(T, Rhs::Item) -> U,
    |                                     ^ impl has extra requirement `F: FnOnce<(T, <Rhs as IntoIterator>::Item)>`
   ::: /cargo/registry/src/index.crates.io-6f17d22bba15001f/generic-array-0.14.4/src/functional.rs:63:5
    |
    |
63  | /     fn zip<B, Rhs, U, F>(self, rhs: Rhs, f: F) -> MappedSequence<Self, T, U>
64  | |     where
65  | |         Self: MappedGenericSequence<T, U>,
66  | |         Rhs: MappedGenericSequence<B, U, Mapped = MappedSequence<Self, T, U>>,
67  | |         Self::Length: ArrayLength<B> + ArrayLength<U>,
68  | |         Rhs: GenericSequence<B, Length = Self::Length>,
69  | |         F: FnMut(Self::Item, Rhs::Item) -> U,
    | |_____________________________________________- definition of `zip` from trait
[RUSTC-TIMING] termize test:false 0.106
   Compiling lazy_static v1.4.0
[RUSTC-TIMING] lazy_static test:false 0.094
   Compiling rand v0.8.5
---
Total duration:                           11m 6s
------------------------------------------------
root INFO: Free disk space: 506.64 GiB out of total 581.32 GiB (12.84% used)
Traceback (most recent call last):
  File "../src/ci/stage-build.py", line 839, in <module>
    raise e
  File "../src/ci/stage-build.py", line 836, in <module>
    execute_build_pipeline(timer, pipeline, build_args)
  File "../src/ci/stage-build.py", line 760, in execute_build_pipeline
    LLVM_PROFILE_DIR=str(pipeline.llvm_profile_dir_root() / "prof-%p")
  File "../src/ci/stage-build.py", line 571, in build_rustc
    cmd(arguments, env=env)
  File "../src/ci/stage-build.py", line 452, in cmd
    return subprocess.run(args, env=environment, check=True)
  File "/usr/lib64/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['/usr/bin/python3', '/checkout/x.py', 'build', '--target', 'x86_64-unknown-linux-gnu', '--host', 'x86_64-unknown-linux-gnu', '--stage', '2', 'library/std', '--llvm-profile-generate']' returned non-zero exit status 1.

[lcnr]

alright, so by extending this to generic types we somehow end up in a case where:

• proving a goal succeeds using the filtered environment
• but we rely on that goal to prove something later

This issue might be because of a Projection goal as we can prove it even when failing to normalize https://rust-lang.zulipchat.com/#narrow/stream/362009-t-types.2Fetc.2Flazy-norm-strategems/topic/.60Projection.60.20assumption.20vs.20requirement/near/349055391.

I am not completely sure what's going on here and am now convinced that this is too brittle to land, especially with the old solver. Think we should go with the "only prove outlives" approach for now.