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

Turn eager normalization errors to delayed errors #82039

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion compiler/rustc_passes/src/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,13 @@ impl Visitor<'tcx> for Checker<'tcx> {
let ty = self.tcx.type_of(item.def_id);
let (adt_def, substs) = match ty.kind() {
ty::Adt(adt_def, substs) => (adt_def, substs),
_ => bug!(),
_ => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this actually reachable? Do we have a test case where hir::ItemKind::Union ends up as ty::Error?

Copy link
Member

Choose a reason for hiding this comment

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

Also curious about this. Was this getting hit? If not, can you revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, I added this because it is reached after removing the other ICE.

self.tcx.sess.delay_span_bug(
item.span,
&format!("unexpected type kind {:?} (`{:?}`)", ty, ty.kind()),
);
return;
}
};

// Non-`Copy` fields are unstable, except for `ManuallyDrop`.
Expand Down
9 changes: 8 additions & 1 deletion compiler/rustc_traits/src/normalize_erasing_regions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::traits::query::NoSolution;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, ParamEnvAnd, TyCtxt, TypeFoldable};
use rustc_span::DUMMY_SP;
use rustc_trait_selection::traits::query::normalize::AtExt;
use rustc_trait_selection::traits::{Normalized, ObligationCause};
use std::sync::atomic::Ordering;
Expand Down Expand Up @@ -51,7 +52,13 @@ fn normalize_after_erasing_regions<'tcx, T: TypeFoldable<'tcx> + PartialEq + Cop
debug_assert!(!erased.needs_infer(), "{:?}", erased);
erased
}
Err(NoSolution) => bug!("could not fully normalize `{:?}`", value),
Copy link
Contributor

@lcnr lcnr Feb 13, 2021

Choose a reason for hiding this comment

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

this bug is quite valuable and helped us catch some bugs with const generics, so I would really like to keep it that way.

We pretty much only emit a bug here if the type is not well formed, and I think normalize_erasing_regions should only be used on types that are well formed (even if there is another error).

Copy link
Member

Choose a reason for hiding this comment

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

I think having delay_span_bug here should be fine. This mimics bug with -Ztreat-err-as-bug right?

Copy link
Contributor

Choose a reason for hiding this comment

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

We get there with const evaluatable bounds when emitting errors, but these errors should be silent when encountered during selection, so just always causing an ICE here is preferable for me.

We shouldn't hit this, even after having a compilation error. Having a delay_span_bug here hides other bugs in the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We get there with const evaluatable bounds when emitting errors, but these errors should be silent when encountered during selection, so just always causing an ICE here is preferable for me.

I'm not sure I follow. Could you rephrase your position? It reads to me like

  • We get there with const evaluatable bounds when emitting errors
  • these errors should be silent when encountered during selection
  • always causing an ICE here is preferable

means

  • when we emit errors with const evaluatable bounds we get here
  • trait selection should silence this errors
  • we should always ICE when we hit these

which to me is saying two different things?


Would it be reasonable to only delay_span_bug on stable/beta and make it bug! in nightly?

Copy link
Member

Choose a reason for hiding this comment

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

I think what @lcnr means is that the error we get here would be silenced by trait selection, but we don't want that and should ICE.

I would be extremely wary of having different behavior on stable vs nightly.

Copy link
Contributor

Choose a reason for hiding this comment

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

We get there with const evaluatable bounds when emitting errors,

it is an implementation error for const evaluatable bounds to reach this bug! and we still don't know all the places where this happens. Often when reaching this bug! compilation would fail anyways so a delay_span_bug would not trigger in these cases. That would mean that we would only get ICE here once they get triggered by const evaluatable bounds during selection (which then succeeds using a different candidate). This happens a lot less often though, so it would take us longer to find all of these cases. Does this make more sense this way?

Err(NoSolution) => {
infcx
.tcx
.sess
.delay_span_bug(DUMMY_SP, &format!("could not fully normalize `{:?}`", value));
value
}
}
})
}
Expand Down
21 changes: 21 additions & 0 deletions src/test/ui/union/normalization-failure-issue-81199-autofix.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// run-rustfix
#[allow(dead_code)]
#[repr(C)]
union PtrRepr<T: ?Sized + Pointee> {
const_ptr: *const T,
mut_ptr: *mut T,
components: std::mem::ManuallyDrop<PtrComponents<T>>,
//~^ ERROR the trait bound `T: Pointee` is not satisfied
}

#[repr(C)]
struct PtrComponents<T: Pointee + ?Sized> {
data_address: *const (),
metadata: <T as Pointee>::Metadata,
}

pub trait Pointee {
type Metadata;
}

fn main() {}
21 changes: 21 additions & 0 deletions src/test/ui/union/normalization-failure-issue-81199-autofix.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// run-rustfix
#[allow(dead_code)]
#[repr(C)]
union PtrRepr<T: ?Sized> {
const_ptr: *const T,
mut_ptr: *mut T,
components: std::mem::ManuallyDrop<PtrComponents<T>>,
//~^ ERROR the trait bound `T: Pointee` is not satisfied
}

#[repr(C)]
struct PtrComponents<T: Pointee + ?Sized> {
data_address: *const (),
metadata: <T as Pointee>::Metadata,
}

pub trait Pointee {
type Metadata;
}

fn main() {}
19 changes: 19 additions & 0 deletions src/test/ui/union/normalization-failure-issue-81199-autofix.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error[E0277]: the trait bound `T: Pointee` is not satisfied
--> $DIR/normalization-failure-issue-81199-autofix.rs:7:40
|
LL | components: std::mem::ManuallyDrop<PtrComponents<T>>,
| ^^^^^^^^^^^^^^^^ the trait `Pointee` is not implemented for `T`
|
note: required by a bound in `PtrComponents`
--> $DIR/normalization-failure-issue-81199-autofix.rs:12:25
|
LL | struct PtrComponents<T: Pointee + ?Sized> {
| ^^^^^^^ required by this bound in `PtrComponents`
help: consider further restricting this bound
|
LL | union PtrRepr<T: ?Sized + Pointee> {
| +++++++++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
12 changes: 12 additions & 0 deletions src/test/ui/union/normalization-failure-issue-81199-min.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
union PtrRepr<T: ?Sized> {
const_ptr: *const T,
mut_ptr: *mut T,
components: <T as Pointee>::Metadata
//~^ ERROR the trait bound `T: Pointee` is not satisfied
}

pub trait Pointee {
type Metadata;
}

fn main() {}
14 changes: 14 additions & 0 deletions src/test/ui/union/normalization-failure-issue-81199-min.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0277]: the trait bound `T: Pointee` is not satisfied
--> $DIR/normalization-failure-issue-81199-min.rs:4:17
|
LL | components: <T as Pointee>::Metadata
| ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Pointee` is not implemented for `T`
|
help: consider further restricting this bound
|
LL | union PtrRepr<T: ?Sized + Pointee> {
| +++++++++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
19 changes: 19 additions & 0 deletions src/test/ui/union/normalization-failure-issue-81199.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#[repr(C)]
union PtrRepr<T: ?Sized> {
const_ptr: *const T,
mut_ptr: *mut T,
components: PtrComponents<T>,
//~^ ERROR the trait bound `T: Pointee` is not satisfied in `PtrComponents<T>`
}

#[repr(C)]
struct PtrComponents<T: Pointee + ?Sized> {
data_address: *const (),
metadata: <T as Pointee>::Metadata,
}

pub trait Pointee {
type Metadata;
}

fn main() {}
29 changes: 29 additions & 0 deletions src/test/ui/union/normalization-failure-issue-81199.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
error[E0277]: the trait bound `T: Pointee` is not satisfied in `PtrComponents<T>`
--> $DIR/normalization-failure-issue-81199.rs:5:17
|
LL | components: PtrComponents<T>,
| ^^^^^^^^^^^^^^^^ within `PtrComponents<T>`, the trait `Pointee` is not implemented for `T`
|
note: required because it appears within the type `PtrComponents<T>`
--> $DIR/normalization-failure-issue-81199.rs:10:8
|
LL | struct PtrComponents<T: Pointee + ?Sized> {
| ^^^^^^^^^^^^^
= note: no field of a union may have a dynamically sized type
= help: change the field's type to have a statically known size
help: consider further restricting this bound
|
LL | union PtrRepr<T: ?Sized + Pointee> {
| +++++++++
help: borrowed types always have a statically known size
|
LL | components: &PtrComponents<T>,
| +
help: the `Box` type always has a statically known size and allocates its contents in the heap
|
LL | components: Box<PtrComponents<T>>,
| ++++ +

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.