stop special-casing `'static` in evaluation by lcnr · Pull Request #102472 · rust-lang/rust

lcnr · 2022-09-29T12:46:28Z

I have no idea whether this actually removed all places where 'static matters. Without canonicalization it's very easy to accidentally rely on 'static again. Blocked on changing the order_dependent_trait_objects future-compat lint to a hard error

r? @nikomatsakis

…, r=jackh726 make `order_dependent_trait_objects` show up in future-breakage reports tried to change it to a hard error in rust-lang#102474 but breaking the more than 1000 dependents of `traitobject` doesn't feel great 😅 This lint has existed since more than 3 years now and the way this is currently implemented is buggy and will break with rust-lang#102472. imo we should upgrade it to also report for dependencies and maybe also backport this to beta. Then after maybe 2-3 stable versions I would like to finally convert this lint to a hard error.

bors · 2022-10-22T03:09:46Z

☔ The latest upstream changes (presumably #103375) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-11-23T10:37:35Z

☔ The latest upstream changes (presumably #104758) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2022-12-15T15:44:08Z

@rustbot label: -T-compiler +T-types

pnkfelix · 2022-12-15T15:44:19Z

@rustbot assign @jackh726

(whoops I guess that should have been a r?)

jackh726 · 2023-01-21T17:52:07Z

@lcnr what's the status of this?

jackh726 · 2023-03-08T14:54:26Z

Given this only changes unstable marker trait behavior, fine to land with open issue referenced in known bug

lcnr · 2023-03-22T10:39:04Z

opened #109481 for overlapping-impl-1-modulo-regions.rs and we already have #89515 for the other test.

r? @jackh726

@rustbot ready

rustbot · 2023-03-22T10:39:05Z

Could not assign reviewer from: jackh726.
User(s) jackh726 are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

jackh726 · 2023-03-28T02:18:13Z

@bors r+

bors · 2023-03-28T02:18:16Z

📌 Commit 73c79cd has been approved by jackh726

It is now in the queue for this repository.

Rollup of 8 pull requests Successful merges: - rust-lang#91793 (socket ancillary data implementation for FreeBSD (from 13 and above).) - rust-lang#92284 (Change advance(_back)_by to return the remainder instead of the number of processed elements) - rust-lang#102472 (stop special-casing `'static` in evaluation) - rust-lang#108480 (Use Rayon's TLV directly) - rust-lang#109321 (Erase impl regions when checking for impossible to eagerly monomorphize items) - rust-lang#109470 (Correctly substitute GAT's type used in `normalize_param_env` in `check_type_bounds`) - rust-lang#109562 (Update ar_archive_writer to 0.1.3) - rust-lang#109629 (remove obsolete `givens` from regionck) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

lqd · 2023-03-31T06:32:41Z

For triage purposes: from the results in the rollup where this PR landed, and the results in a revert.

@rustbot label: +perf-regression

The `TypeOutlives` handler in `evaluate_predicate_recursively` checks whether a type in a `T: 'a` predicate has free regions, bound regions, or inference variables -- and if so, returns `EvaluatedToOkModuloRegions` rather than `EvaluatedToOk`. The comment says "no free lifetimes or generic parameters", but the code checked `has_non_region_infer()` twice instead of checking `has_non_region_param()` for the second condition. This meant that `TypeOutlives(T, 'static)` where `T` is a type parameter returned `EvaluatedToOk` -- claiming the result holds unconditionally -- when the correct answer is `EvaluatedToOkModuloRegions`. The distinction matters during marker trait winnowing in `prefer_lhs_over_victim`, which uses `must_apply_considering_regions()` (true only for `EvaluatedToOk`) to decide whether one overlapping impl beats another. With the bug, a `T: 'static`-bounded impl appeared equally strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one. Fixesrust-lang#109481. This same symptom was originally filed as rust-lang#84917 and fixed in PR rust-lang#88139. Then PR rust-lang#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Shortly after, rust-lang#109481 was filed. It noted the connection to rust-lang#102472 -- that it was only relevant after it -- but apparently no one noticed the duplicate condition.

@lcnr

…ssing-param-check, r=lcnr Fix marker trait winnowing depending on impl order Story: I was looking through this code for an unrelated reason and happened to notice the duplicate condition. That seemed obviously wrong, especially in light of the comment, so I worked backward to what it affected, wrote a test that failed, and then found that the test matched an existing `known-bug` test, which pointed me to rust-lang#109481. --- The `TypeOutlives` handler in `evaluate_predicate_recursively` means to check whether a type in a `T: 'a` predicate has free regions, bound regions, non-region inference variables, or non-region generic parameters -- and if so, to return `EvaluatedToOkModuloRegions` rather than `EvaluatedToOk`. Correspondingly, the comment says "no free lifetimes or generic parameters". But the code was mistakenly checking `has_non_region_infer()` twice instead of checking both `has_non_region_infer()` and `has_non_region_param()`. This meant that `TypeOutlives(T, 'static)` where `T` is a type parameter returned `EvaluatedToOk` -- claiming the result holds unconditionally -- when the correct answer is `EvaluatedToOkModuloRegions`. The distinction matters during marker trait winnowing in `prefer_lhs_over_victim`, which uses `must_apply_considering_regions()` (true only for `EvaluatedToOk`) to decide whether one impl beats another when there are multiple candidates. With the bug, a `T: 'static`-bounded impl appeared equally as strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one. Fixesrust-lang#109481. This same symptom was originally filed as rust-lang#84917 and fixed in PR rust-lang#88139. Then PR rust-lang#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, rust-lang#109481 was filed. It noted the connection to rust-lang#102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

@lcnr

…ssing-param-check, r=lcnr Fix marker trait winnowing depending on impl order Story: I was looking through this code for an unrelated reason and happened to notice the duplicate condition. That seemed obviously wrong, especially in light of the comment, so I worked backward to what it affected, wrote a test that failed, and then found that the test matched an existing `known-bug` test, which pointed me to rust-lang#109481. --- The `TypeOutlives` handler in `evaluate_predicate_recursively` means to check whether a type in a `T: 'a` predicate has free regions, bound regions, non-region inference variables, or non-region generic parameters -- and if so, to return `EvaluatedToOkModuloRegions` rather than `EvaluatedToOk`. Correspondingly, the comment says "no free lifetimes or generic parameters". But the code was mistakenly checking `has_non_region_infer()` twice instead of checking both `has_non_region_infer()` and `has_non_region_param()`. This meant that `TypeOutlives(T, 'static)` where `T` is a type parameter returned `EvaluatedToOk` -- claiming the result holds unconditionally -- when the correct answer is `EvaluatedToOkModuloRegions`. The distinction matters during marker trait winnowing in `prefer_lhs_over_victim`, which uses `must_apply_considering_regions()` (true only for `EvaluatedToOk`) to decide whether one impl beats another when there are multiple candidates. With the bug, a `T: 'static`-bounded impl appeared equally as strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one. Fixesrust-lang#109481. This same symptom was originally filed as rust-lang#84917 and fixed in PR rust-lang#88139. Then PR rust-lang#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, rust-lang#109481 was filed. It noted the connection to rust-lang#102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

@lcnr

…ssing-param-check, r=lcnr Fix marker trait winnowing depending on impl order Story: I was looking through this code for an unrelated reason and happened to notice the duplicate condition. That seemed obviously wrong, especially in light of the comment, so I worked backward to what it affected, wrote a test that failed, and then found that the test matched an existing `known-bug` test, which pointed me to rust-lang#109481. --- The `TypeOutlives` handler in `evaluate_predicate_recursively` means to check whether a type in a `T: 'a` predicate has free regions, bound regions, non-region inference variables, or non-region generic parameters -- and if so, to return `EvaluatedToOkModuloRegions` rather than `EvaluatedToOk`. Correspondingly, the comment says "no free lifetimes or generic parameters". But the code was mistakenly checking `has_non_region_infer()` twice instead of checking both `has_non_region_infer()` and `has_non_region_param()`. This meant that `TypeOutlives(T, 'static)` where `T` is a type parameter returned `EvaluatedToOk` -- claiming the result holds unconditionally -- when the correct answer is `EvaluatedToOkModuloRegions`. The distinction matters during marker trait winnowing in `prefer_lhs_over_victim`, which uses `must_apply_considering_regions()` (true only for `EvaluatedToOk`) to decide whether one impl beats another when there are multiple candidates. With the bug, a `T: 'static`-bounded impl appeared equally as strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one. Fixesrust-lang#109481. This same symptom was originally filed as rust-lang#84917 and fixed in PR rust-lang#88139. Then PR rust-lang#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, rust-lang#109481 was filed. It noted the connection to rust-lang#102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

@lcnr

Rollup merge of #153847 - traviscross:TC/fix-typeoutlives-missing-param-check, r=lcnr Fix marker trait winnowing depending on impl order Story: I was looking through this code for an unrelated reason and happened to notice the duplicate condition. That seemed obviously wrong, especially in light of the comment, so I worked backward to what it affected, wrote a test that failed, and then found that the test matched an existing `known-bug` test, which pointed me to #109481. --- The `TypeOutlives` handler in `evaluate_predicate_recursively` means to check whether a type in a `T: 'a` predicate has free regions, bound regions, non-region inference variables, or non-region generic parameters -- and if so, to return `EvaluatedToOkModuloRegions` rather than `EvaluatedToOk`. Correspondingly, the comment says "no free lifetimes or generic parameters". But the code was mistakenly checking `has_non_region_infer()` twice instead of checking both `has_non_region_infer()` and `has_non_region_param()`. This meant that `TypeOutlives(T, 'static)` where `T` is a type parameter returned `EvaluatedToOk` -- claiming the result holds unconditionally -- when the correct answer is `EvaluatedToOkModuloRegions`. The distinction matters during marker trait winnowing in `prefer_lhs_over_victim`, which uses `must_apply_considering_regions()` (true only for `EvaluatedToOk`) to decide whether one impl beats another when there are multiple candidates. With the bug, a `T: 'static`-bounded impl appeared equally as strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one. Fixes#109481. This same symptom was originally filed as #84917 and fixed in PR #88139. Then PR #102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, #109481 was filed. It noted the connection to #102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

@lcnr

…am-check, r=lcnr Fix marker trait winnowing depending on impl order Story: I was looking through this code for an unrelated reason and happened to notice the duplicate condition. That seemed obviously wrong, especially in light of the comment, so I worked backward to what it affected, wrote a test that failed, and then found that the test matched an existing `known-bug` test, which pointed me to rust-lang/rust#109481. --- The `TypeOutlives` handler in `evaluate_predicate_recursively` means to check whether a type in a `T: 'a` predicate has free regions, bound regions, non-region inference variables, or non-region generic parameters -- and if so, to return `EvaluatedToOkModuloRegions` rather than `EvaluatedToOk`. Correspondingly, the comment says "no free lifetimes or generic parameters". But the code was mistakenly checking `has_non_region_infer()` twice instead of checking both `has_non_region_infer()` and `has_non_region_param()`. This meant that `TypeOutlives(T, 'static)` where `T` is a type parameter returned `EvaluatedToOk` -- claiming the result holds unconditionally -- when the correct answer is `EvaluatedToOkModuloRegions`. The distinction matters during marker trait winnowing in `prefer_lhs_over_victim`, which uses `must_apply_considering_regions()` (true only for `EvaluatedToOk`) to decide whether one impl beats another when there are multiple candidates. With the bug, a `T: 'static`-bounded impl appeared equally as strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one. Fixesrust-lang/rust#109481. This same symptom was originally filed as rust-lang/rust#84917 and fixed in PR rust-lang/rust#88139. Then PR rust-lang/rust#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, rust-lang/rust#109481 was filed. It noted the connection to rust-lang/rust#102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

@lcnr

…am-check, r=lcnr Fix marker trait winnowing depending on impl order Story: I was looking through this code for an unrelated reason and happened to notice the duplicate condition. That seemed obviously wrong, especially in light of the comment, so I worked backward to what it affected, wrote a test that failed, and then found that the test matched an existing `known-bug` test, which pointed me to rust-lang/rust#109481. --- The `TypeOutlives` handler in `evaluate_predicate_recursively` means to check whether a type in a `T: 'a` predicate has free regions, bound regions, non-region inference variables, or non-region generic parameters -- and if so, to return `EvaluatedToOkModuloRegions` rather than `EvaluatedToOk`. Correspondingly, the comment says "no free lifetimes or generic parameters". But the code was mistakenly checking `has_non_region_infer()` twice instead of checking both `has_non_region_infer()` and `has_non_region_param()`. This meant that `TypeOutlives(T, 'static)` where `T` is a type parameter returned `EvaluatedToOk` -- claiming the result holds unconditionally -- when the correct answer is `EvaluatedToOkModuloRegions`. The distinction matters during marker trait winnowing in `prefer_lhs_over_victim`, which uses `must_apply_considering_regions()` (true only for `EvaluatedToOk`) to decide whether one impl beats another when there are multiple candidates. With the bug, a `T: 'static`-bounded impl appeared equally as strong as an unrestricted impl, making the winner depend on source order. This caused spurious E0310 errors when the more-constrained impl happened to appear after the less-constrained one. Fixesrust-lang/rust#109481. This same symptom was originally filed as rust-lang/rust#84917 and fixed in PR rust-lang/rust#88139. Then PR rust-lang/rust#102472 rewrote the `TypeOutlives` handler, introducing the duplicate `has_non_region_infer()` and losing the param check, regressing this. Around this same time, rust-lang/rust#109481 was filed. It noted the connection to rust-lang/rust#102472 -- the bug only appeared after it -- but the duplicate condition was not noticed. r? @lcnr cc @oli-obk @nikomatsakis

lcnr added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 29, 2022

rust-highfive assigned nikomatsakis Sep 29, 2022

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

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

lcnr mentioned this pull request Sep 29, 2022
make incoherent_auto_trait_objects a hard error #102474
Closed

This comment has been minimized.
Sign in to view

This was referenced Oct 3, 2022
make order_dependent_trait_objects show up in future-breakage reports #102635
Merged
remove fresh inference vars #102205
Closed
evaluation cache, freshen each predicate by itself #102713
Closed

lcnr force-pushed the static-in-eval branch from bf9b5bb to bde9718Compare November 8, 2022 14:22

lcnr marked this pull request as ready for review November 8, 2022 14:24

rustbot 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 Dec 15, 2022

rustbot assigned jackh726 and unassigned nikomatsakis Dec 15, 2022

jackh726 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2023

stop special-casing 'static in evaluate
73c79cd

lcnr force-pushed the static-in-eval branch from bde9718 to 73c79cdCompare March 22, 2023 10:38

rustbot removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Mar 22, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2023

lcnr mentioned this pull request Mar 22, 2023
marker traits, cannot prefer impl with no bounds #109481
Closed

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 Mar 28, 2023

Noratrieb mentioned this pull request Mar 28, 2023
Rollup of 8 pull requests #109692
Merged

bors merged commit 4bd33fd into rust-lang:masterMar 28, 2023

rustbot added this to the 1.70.0 milestone Mar 28, 2023

lcnr deleted the static-in-eval branch March 28, 2023 19:35

lqd mentioned this pull request Mar 30, 2023
[perf] Try revert of #102472 #109780
Closed

rustbot added the perf-regression Performance regression. label Mar 31, 2023

Noratrieb mentioned this pull request Jun 20, 2023
ICE: assertion failed: !ty.needs_infer() #112832
Closed

lcnr mentioned this pull request Dec 13, 2023
cache param env canonicalization #117749
Merged

lcnr mentioned this pull request Jan 8, 2024
unify query canonicalization mode #118968
Merged

traviscross mentioned this pull request Mar 14, 2026
Fix marker trait winnowing depending on impl order #153847
Merged

Navigation Menu

Uh oh!

Uh oh!

stop special-casing `'static` in evaluation - #102472

stop special-casing `'static` in evaluation#102472
bors merged 1 commit into
rust-lang:masterfrom
lcnr:static-in-eval

lcnr commented Sep 29, 2022•
edited
Loading

Uh oh!

This comment has been minimized.

bors commented Oct 22, 2022

Uh oh!

bors commented Nov 23, 2022

Uh oh!

pnkfelix commented Dec 15, 2022

Uh oh!

pnkfelix commented Dec 15, 2022•
edited
Loading

Uh oh!

jackh726 commented Jan 21, 2023

Uh oh!

jackh726 commented Mar 8, 2023

Uh oh!

lcnr commented Mar 22, 2023

Uh oh!

rustbot commented Mar 22, 2023

Uh oh!

jackh726 commented Mar 28, 2023

Uh oh!

bors commented Mar 28, 2023

Uh oh!

lqd commented Mar 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Uh oh!

Conversation

lcnr commented Sep 29, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Oct 22, 2022

Uh oh!

bors commented Nov 23, 2022

Uh oh!

pnkfelix commented Dec 15, 2022

Uh oh!

pnkfelix commented Dec 15, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackh726 commented Jan 21, 2023

Uh oh!

jackh726 commented Mar 8, 2023

Uh oh!

lcnr commented Mar 22, 2023

Uh oh!

rustbot commented Mar 22, 2023

Uh oh!

jackh726 commented Mar 28, 2023

Uh oh!

bors commented Mar 28, 2023

Uh oh!

lqd commented Mar 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lcnr commented Sep 29, 2022•
edited
Loading

pnkfelix commented Dec 15, 2022•
edited
Loading