Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid ICEs in trait names without dyn #119752

Merged
merged 3 commits into from
Jan 20, 2024
Merged

Avoid ICEs in trait names without dyn #119752

merged 3 commits into from
Jan 20, 2024

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jan 8, 2024

Check diagnostic is error before downgrading. Fix #119633.

Account for traits using self-trait by name without dyn. Fix #119652.

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2024

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 8, 2024
@estebank estebank changed the title Avoid ICE: Check diagnostic is error before downgrading Avoid ICEs in trait names without dyn Jan 10, 2024
Res::Def(DefKind::Trait, id) => {
// When we're dealing with a recursive trait, we don't want to downgrade
// the error, so we consider them to be object safe always. (#119652)
tcx.check_is_object_safe(id) || Some(id) == owner
Copy link
Member

Choose a reason for hiding this comment

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

Can this be swapped to avoid executing the query if not necessary or would that change the meaning?

Copy link
Contributor Author

@estebank estebank Jan 11, 2024

Choose a reason for hiding this comment

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

You mean something like the following?

                    Res::Def(DefKind::Trait, id) if Some(id) == owner => {
                        // When we're dealing with a recursive trait, we don't want to downgrade
                        // the error, so we consider them to be object safe always. (#119652)
                        true
                    }
                    Res::Def(DefKind::Trait, id) => {
                        tcx.check_is_object_safe(id)
                    }

I only left it unconditionally because it doesn't cause any additional problems (it's ok, if incorrect, to check if Self is object safe, surprisingly enough), but don't mind making this split more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be causing some issues for me generating accurate lint suggestions, because types that are not actually object safe are now being marked as object safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trevyn We can also entirely remove the downgrade, given how many head aches this is giving us, but your PR looks correct.

@matthiaskrgr
Copy link
Member

@bors reroll

since its been a week I have been hitting a ton of these while fuzzing which is quite annoying 🙈

@fmease
Copy link
Member

fmease commented Jan 16, 2024

bors-reroll isn't a thing.
r? fmease

@rustbot rustbot assigned fmease and unassigned b-naber Jan 16, 2024
@fmease
Copy link
Member

fmease commented Jan 18, 2024

Looks okay, thanks for fixing this!
What happens though when we have mutual recursion going on?

trait B { fn f(a: A) -> A; }
trait A { fn g(b: B) -> B; }

Then the owner check won't fire I guess and I think we're still ICE'ing? Don't have access to my computer right now, can't check.

If we don't ICE r=me

@fmease
Copy link
Member

fmease commented Jan 18, 2024

relevant

@estebank
Copy link
Contributor Author

@bors r=fmease

@bors
Copy link
Contributor

bors commented Jan 19, 2024

📌 Commit 7edbc95 has been approved by fmease

It is now in the queue for this repository.

@bors 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 Jan 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#103730 (Added NonZeroXxx::from_mut(_unchecked)?)
 - rust-lang#113142 (optimize EscapeAscii's Display  and CStr's Debug)
 - rust-lang#118799 (Stabilize single-field offset_of)
 - rust-lang#119613 (Expose Obligations created during type inference.)
 - rust-lang#119752 (Avoid ICEs in trait names without `dyn`)
 - rust-lang#120132 (Teach tidy about line/col information for malformed features)
 - rust-lang#120135 (SMIR: Make the remaining "private" fields actually private)
 - rust-lang#120148 (`single_use_lifetimes`: Don't suggest deleting lifetimes with bounds)
 - rust-lang#120150 (Stabilize `round_ties_even`)
 - rust-lang#120155 (Don't use `ReErased` to detect type test promotion failed)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#103730 (Added NonZeroXxx::from_mut(_unchecked)?)
 - rust-lang#113142 (optimize EscapeAscii's Display  and CStr's Debug)
 - rust-lang#118799 (Stabilize single-field offset_of)
 - rust-lang#119613 (Expose Obligations created during type inference.)
 - rust-lang#119752 (Avoid ICEs in trait names without `dyn`)
 - rust-lang#120132 (Teach tidy about line/col information for malformed features)
 - rust-lang#120135 (SMIR: Make the remaining "private" fields actually private)
 - rust-lang#120148 (`single_use_lifetimes`: Don't suggest deleting lifetimes with bounds)
 - rust-lang#120150 (Stabilize `round_ties_even`)
 - rust-lang#120155 (Don't use `ReErased` to detect type test promotion failed)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 177d513 into rust-lang:master Jan 20, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
Rollup merge of rust-lang#119752 - estebank:ice-ice, r=fmease

Avoid ICEs in trait names without `dyn`

Check diagnostic is error before downgrading. Fix rust-lang#119633.

 Account for traits using self-trait by name without `dyn`. Fix rust-lang#119652.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…-errors

`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`

rust-lang#119752 leveraged and overloaded `is_object_safe` to prevent an ICE, but accurate object safety information is needed for precise suggestions. This separates out `is_downgradable`, used for the ICE prevention, and `is_object_safe`, which returns to its original meaning.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…-errors

`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`

rust-lang#119752 leveraged and overloaded `is_object_safe` to prevent an ICE, but accurate object safety information is needed for precise suggestions. This separates out `is_downgradable`, used for the ICE prevention, and `is_object_safe`, which returns to its original meaning.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…-errors

`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`

rust-lang#119752 leveraged and overloaded `is_object_safe` to prevent an ICE, but accurate object safety information is needed for precise suggestions. This separates out `is_downgradable`, used for the ICE prevention, and `is_object_safe`, which returns to its original meaning.
@fmease fmease mentioned this pull request Jan 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
Rollup merge of rust-lang#120164 - trevyn:is_downgradable, r=compiler-errors

`maybe_lint_impl_trait`: separate `is_downgradable` from `is_object_safe`

rust-lang#119752 leveraged and overloaded `is_object_safe` to prevent an ICE, but accurate object safety information is needed for precise suggestions. This separates out `is_downgradable`, used for the ICE prevention, and `is_object_safe`, which returns to its original meaning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants