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.

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

Erase late bound regions for dropped instances #107936

Closed
wants to merge 3 commits into from
Closed

Erase late bound regions for dropped instances #107936

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b00a3c0
Erase late bound regions while resolving drop_in_place
Rattenkrieg Feb 11, 2023
2826efc
fix imports after merge
Rattenkrieg Feb 14, 2023
6c7bcb5
move test from run-make to ui/coercion
Rattenkrieg Feb 17, 2023
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
13 changes: 11 additions & 2 deletions compiler/rustc_middle/src/ty/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use crate::ty::print::{FmtPrinter, Printer};
use crate::ty::{self, Ty, TyCtxt, TypeFoldable, TypeSuperFoldable};
use crate::ty::{self, Binder, Ty, TyCtxt, TyKind, TypeFoldable, TypeSuperFoldable};
use crate::ty::{EarlyBinder, InternalSubsts, SubstsRef};
use rustc_errors::ErrorGuaranteed;
use rustc_hir::def::Namespace;
Expand Down Expand Up @@ -539,8 +539,17 @@ impl<'tcx> Instance<'tcx> {
}

pub fn resolve_drop_in_place(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> ty::Instance<'tcx> {
let ty_erased =
if let (true, TyKind::FnPtr(poly_fn_sig)) = (ty.has_late_bound_regions(), ty.kind()) {
let re_erased = tcx.erase_late_bound_regions(*poly_fn_sig);
let rebound = Binder::dummy(re_erased);
let reassembled = tcx.mk_fn_ptr(rebound);
reassembled
} else {
ty
};
Comment on lines +542 to +550
Copy link
Contributor

@cjgillot cjgillot Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
let ty_erased =
if let (true, TyKind::FnPtr(poly_fn_sig)) = (ty.has_late_bound_regions(), ty.kind()) {
let re_erased = tcx.erase_late_bound_regions(*poly_fn_sig);
let rebound = Binder::dummy(re_erased);
let reassembled = tcx.mk_fn_ptr(rebound);
reassembled
} else {
ty
};
// Erase regions before calling resolve, as they must not change the resolution.
let ty_erased = tcx.erase_regions(ty);

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 I tried that but normalizing late bound regions is not enough in this case. There's still a mismatch because one symbol looks like core::ptr::drop_in_place::<for<'a> fn(&'a ())> and the other like core::ptr::drop_in_place::<fn(&())>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed my suggestion to use erase_regions that should erase everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, not sure actually. It's probably the instance resolution code that needs to be fortified against leaking regions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed my suggestion to use erase_regions that should erase everything.

I believe this already happening without desired effect wrt to this bug: Instance::expect_resolve -> Instance::resolve -> Instance::resolve_opt_const_arg -> tcx.erase_regions(substs). Anyway, I'm going to give it a try.

Copy link
Member

@michaelwoerister michaelwoerister Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Changed my suggestion to use erase_regions that should erase everything.

That's what I thought too, but it doesn't seem to be the case:

/// Returns an equivalent value with all free regions removed (note
/// that late-bound regions remain, because they are important for
/// subtyping, but they are anonymized and normalized as well)..
pub fn erase_regions(self, value: T) -> T

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked, it doesn't erase late bound regions.

Copy link
Contributor Author

@Rattenkrieg Rattenkrieg Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

rust/compiler/rustc_middle/src/ty/erase_regions.rs

Lines 54 to 64 in b5c8c32

fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
// because late-bound regions affect subtyping, we can't
// erase the bound/free distinction, but we can replace
// all free regions with 'erased.
//
// Note that we *CAN* replace early-bound regions -- the
// type system never "sees" those, they get substituted
// away. In codegen, they will always be erased to 'erased
// whenever a substitution occurs.
match *r {
ty::ReLateBound(..) => r,

this is the TypeFolder invoked during erase_regions

let def_id = tcx.require_lang_item(LangItem::DropInPlace, None);
let substs = tcx.intern_substs(&[ty.into()]);
let substs = tcx.intern_substs(&[ty_erased.into()]);
Instance::expect_resolve(tcx, ty::ParamEnv::reveal_all(), def_id, substs)
}

Expand Down
4 changes: 4 additions & 0 deletions tests/run-make/issue-107205/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
include ../../run-make-fulldeps/tools.mk

all:
$(RUSTC) main.rs
16 changes: 16 additions & 0 deletions tests/run-make/issue-107205/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
use std::fmt;

pub struct Wrapper(fn(val: &()));

impl fmt::Debug for Wrapper {
fn fmt<'a>(&'a self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_tuple("Wrapper").field(&self.0 as &fn(&'a ())).finish()
}
}

fn useful(_: &()) {
}

fn main() {
println!("{:?}", Wrapper(useful));
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you try to turn this into a build-pass UI test? That would be more lightweight than a run-make test. Here is an example:

rust/tests/ui/associated-types/issue-64848.rs

Line 1 in 9556b56

// build-pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it under tests/ui/coercion/ directory.