where_clauses_object_safety regression on a trait where it shouldn't trigger

[dtolnay]

#![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_use warning for Box::from_raw #104024 — DIAGNOSTIC TEXT ONLY
• Fix backoff doc to match implementation #104708 — COMMENT ONLY
• Fix core::any docs #106184 — DOCS ONLY
• Remove some totally duplicated files in rustc_infer #106205 — DEAD CODE

Highly unlikely:

• Remove iter::Empty hack #103945 — INTERNALS OF CORE::ITER::EMPTY
• Account for match expr in single line #105347 — MINOR DIAGNOSTIC FORMATTING TWEAK
• Emit fewer errors on invalid #[repr(transparent)] on enum #106201 — REPR(TRANSPARENT)-RELATED DIAGNOSTICS

I guess that leaves:

• Implement allow-by-default multiple_supertrait_upcastable lint #105484

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.

[dtolnay]

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]

I think the way where_clause_object_safety currently 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. having is_object_safe return yes/no/no-but-we-accidentally-allowed-it.

Is the issue serious enough for a revert? I think a forward fix is better.

[dtolnay]

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 with dyn. 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]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

[Noratrieb]

Removing P-critical as the revert has landed in #106248. Should we reprioritize this?

[apiraino]

@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 stay P-critical for 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-infra can decide for a stable backport (i.e. a point release).

@rustbot label P-critical

[pnkfelix]

Discussed in triage meeting. Downgrading (we do not need to revisit every week)

@rustbot label: +P-high -P-critical

[pnkfelix]

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)