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 4 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
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()
}
179 changes: 175 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,179 @@ 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.
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_args(
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())
}
}

/// 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.
/// We need to do because RPITs ignore the lifetimes of the function,
/// as they have their own copies of all the lifetimes they capture.
/// So the only way to get the lifetimes represented in terms of the function,
/// is to look how they are used in the function signature (or do some other fancy
/// recording of this mapping at ast -> hir lowering time).
///
/// As an example:
/// ```text
/// trait Id {
/// type Assoc;
/// }
/// impl<'a> Id for &'a () {
/// type Assoc = &'a ();
/// }
/// fn func<'a>(x: &'a ()) -> impl Id<Assoc = impl Sized + 'a> { x }
/// // desugared to
/// fn func<'a>(x: &'a () -> Outer<'a, Assoc = Inner<'a>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// fn func<'a>(x: &'a () -> Outer<'a, Assoc = Inner<'a>> {
/// fn func<'a>(x: &'a () -> Outer<'a, 'a> {

? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea... I don't know how to represent the fact that we walk the item bounds of Outer

/// // Note that in contrast to other nested items, RPIT type aliases can
/// // access their parents' generics.
///
/// // hidden type is `&'aDupOuter ()`
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels wrong, if the hidden type of Outer is &'aDupOuter (), shouldn't wf check of Outer define the hidden type of Inner to be <&'aDupOuter () as Id>::Assoc which is &'aDupOuter (), so this should worK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because we figure out the hidden type of Inner anew during wf check of Outer, we don't get it from type_of. We get the hidden type of Outer, which is &'a (), so the id projection is also &'a ()

/// // During wfcheck the hidden type of `Inner<'aDupOuter>` is `&'a ()`, but
/// // `typeof(Inner<'aDupOuter>) = &'aDupOuter ()`.
/// // So we walk the signature of `func` to find the use of `Inner<'a>`
/// // and then use that to replace the lifetimes in the hidden type, obtaining
/// // `&'a ()`.
/// type Outer<'aDupOuter> = impl Id<Assoc = Inner<'aDupOuter>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

is the 'a param of Inner 'a or 'aDupOuter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'a, it is inherited from the function.

///
/// // hidden type is `&'aDupInner ()`
/// type Inner<'aDupInner> = impl Sized + 'aDupInner;
///
/// x
/// }
/// ```
fn find_and_apply_rpit_args<'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 args 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();
lcnr marked this conversation as resolved.
Show resolved Hide resolved
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