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

Double check that hidden types match the expected hidden type #113661

Merged
merged 5 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
Double check that hidden types match the expected hidden type
  • Loading branch information
oli-obk committed Jul 21, 2023
commit 44e21503a8f2e789bbd90b5dd52590879ba2fab6
4 changes: 2 additions & 2 deletions compiler/rustc_borrowck/src/region_infer/opaque_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,8 @@ fn check_opaque_type_well_formed<'tcx>(
// version.
let errors = ocx.select_all_or_error();

// This is still required for many(half of the tests in ui/type-alias-impl-trait)
// tests to pass
// This is fishy, but we check it again in `check_opaque_meets_bounds`.
// Remove once we can prepopulate with known hidden types.
let _ = infcx.take_opaque_types();

if errors.is_empty() {
Expand Down
11 changes: 8 additions & 3 deletions compiler/rustc_const_eval/src/util/compare_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,13 @@ pub fn is_subtype<'tcx>(
// With `Reveal::All`, opaque types get normalized away, with `Reveal::UserFacing`
// we would get unification errors because we're unable to look into opaque types,
// even if they're constrained in our current function.
//
// It seems very unlikely that this hides any bugs.
let _ = infcx.take_opaque_types();
for (key, ty) in infcx.take_opaque_types() {
span_bug!(
ty.hidden_type.span,
"{}, {}",
tcx.type_of(key.def_id).instantiate(tcx, key.args),
ty.hidden_type.ty
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should hit this bug? 🤔 or do we avoid it because we have OpaqueCast everywhere we would assign to/from opaques?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised, too, but I didn't debug it.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we run the validator before optimizations by default 😅 iirc we only do it with -Zvalidate-mir 🤔

Copy link
Member

Choose a reason for hiding this comment

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

#114121 :D

}
errors.is_empty()
}
143 changes: 139 additions & 4 deletions compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@ use rustc_lint_defs::builtin::REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS;
use rustc_middle::hir::nested_filter;
use rustc_middle::middle::stability::EvalResult;
use rustc_middle::traits::DefiningAnchor;
use rustc_middle::ty::fold::BottomUpFolder;
use rustc_middle::ty::layout::{LayoutError, MAX_SIMD_LANES};
use rustc_middle::ty::util::{Discr, IntTypeExt};
use rustc_middle::ty::GenericArgKind;
use rustc_middle::ty::{
self, AdtDef, ParamEnv, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt,
self, AdtDef, ParamEnv, RegionKind, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable,
TypeVisitableExt,
};
use rustc_session::lint::builtin::{UNINHABITED_STATIC, UNSUPPORTED_CALLING_CONVENTIONS};
use rustc_span::symbol::sym;
Expand All @@ -34,6 +36,7 @@ use rustc_trait_selection::traits::error_reporting::on_unimplemented::OnUnimplem
use rustc_trait_selection::traits::error_reporting::TypeErrCtxtExt as _;
use rustc_trait_selection::traits::outlives_bounds::InferCtxtExt as _;
use rustc_trait_selection::traits::{self, ObligationCtxt, TraitEngine, TraitEngineExt as _};
use rustc_type_ir::fold::TypeFoldable;

use std::ops::ControlFlow;

Expand Down Expand Up @@ -437,7 +440,7 @@ fn check_opaque_meets_bounds<'tcx>(
// hidden type is well formed even without those bounds.
let predicate =
ty::Binder::dummy(ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(hidden_ty.into())));
ocx.register_obligation(Obligation::new(tcx, misc_cause, param_env, predicate));
ocx.register_obligation(Obligation::new(tcx, misc_cause.clone(), param_env, predicate));

// Check that all obligations are satisfied by the implementation's
// version.
Expand All @@ -464,11 +467,143 @@ fn check_opaque_meets_bounds<'tcx>(
ocx.resolve_regions_and_report_errors(defining_use_anchor, &outlives_env)?;
}
}
// Clean up after ourselves
let _ = infcx.take_opaque_types();
// Check that any hidden types found during wf checking match the hidden types that `type_of` sees.
for (key, mut ty) in infcx.take_opaque_types() {
ty.hidden_type.ty = infcx.resolve_vars_if_possible(ty.hidden_type.ty);
sanity_check_found_hidden_type(tcx, key, ty.hidden_type, defining_use_anchor, origin)?;
}
Ok(())
}

fn sanity_check_found_hidden_type<'tcx>(
tcx: TyCtxt<'tcx>,
key: ty::OpaqueTypeKey<'tcx>,
mut ty: ty::OpaqueHiddenType<'tcx>,
defining_use_anchor: LocalDefId,
origin: &hir::OpaqueTyOrigin,
) -> Result<(), ErrorGuaranteed> {
if ty.ty.is_ty_var() {
// Nothing was actually constrained.
return Ok(());
}
if let ty::Alias(ty::Opaque, alias) = ty.ty.kind() {
if alias.def_id == key.def_id.to_def_id() && alias.args == key.args {
// Nothing was actually constrained, this is an opaque usage that was
// only discovered to be opaque after inference vars resolved.
return Ok(());
}
}
// Closures frequently end up containing erased lifetimes in their final representation.
// These correspond to lifetime variables that never got resolved, so we patch this up here.
ty.ty = ty.ty.fold_with(&mut BottomUpFolder {
tcx,
ty_op: |t| t,
ct_op: |c| c,
lt_op: |l| match l.kind() {
RegionKind::ReVar(_) => tcx.lifetimes.re_erased,
_ => l,
},
});
// Get the hidden type, and in case it is in a nested opaque type, find that opaque type's
// usage in the function signature and use the generic arguments from the usage site.
let mut hidden_ty = tcx.type_of(key.def_id).instantiate(tcx, key.args);
if let hir::OpaqueTyOrigin::FnReturn(..) | hir::OpaqueTyOrigin::AsyncFn(..) = origin {
if hidden_ty != ty.ty {
hidden_ty = find_and_apply_rpit_substs(
tcx,
hidden_ty,
defining_use_anchor.to_def_id(),
key.def_id.to_def_id(),
)?;
}
}
// If the hidden types differ, emit a type mismatch diagnostic.
if hidden_ty == ty.ty {
Ok(())
} else {
let span = tcx.def_span(key.def_id);
let other = ty::OpaqueHiddenType { ty: hidden_ty, span };
Err(ty.report_mismatch(&other, key.def_id, tcx).emit())
}
}

fn find_and_apply_rpit_substs<'tcx>(
tcx: TyCtxt<'tcx>,
mut hidden_ty: Ty<'tcx>,
function: DefId,
opaque: DefId,
) -> Result<Ty<'tcx>, ErrorGuaranteed> {
// Find use of the RPIT in the function signature and thus find the right substs to
// convert it into the parameter space of the function signature. This is needed,
// because that's what `type_of` returns, against which we compare later.
let ret = tcx.fn_sig(function).instantiate_identity().output();
struct Visitor<'tcx> {
tcx: TyCtxt<'tcx>,
opaque: DefId,
function: DefId,
seen: FxHashSet<DefId>,
}
impl<'tcx> ty::TypeVisitor<TyCtxt<'tcx>> for Visitor<'tcx> {
type BreakTy = GenericArgsRef<'tcx>;

#[instrument(level = "trace", skip(self), ret)]
fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
trace!("{:#?}", t.kind());
match t.kind() {
ty::Alias(ty::Opaque, alias) => {
trace!(?alias.def_id);
if alias.def_id == self.opaque {
return ControlFlow::Break(alias.args);
} else if self.seen.insert(alias.def_id) {
for clause in self
.tcx
.explicit_item_bounds(alias.def_id)
.iter_instantiated_copied(self.tcx, alias.args)
{
trace!(?clause);
clause.visit_with(self)?;
}
}
}
ty::Alias(ty::Projection, alias) => {
if self.tcx.is_impl_trait_in_trait(alias.def_id)
&& self.tcx.impl_trait_in_trait_parent_fn(alias.def_id) == self.function
{
// If we're lowering to associated item, install the opaque type which is just
// the `type_of` of the trait's associated item. If we're using the old lowering
// strategy, then just reinterpret the associated type like an opaque :^)
self.tcx
.type_of(alias.def_id)
.instantiate(self.tcx, alias.args)
.visit_with(self)?;
}
}
ty::Alias(ty::Weak, alias) => {
self.tcx
.type_of(alias.def_id)
.instantiate(self.tcx, alias.args)
.visit_with(self)?;
}
_ => (),
}

t.super_visit_with(self)
}
}
if let ControlFlow::Break(args) =
ret.visit_with(&mut Visitor { tcx, function, opaque, seen: Default::default() })
{
trace!(?args);
trace!("expected: {hidden_ty:#?}");
hidden_ty = ty::EarlyBinder::bind(hidden_ty).instantiate(tcx, args);
trace!("expected: {hidden_ty:#?}");
} else {
tcx.sess
.delay_span_bug(tcx.def_span(function), format!("{ret:?} does not contain {opaque:?}"));
}
Ok(hidden_ty)
}

fn is_enum_of_nonnullable_ptr<'tcx>(
tcx: TyCtxt<'tcx>,
adt_def: AdtDef<'tcx>,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_trait_selection/src/solve/eval_ctxt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
// assertions against dropping an `InferCtxt` without taking opaques.
// FIXME: Once we remove support for the old impl we can remove this.
if input.anchor != DefiningAnchor::Error {
// This seems ok, but fragile.
let _ = infcx.take_opaque_types();
}

Expand Down
2 changes: 1 addition & 1 deletion src/tools/tidy/src/ui_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::path::{Path, PathBuf};

const ENTRY_LIMIT: usize = 900;
// FIXME: The following limits should be reduced eventually.
const ISSUES_ENTRY_LIMIT: usize = 1894;
const ISSUES_ENTRY_LIMIT: usize = 1893;
const ROOT_ENTRY_LIMIT: usize = 870;

const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
Expand Down
57 changes: 57 additions & 0 deletions tests/ui/type-alias-impl-trait/hidden_type_mismatch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
//! This test checks that we don't lose hidden types
//! for *other* opaque types that we register and use
//! to prove bounds while checking that a hidden type
//! satisfies its opaque type's bounds.

#![feature(trivial_bounds, type_alias_impl_trait)]
#![allow(trivial_bounds)]

mod sus {
use super::*;
pub type Sep = impl Sized + std::fmt::Display;
//~^ ERROR: concrete type differs from previous defining opaque type use
pub fn mk_sep() -> Sep {
String::from("hello")
}

pub trait Proj {
type Assoc;
}
impl Proj for () {
type Assoc = sus::Sep;
}

pub struct Bar<T: Proj> {
pub inner: <T as Proj>::Assoc,
pub _marker: T,
}
impl<T: Proj> Clone for Bar<T> {
fn clone(&self) -> Self {
todo!()
}
}
impl<T: Proj<Assoc = i32> + Copy> Copy for Bar<T> {}
// This allows producing `Tait`s via `From`, even though
// `define_tait` is not actually callable, and thus assumed
// `Bar<()>: Copy` even though it isn't.
pub type Tait = impl Copy + From<Bar<()>> + Into<Bar<()>>;
pub fn define_tait() -> Tait
where
// this proves `Bar<()>: Copy`, but `define_tait` is
// now uncallable
(): Proj<Assoc = i32>,
{
Bar { inner: 1i32, _marker: () }
}
}

fn copy_tait(x: sus::Tait) -> (sus::Tait, sus::Tait) {
(x, x)
}

fn main() {
let bar = sus::Bar { inner: sus::mk_sep(), _marker: () };
let (y, z) = copy_tait(bar.into()); // copy a string
drop(y.into()); // drop one instance
println!("{}", z.into().inner); // print the other
}
14 changes: 14 additions & 0 deletions tests/ui/type-alias-impl-trait/hidden_type_mismatch.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: concrete type differs from previous defining opaque type use
--> $DIR/hidden_type_mismatch.rs:11:20
|
LL | pub type Sep = impl Sized + std::fmt::Display;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `i32`, got `String`
|
note: previous use here
--> $DIR/hidden_type_mismatch.rs:37:21
|
LL | pub type Tait = impl Copy + From<Bar<()>> + Into<Bar<()>>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// check-pass

// Regression test for issue #83190, triggering an ICE in borrowck.

// check-pass

pub trait Any {}
impl<T> Any for T {}

Expand Down