Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

compare_impl_item: remove trivial bounds#109491

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@lcnr
Copy link
Contributor

fixes#108544, alternative to#109356

whenever we instantiate aparam_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

@rustbotrustbot added S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. T-compilerRelevant to the compiler team, which will review and decide on the PR/issue. labelsMar 22, 2023
@lcnrlcnrforce-pushed thecheck-impl-trivial-bounds branch fromde13d74 to726eb29CompareMarch 22, 2023 15:27
@compiler-errors
Copy link
Member

@bors try@rust-timer queue

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

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perfStatus: Waiting on a perf run to be completed. labelMar 22, 2023
@bors
Copy link
Collaborator

⌛ Trying commit 726eb29ca9eed5bf124f9f92c72abe4a48ffa4cc with merge a370a8bbe1b1266b2436ce1b9a40099992c8b8e5...

@lcnr
Copy link
ContributorAuthor

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

@bors
Copy link
Collaborator

💔 Test failed -checks-actions

@borsbors added S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelsMar 22, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

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
Copy link
Collaborator

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

cc@lcnr,@compiler-errors

@lcnr
Copy link
ContributorAuthor

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 usingTraitQueryMode::Canonical 😅 can probably do the same in rustdoc. While we should remain careful with where we're using it, it seems unproblematic here.

@lcnrlcnrforce-pushed thecheck-impl-trivial-bounds branch from6410e68 to2538c7aCompareMarch 23, 2023 11:27
@lcnr
Copy link
ContributorAuthor

@bors try@rust-timer queue

@rust-timer

This comment has been minimized.

@bors
Copy link
Collaborator

⌛ Trying commit 2538c7aac8e66b90bb8ce918048e4a47d32128ec with merge c5a9b52fc828502976cf29d7dca61a663cd88100...

@lcnr
Copy link
ContributorAuthor

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 allT: 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
Copy link
Collaborator

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

@rust-timer

This comment has been minimized.

@bors
Copy link
Collaborator

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

@rust-timer
Copy link
Collaborator

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.

meanrangecount
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.

meanrangecount
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.

meanrangecount
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
aliemjay reacted with eyes emoji

@rustbotrustbot added the perf-regressionPerformance regression. labelMar 23, 2023
@rust-log-analyzer

This comment has been minimized.

@lcnrlcnrforce-pushed thecheck-impl-trivial-bounds branch from0e7240f to901c816CompareMarch 27, 2023 10:29
@lcnr
Copy link
ContributorAuthor

@bors try@rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perfStatus: Waiting on a perf run to be completed. labelMar 27, 2023
@bors
Copy link
Collaborator

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

@bors
Copy link
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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.

meanrangecount
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.

meanrangecount
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.

meanrangecount
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

@rustbotrustbot removed S-waiting-on-perfStatus: Waiting on a perf run to be completed. perf-regressionPerformance regression. labelsMar 27, 2023
Copy link
Member

@compiler-errorscompiler-errors left a comment

Choose a reason for hiding this comment

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

r=me, fcp needed per discussion

@compiler-errorscompiler-errors added the needs-fcpThis change is insta-stable, or significant enough to need a team FCP to proceed. labelMar 27, 2023
@jackh726
Copy link
Member

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
Copy link
Member

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

@lcnrlcnrforce-pushed thecheck-impl-trivial-bounds branch from901c816 to97c5793CompareApril 11, 2023 10:11
@lcnr
Copy link
ContributorAuthor

@bors try@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbotrustbot added the S-waiting-on-perfStatus: Waiting on a perf run to be completed. labelApr 11, 2023
@bors
Copy link
Collaborator

⌛ Trying commit97c5793 with merge 7262b3ee56b5293bbc413e007d1365cea78b5fa0...

@bors
Copy link
Collaborator

💔 Test failed -checks-actions

@rust-log-analyzer
Copy link
Collaborator

The jobmingw-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 actionsGetting action download infoDownload action repository 'actions/checkout@v3' (SHA:8f4b7f84864484a7bf31766abe9204da3cbe65b3)Download action repository 'rust-lang/simpleinfra@master' (SHA:976c0fd7f953d1b34bea4ff6f03b3baa6d7a705f)Complete job name: PR - mingw-check-tidygit config --global core.autocrlf falseshell: /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/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76dInstalling 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 c32c7be5f65eSuccessfully tagged rust-ci:latestBuilt container sha256:c32c7be5f65e08e62d86bc68993c1a28222c218d679c974464742538d710ed69Uploading finished image to https://ci-caches.rust-lang.org/docker/5a83b1174027be8472df586f36f1b4e77c39a20882cc84831c17b85c3cc5731b4e272c71c37dd18e09cacea39bf89c1b3226a304723983ec1270656c20627ec9upload 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.21sfmt checktidy checktidy: 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 backtickstidy error: /checkout/tests/ui/implied-bounds/issue-108544-1.rs: leading newlinesome tidy checks failed

@rust-log-analyzer
Copy link
Collaborator

The jobdist-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.132error[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  | |     where50  | |         Self: MappedGenericSequence<T, U>,51  | |         Self::Length: ArrayLength<U>,52  | |         F: FnMut(Self::Item) -> U,    | |__________________________________- definition of `map` from traiterror[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  | |     where50  | |         Self: MappedGenericSequence<T, U>,51  | |         Self::Length: ArrayLength<U>,52  | |         F: FnMut(Self::Item) -> U,    | |__________________________________- definition of `map` from traiterror[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  | |     where65  | |         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 traiterror[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  | |     where65  | |         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
Copy link
ContributorAuthor

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 aProjection goal as we can prove it even when failing to normalizehttps://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.

@lcnrlcnr closed thisApr 20, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@compiler-errorscompiler-errorscompiler-errors approved these changes

Assignees

@compiler-errorscompiler-errors

Labels

needs-fcpThis change is insta-stable, or significant enough to need a team FCP to proceed.S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.S-waiting-on-perfStatus: Waiting on a perf run to be completed.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

false positive implied_bounds_entailment lint

7 participants

@lcnr@compiler-errors@rust-timer@bors@rust-log-analyzer@rustbot@jackh726
[8]ページ先頭
©2009-2025 Movatter.jp