Skip to content

fix: Fix mir evaluator not respecting block local trait impls #16299

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: Fix mir evaluator not respecting block local trait impls #16299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
122 changes: 79 additions & 43 deletions crates/hir-ty/src/method_resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//! For details about how this works in rustc, see the method lookup page in the
//! [rustc guide](https://rust-lang.github.io/rustc-guide/method-lookup.html)
//! and the corresponding code mostly in rustc_hir_analysis/check/method/probe.rs.
use std::ops::ControlFlow;
use std::{iter, ops::ControlFlow};

use base_db::{CrateId, Edition};
use chalk_ir::{cast::Cast, Mutability, TyKind, UniverseIndex, WhereClause};
Expand Down Expand Up @@ -705,15 +705,13 @@ pub(crate) fn lookup_impl_method_query(
};

let name = &db.function_data(func).name;
let Some((impl_fn, impl_subst)) =
lookup_impl_assoc_item_for_trait_ref(trait_ref, db, env, name).and_then(|assoc| {
if let (AssocItemId::FunctionId(id), subst) = assoc {
Some((id, subst))
} else {
None
}
})
else {
let Some((impl_fn, impl_subst)) = lookup_impl_assoc_item_for_trait_ref(
trait_ref, db, env, name,
)
.and_then(|assoc| match assoc {
(AssocItemId::FunctionId(id), subst) => Some((id, subst)),
_ => None,
}) else {
return (func, fn_subst);
};
(
Expand All @@ -734,21 +732,54 @@ fn lookup_impl_assoc_item_for_trait_ref(
let hir_trait_id = trait_ref.hir_trait_id();
let self_ty = trait_ref.self_type_parameter(Interner);
let self_ty_fp = TyFingerprint::for_trait_impl(&self_ty)?;
let impls = db.trait_impls_in_deps(env.krate);
let self_impls = match self_ty.kind(Interner) {
TyKind::Adt(id, _) => {
id.0.module(db.upcast()).containing_block().map(|it| db.trait_impls_in_block(it))

let trait_module = hir_trait_id.module(db.upcast());
let type_module = match self_ty_fp {
TyFingerprint::Adt(adt_id) => Some(adt_id.module(db.upcast())),
TyFingerprint::ForeignType(type_id) => {
Some(from_foreign_def_id(type_id).module(db.upcast()))
}
TyFingerprint::Dyn(trait_id) => Some(trait_id.module(db.upcast())),
_ => None,
};
let impls = impls
.iter()
.chain(self_impls.as_ref())
.flat_map(|impls| impls.for_trait_and_self_ty(hir_trait_id, self_ty_fp));

let table = InferenceTable::new(db, env);
let mut def_blocks =
[trait_module.containing_block(), type_module.and_then(|it| it.containing_block())];

let impls = db.trait_impls_in_deps(env.krate);
let in_self = db.trait_impls_in_crate(env.krate);
let block_impls = iter::successors(env.block, |&block_id| {
db.block_def_map(block_id).parent().and_then(|module| module.containing_block())
})
.inspect(|&block_id| {
// make sure we don't search the same block twice
def_blocks.iter_mut().for_each(|block| {
if *block == Some(block_id) {
*block = None;
}
});
})
.map(|block_id| db.trait_impls_in_block(block_id));

let impls = block_impls.chain(iter::once(in_self).chain(impls.iter().cloned()));

let mut table = InferenceTable::new(db, env);

let (impl_data, impl_subst) = find_matching_impl(impls, table, trait_ref)?;
let (impl_data, impl_subst) =
match find_matching_impl(impls, &mut table, trait_ref.clone(), self_ty_fp) {
Some(it) => it,
None => {
if def_blocks.iter().all(Option::is_none) {
return None;
}
find_matching_impl(
def_blocks.into_iter().flatten().map(|b| db.trait_impls_in_block(b)),
&mut table,
trait_ref,
self_ty_fp,
)?
}
};
let item = impl_data.items.iter().find_map(|&it| match it {
AssocItemId::FunctionId(f) => {
(db.function_data(f).name == *name).then_some(AssocItemId::FunctionId(f))
Expand All @@ -765,33 +796,38 @@ fn lookup_impl_assoc_item_for_trait_ref(
}

fn find_matching_impl(
mut impls: impl Iterator<Item = ImplId>,
mut table: InferenceTable<'_>,
mut impls: impl Iterator<Item = Arc<TraitImpls>>,
table: &mut InferenceTable<'_>,
actual_trait_ref: TraitRef,
self_ty_fp: TyFingerprint,
) -> Option<(Arc<ImplData>, Substitution)> {
let db = table.db;
impls.find_map(|impl_| {
table.run_in_snapshot(|table| {
let impl_data = db.impl_data(impl_);
let impl_substs =
TyBuilder::subst_for_def(db, impl_, None).fill_with_inference_vars(table).build();
let trait_ref = db
.impl_trait(impl_)
.expect("non-trait method in find_matching_impl")
.substitute(Interner, &impl_substs);

if !table.unify(&trait_ref, &actual_trait_ref) {
return None;
}
let hir_trait_id = actual_trait_ref.hir_trait_id();
impls.find_map(|impls| {
for impl_ in impls.for_trait_and_self_ty(hir_trait_id, self_ty_fp) {
return table.run_in_snapshot(|table| {
let impl_substs = TyBuilder::subst_for_def(db, impl_, None)
.fill_with_inference_vars(table)
.build();
let trait_ref = db
.impl_trait(impl_)
.expect("non-trait method in find_matching_impl")
.substitute(Interner, &impl_substs);

if !table.unify(&trait_ref, &actual_trait_ref) {
return None;
}

let wcs = crate::chalk_db::convert_where_clauses(db, impl_.into(), &impl_substs)
.into_iter()
.map(|b| b.cast(Interner));
let goal = crate::Goal::all(Interner, wcs);
table.try_obligation(goal.clone())?;
table.register_obligation(goal);
Some((impl_data, table.resolve_completely(impl_substs)))
})
let wcs = crate::chalk_db::convert_where_clauses(db, impl_.into(), &impl_substs)
.into_iter()
.map(|b| b.cast(Interner));
let goal = crate::Goal::all(Interner, wcs);
table.try_obligation(goal.clone())?;
table.register_obligation(goal);
Some((db.impl_data(impl_), table.resolve_completely(impl_substs)))
});
}
None
})
}

Expand Down
6 changes: 5 additions & 1 deletion crates/hir-ty/src/mir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,8 @@ pub enum Rvalue {

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum StatementKind {
TraitEnvBlockEnter(hir_def::BlockId),
TraitEnvBlockExit,
Comment on lines 1018 to +1020
Copy link
Member Author

@Veykril Veykril Jan 7, 2024

Choose a reason for hiding this comment

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

Not too happy about having to add extra statements for this, but I can't think of a better way to encode this.

Copy link
Member

@flodiebold flodiebold Jan 7, 2024

Choose a reason for hiding this comment

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

Sorry to be making comments from the sidelines since I don't have the whole context, but it's kind of surprising to me that there's any trait solving / method resolution happening during MIR eval at all 🤔

Copy link
Member Author

@Veykril Veykril Jan 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is surprising, but the reason for that is that we currently try to resolve the concrete method of a trait function "late". That is inference result only resolves the trait function, but MIR later wants the function of the actual trait implementation which happens in mir lowering/eval currrently (and in the IDE layer for goto def etc). I guess we should be able to move that logic into type check (and get rid of the lookup_impl_method_query).

Also comments like this are very much appreciated.

Copy link
Member

@flodiebold flodiebold Jan 7, 2024

Choose a reason for hiding this comment

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

Can we do it during MIR lowering?

Copy link
Member

@HKalbasi HKalbasi Jan 7, 2024

Choose a reason for hiding this comment

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

I don't think we can get rid of the lookup_impl_method_query in MIR eval completely without changing the evaulation of dyn Trait which currently only stores the type id in the metadata part, and resolve the impl method on demand. Though I think we can switch to a more direct vtable representation, and then do all the method resolutions in the monomorphization.

Does the current implementation handle block local trait impls for dyn Trait? For example:

trait Foo {
    fn foo(&self);
}

fn some_dyn_foo() -> Box<dyn Foo> {
    struct St;
    impl Foo for St { fn foo(&self) { println!("foo"); } }
    Box::new(St)
}

fn main() {
    let x = some_dyn_foo();
    x.foo();
}

If not, is it possible to handle it in future under current architecture?

Copy link
Member

@flodiebold flodiebold Jan 7, 2024

Choose a reason for hiding this comment

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

Though I think we can switch to a more direct vtable representation, and then do all the method resolutions in the monomorphization.

Yes, IMO that's what we should do.

Assign(Place, Rvalue),
FakeRead(Place),
//SetDiscriminant {
Expand Down Expand Up @@ -1126,7 +1128,9 @@ impl MirBody {
StatementKind::FakeRead(p) | StatementKind::Deinit(p) => {
f(p, &mut self.projection_store)
}
StatementKind::StorageLive(_)
StatementKind::TraitEnvBlockEnter(_)
| StatementKind::TraitEnvBlockExit
| StatementKind::StorageLive(_)
| StatementKind::StorageDead(_)
| StatementKind::Nop => (),
}
Expand Down
14 changes: 11 additions & 3 deletions crates/hir-ty/src/mir/borrowck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ fn moved_out_of_ref(db: &dyn HirDatabase, body: &MirBody) -> Vec<MovedOutOfRef>
| StatementKind::Deinit(_)
| StatementKind::StorageLive(_)
| StatementKind::StorageDead(_)
| StatementKind::Nop => (),
| StatementKind::Nop
| StatementKind::TraitEnvBlockEnter(_)
| StatementKind::TraitEnvBlockExit => (),
}
}
match &block.terminator {
Expand Down Expand Up @@ -269,7 +271,9 @@ fn ever_initialized_map(
StatementKind::Deinit(_)
| StatementKind::FakeRead(_)
| StatementKind::Nop
| StatementKind::StorageLive(_) => (),
| StatementKind::StorageLive(_)
| StatementKind::TraitEnvBlockEnter(_)
| StatementKind::TraitEnvBlockExit => (),
}
}
let Some(terminator) = &block.terminator else {
Expand Down Expand Up @@ -424,7 +428,11 @@ fn mutability_of_locals(
StatementKind::StorageDead(p) => {
ever_init_map.insert(*p, false);
}
StatementKind::Deinit(_) | StatementKind::StorageLive(_) | StatementKind::Nop => (),
StatementKind::Deinit(_)
| StatementKind::StorageLive(_)
| StatementKind::Nop
| StatementKind::TraitEnvBlockEnter(_)
| StatementKind::TraitEnvBlockExit => (),
}
}
let Some(terminator) = &block.terminator else {
Expand Down
42 changes: 28 additions & 14 deletions crates/hir-ty/src/mir/eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ enum MirOrDynIndex {

pub struct Evaluator<'a> {
db: &'a dyn HirDatabase,
trait_env: Arc<TraitEnvironment>,
trait_env: Vec<Arc<TraitEnvironment>>,
stack: Vec<u8>,
heap: Vec<u8>,
code_stack: Vec<StackFrame>,
Expand Down Expand Up @@ -578,7 +578,7 @@ impl Evaluator<'_> {
static_locations: Default::default(),
db,
random_state: oorandom::Rand64::new(0),
trait_env: trait_env.unwrap_or_else(|| db.trait_environment_for_body(owner)),
trait_env: vec![trait_env.unwrap_or_else(|| db.trait_environment_for_body(owner))],
crate_id,
stdout: vec![],
stderr: vec![],
Expand Down Expand Up @@ -784,7 +784,7 @@ impl Evaluator<'_> {
}
let r = self
.db
.layout_of_ty(ty.clone(), self.trait_env.clone())
.layout_of_ty(ty.clone(), self.trait_env.last().unwrap().clone())
.map_err(|e| MirEvalError::LayoutError(e, ty.clone()))?;
self.layout_cache.borrow_mut().insert(ty.clone(), r.clone());
Ok(r)
Expand Down Expand Up @@ -861,6 +861,12 @@ impl Evaluator<'_> {
| StatementKind::FakeRead(_)
| StatementKind::StorageDead(_)
| StatementKind::Nop => (),
&StatementKind::TraitEnvBlockEnter(block) => {
let mut with_block = self.trait_env.last().unwrap().clone();
TraitEnvironment::with_block(&mut with_block, block);
self.trait_env.push(with_block);
}
StatementKind::TraitEnvBlockExit => _ = self.trait_env.pop(),
}
}
let Some(terminator) = current_block.terminator.as_ref() else {
Expand Down Expand Up @@ -1689,13 +1695,22 @@ impl Evaluator<'_> {
let mut const_id = *const_id;
let mut subst = subst.clone();
if let hir_def::GeneralConstId::ConstId(c) = const_id {
let (c, s) = lookup_impl_const(self.db, self.trait_env.clone(), c, subst);
let (c, s) = lookup_impl_const(
self.db,
self.trait_env.last().unwrap().clone(),
c,
subst,
);
const_id = hir_def::GeneralConstId::ConstId(c);
subst = s;
}
result_owner = self
.db
.const_eval(const_id.into(), subst, Some(self.trait_env.clone()))
.const_eval(
const_id.into(),
subst,
Some(self.trait_env.last().unwrap().clone()),
)
.map_err(|e| {
let name = const_id.name(self.db.upcast());
MirEvalError::ConstEvalError(name, Box::new(e))
Expand Down Expand Up @@ -2015,7 +2030,7 @@ impl Evaluator<'_> {
if let Some((v, l)) = detect_variant_from_bytes(
&layout,
this.db,
this.trait_env.clone(),
this.trait_env.last().unwrap().clone(),
bytes,
e,
) {
Expand Down Expand Up @@ -2096,7 +2111,7 @@ impl Evaluator<'_> {
if let Some((variant, layout)) = detect_variant_from_bytes(
&layout,
self.db,
self.trait_env.clone(),
self.trait_env.last().unwrap().clone(),
self.read_memory(addr, layout.size.bytes_usize())?,
e,
) {
Expand Down Expand Up @@ -2197,7 +2212,7 @@ impl Evaluator<'_> {
.monomorphized_mir_body_for_closure(
closure,
generic_args.clone(),
self.trait_env.clone(),
self.trait_env.last().unwrap().clone(),
)
.map_err(|it| MirEvalError::MirLowerErrorForClosure(closure, it))?;
let closure_data = if mir_body.locals[mir_body.param_locals[0]].ty.as_reference().is_some()
Expand Down Expand Up @@ -2295,17 +2310,16 @@ impl Evaluator<'_> {
return Ok(r.clone());
}
let (def, generic_args) = pair;
let env = self.trait_env.last().unwrap().clone();
let r = if let Some(self_ty_idx) =
is_dyn_method(self.db, self.trait_env.clone(), def, generic_args.clone())
is_dyn_method(self.db, env.clone(), def, generic_args.clone())
{
MirOrDynIndex::Dyn(self_ty_idx)
} else {
let (imp, generic_args) =
self.db.lookup_impl_method(self.trait_env.clone(), def, generic_args.clone());
let mir_body = self
.db
.monomorphized_mir_body(imp.into(), generic_args, self.trait_env.clone())
.map_err(|e| {
self.db.lookup_impl_method(env.clone(), def, generic_args.clone());
let mir_body =
self.db.monomorphized_mir_body(imp.into(), generic_args, env).map_err(|e| {
MirEvalError::InFunction(
Box::new(MirEvalError::MirLowerError(imp, e)),
vec![(Either::Left(imp), span, locals.body.owner)],
Expand Down
35 changes: 35 additions & 0 deletions crates/hir-ty/src/mir/eval/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,3 +833,38 @@ fn main() {
"#,
);
}

#[test]
fn regression_block_def_map() {
check_pass(
r#"
//- minicore: sized

trait Write {
fn write(&mut self) {
trait SpecWriteFmt {
fn spec_write_fmt(self);
}

impl<W: Write + ?Sized> SpecWriteFmt for &mut W {
#[inline]
default fn spec_write_fmt(self) {}
}

impl<W: Write> SpecWriteFmt for &mut W {
#[inline]
fn spec_write_fmt(self) {}
}
self.spec_write_fmt()
}
}

struct C;
impl Write for C {}

fn main() {
C.write();
}
"#,
);
}
Loading