Repository navigation
where_clauses_object_safety regression on a trait where it shouldn't trigger #106247
Description
#![deny(where_clauses_object_safety)]
pub trait Trait {
fn method(&self) where Self: Sync;
}I bisected #51443 (comment). According to cargo-bisect-rustc --start 2022-12-28 --end 2022-12-29 -- check it bisects to 6a20f7d. This is concerning because nothing in #106209 looks remotely related to where_clause_object_safety so I am inferring that this is an unintentional regression.
Here are the 9 PRs in the rollup:
Definitely not it:
- Test leaking of BinaryHeap Drain iterators #94145 — TEST ONLY
- Fix
unused_must_usewarning forBox::from_raw#104024 — DIAGNOSTIC TEXT ONLY - Fix backoff doc to match implementation #104708 — COMMENT ONLY
- Fix
core::anydocs #106184 — DOCS ONLY - Remove some totally duplicated files in
rustc_infer#106205 — DEAD CODE
Highly unlikely:
- Remove
iter::Emptyhack #103945 — INTERNALS OF CORE::ITER::EMPTY - Account for
matchexpr in single line #105347 — MINOR DIAGNOSTIC FORMATTING TWEAK - Emit fewer errors on invalid
#[repr(transparent)]onenum#106201 — REPR(TRANSPARENT)-RELATED DIAGNOSTICS
I guess that leaves:
which only adds a brand new lint pass unrelated to where_clauses_object_safety, independent of any existing code. The PR is +167 -0. It seems like the only possible effect should be adding new occurrences of multiple_supertrait_upcastable, not affecting anything else.
In any case cc @nbdd0121 @compiler-errors in case you can tell what's going on.
Reacted by Agost Biro, William Woodruff and Andrew Pollack-Gray
Activity
dtolnay commentedon Dec 29, 2022
I confirmed that the regression no longer reproduces after reverting #105484.
I have put up a revert PR in #106248 since it really seems like @nbdd0121's PR did not intentionally have anything to do with where_clauses_object_safety.
nbdd0121 commentedon Dec 29, 2022
I think the way
where_clause_object_safetycurrently being implemented is problematic. It currently fires when the compiler is checking whether a trait is object-safe or not, which doesn't make sense. It should be checked at actual use site (i.e. havingis_object_safereturn yes/no/no-but-we-accidentally-allowed-it.Is the issue serious enough for a revert? I think a forward fix is better.
dtolnay commentedon Dec 29, 2022
Yes I think it's serious enough for a revert. If I understand your explanation correctly, the multiple_supertrait_upcastable check gets run on every trait, which means we're now reporting scary object safety breakage warnings on literally every trait with a where-clause that mentions
Self, for traits that no one ever though of using withdyn. That is a quite disruptive false positive.(Of course you're free to race to put up a forward fix before the revert gets approved.)
apiraino commentedon Dec 29, 2022
WG-prioritization assigning priority (Zulip discussion).
@rustbot label -I-prioritize +P-critical
Temporarily suppress where_clauses_object_safety due to regression
minimal-versions: pin nightly to 2022-12-25
minimal-versions: pin nightly to 2022-12-25 (#23)
Rollup merge of rust-lang#106248 - dtolnay:revertupcastlint, r=jackh726
Noratrieb commentedon Dec 31, 2022
Removing P-critical as the revert has landed in #106248. Should we reprioritize this?
apiraino commentedon Dec 31, 2022
@Nilstrieb I think
P-labels can stay attached to an issue. In fact they're just signaling the priority, not their open/solved status; therefore a closed issue can stayP-criticalfor good (and sometimes it's also useful to have a reference or a "previous case" to refer to in case similar issues are filed).In addition, it's also useful in case critical regressions are fixed on the stable channel so
T-infracan decide for a stable backport (i.e. a point release).@rustbot label P-critical
pnkfelix commentedon Jan 5, 2023
Discussed in triage meeting. Downgrading (we do not need to revisit every week)
@rustbot label: +P-high -P-critical
pnkfelix commentedon Jan 5, 2023
further discussion led us to realize that this should just be closed due to the revert
(a test landed, so we should not reinject the bug when the lint re-lands)
Auto merge of rust-lang#106253 - nbdd0121:upcast, r=compiler-errors
Rollup merge of #106248 - dtolnay:revertupcastlint, r=jackh726