Refactoring TyBox -> TyAdt #39230

petrochenkov · 2017-01-21T20:34:59Z

petrochenkov · 2017-01-21T20:49:40Z

The largest area where Box is still special is various layout calculations and translation, I haven't touched anything of it. I believe all of it can be done uniformly with normal structs.
There's also drop translation. I'm not sure it can be completely moved into the library given that Box is piece-by-piece movable, but it may be possible to move at least deallocation with some remaining compiler support.
All the remaining special-ness is borrowck stuff and unsizing coercions + box expressions and patterns.

eddyb · 2017-01-21T20:40:46Z

src/librustc_typeck/check/regionck.rs

-                self.walk_cast(cast_expr, from_referent_ty, to_referent_ty);
+            /*From:*/ (&ty::TyAdt(from_def, _),
+            /*To:  */  &ty::TyAdt(to_def, _)) if from_def.is_box() && to_def.is_box() => {
+                self.walk_cast(cast_expr, from_ty.boxed_ty(), to_ty.boxed_ty());


@nikomatsakis Why is this special-casing Box<T>?

eddyb · 2017-01-21T20:42:19Z

src/librustc_typeck/check/mod.rs

@@ -3486,7 +3486,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
          hir::ExprBox(ref subexpr) => {
            let expected_inner = expected.to_option(self).map_or(NoExpectation, |ty| {
                match ty.sty {
-                    ty::TyBox(ty) => Expectation::rvalue_hint(self, ty),
+                    ty::TyAdt(def, _) if def.is_box()
+                        => Expectation::rvalue_hint(self, ty.boxed_ty()),


I'd have substs.type_at(0) everywhere instead of a boxed_ty method.

But of course, this may be too annoying to repeat at every single use site.

It's used often. A dedicated method seems to be more convenient and readable.

Fair enough. My next choice would be box_pointee but boxed_ty seems fine anyway.

eddyb · 2017-01-21T20:44:54Z

src/librustc_trans/glue.rs

-            // Support for TyBox is built-in and its drop glue is
-            // special. It may move to library and have Drop impl. As
-            // a safe-guard, assert TyBox not used with TyContents.
-            assert!(!skip_dtor);


Don't we still need the assert?

The assert started failing when Box got a Drop impl.

Oh, then skip_dtor should toggle calling free (which can also be the Drop impl?).

Do you mean something like

--- a/src/librustc_trans/glue.rs +++ b/src/librustc_trans/glue.rs @@ -210,7 +210,7 @@ pub fn implement_drop_glue<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, g: DropGlueKi }; let bcx = match t.sty { - ty::TyAdt(def, _) if def.is_box() => { + ty::TyAdt(def, _) if def.is_box() && !skip_dtor => { // Support for Box is built-in as yet and its drop glue is special // despite having a dummy Drop impl in the library. let content_ty = t.boxed_ty();

?

I.e. if we get here with skip_dtor == false, we generate full drop glue (including both dropping the inner type and deallocation), otherwise we generate "structural drop" only, which is noop for Box.

Btw, what happens if full drop glue in generated in both cases? I haven't observed any regressions in tests.

It might be not used at all. @michaelwoerister and @arielb1 would know.
What I meant is making the trans_exchange_free_ty call conditional on !skip_dtor.

I've restored the previous behavior in another place (find_drop_glue_neighbors) and put this assert back.

eddyb · 2017-01-21T20:48:43Z

src/librustc/ty/sty.rs

+    pub fn boxed_ty(&self) -> Ty<'tcx> {
+        match self.sty {
+            TyAdt(def, substs) if def.is_box() =>
+                substs.types().next().expect("Box<T> doesn't have type parameters"),


This should be substs.type_at(0).

eddyb · 2017-01-21T20:49:46Z

src/librustc/ty/mod.rs

@@ -1322,6 +1322,7 @@ bitflags! {
        const IS_SIMD             = 1 << 4,
        const IS_FUNDAMENTAL      = 1 << 5,
        const IS_UNION            = 1 << 6,
+        const IS_BOX              = 1 << 7,


This seems maybe a bit sketchy but it's a bit of cache so I can live with it.

Manishearth · 2017-01-21T20:53:00Z

I would like to eventually move the piece-by-piece movable stuff into an internal-only DerefMove. Might need to make it an unsafe trait since we may have to make assumptions about the destructor.

eddyb · 2017-01-21T20:53:38Z

Unsizing coercions and layout go hand in hand. The problem with layout is unpacking newtypes and that's a pretty big can of worms given how LLVM "types" work, several strategical refactors are still needed there.

petrochenkov · 2017-01-28T00:21:58Z

Rebased, comments addressed.

eddyb · 2017-01-28T00:52:10Z

@bors r+

bors · 2017-01-28T00:52:11Z

📌 Commit 8d36b1c has been approved by eddyb

bors · 2017-01-28T05:59:28Z

⌛ Testing commit 8d36b1c with merge 4065356...

bors · 2017-01-28T06:21:19Z

💔 Test failed - status-appveyor

bors · 2017-01-28T08:50:34Z

☔ The latest upstream changes (presumably #39305) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2017-01-28T12:27:26Z

I can't reproduce the Appveyor failure locally. Let's try again.
@bors r=eddyb

bors · 2017-01-28T12:27:26Z

📌 Commit f6be8fa has been approved by eddyb

bors · 2017-01-28T12:27:33Z

⌛ Testing commit f6be8fa with merge c3dec74...

bors · 2017-01-28T12:50:59Z

💔 Test failed - status-appveyor

petrochenkov · 2017-01-28T16:15:23Z

I still can't reproduce this assert with all debug and assert options enabled.
The assert doesn't also happen when I use exactly the options used by Appveyor (modulo --enable-sccache).

@alexcrichton
What version of LLVM is used by Appveyor ?
How is it possible for the LLVM assert to fire on Appveyor while LLVM asserts are not enabled there?

petrochenkov · 2017-01-28T16:21:59Z

The assert is about MSVC debuginfo:

Assertion failed: !isa<DIType>(Scope) && "shouldn't make a namespace scope for a type", file C:\projects\rust\src\llvm\lib\CodeGen\AsmPrinter\CodeViewDebug.cpp, line 202

@michaelwoerister Maybe you have ideas what part of this PR could do this?

alexcrichton · 2017-01-28T18:52:34Z

@petrochenkov it should be whatever the llvm submodule is, but maybe a stale version is cached? You could try touching the llvm trigger and re-send to bors to see if a fresh compile works?

petrochenkov · 2017-01-28T19:39:25Z

@bors r=eddyb

bors · 2017-01-28T19:39:25Z

📌 Commit 468a79d has been approved by eddyb

bors · 2017-01-28T19:39:33Z

⌛ Testing commit 468a79d with merge a576531...

bors · 2017-01-28T20:32:52Z

💔 Test failed - status-appveyor

petrochenkov · 2017-01-29T11:46:09Z

Phew, reproduced locally.

michaelwoerister · 2017-01-30T14:45:23Z

Maybe you have ideas what part of this PR could do this?

I don't see any obvious place. Maybe you could instrument your LLVM to print out which Scope it's complaining about?

petrochenkov · 2017-01-30T19:39:10Z

@michaelwoerister

directory:
filename:
name: Box<std::error::{{impl}}::from::StringError>

petrochenkov · 2017-01-30T20:12:07Z

Found it.

diff --git a/src/librustc_trans/debuginfo/mod.rs b/src/librustc_trans/debuginfo/mod.rs
index f05d48566d..e9468e5663 100644
--- a/src/librustc_trans/debuginfo/mod.rs
+++ b/src/librustc_trans/debuginfo/mod.rs
@@ -400,7 +400,7 @@ pub fn create_function_debug_context<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
                 // Only "class" methods are generally understood by LLVM,
                 // so avoid methods on other types (e.g. `<*mut T>::null`).
                 match impl_self_ty.sty {
-                    ty::TyAdt(..) => {
+                    ty::TyAdt(def, ..) if !def.is_box() => {
                         Some(type_metadata(cx, impl_self_ty, syntax_pos::DUMMY_SP))
                     }
                     _ => None

petrochenkov · 2017-01-30T21:04:59Z

@bors r=eddyb

bors · 2017-01-30T21:05:00Z

📌 Commit 93e3f63 has been approved by eddyb

bors · 2017-01-31T00:02:20Z

⌛ Testing commit 93e3f63 with merge 8e9e055...

@eddyb

Refactoring TyBox -> TyAdt r? @eddyb cc @Manishearth

bors · 2017-01-31T02:31:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 8e9e055 to master...

rust-highfive assigned eddyb Jan 21, 2017

eddyb reviewed Jan 21, 2017

View reviewed changes

petrochenkov force-pushed the nobox branch from 421a721 to 8d36b1c Compare January 28, 2017 00:18

eddyb approved these changes Jan 28, 2017

View reviewed changes

petrochenkov force-pushed the nobox branch from 8d36b1c to f6be8fa Compare January 28, 2017 12:23

petrochenkov added 4 commits January 30, 2017 23:14

Merge ty::TyBox into ty::TyAdt

ffba0ce

Implement Drop for Box

4a4f8ff

cleanup: Refactor away DtorKind

a5d725c

Fix debuginfo scope issue with Box

93e3f63

petrochenkov force-pushed the nobox branch from 468a79d to 93e3f63 Compare January 30, 2017 21:04

bors added a commit that referenced this pull request Jan 31, 2017

Auto merge of #39230 - petrochenkov:nobox, r=eddyb

8e9e055

Refactoring TyBox -> TyAdt r? @eddyb cc @Manishearth

bors merged commit 93e3f63 into rust-lang:master Jan 31, 2017

This was referenced Jan 31, 2017

MIR EndRegion Statements (was MIR dataflow for Borrows) #39409

Merged

Add warning for () to ! switch #39009

Merged

Resolve a bunch of fixmes #39384

Merged

choleraehyq mentioned this pull request Feb 3, 2017

Build failure with rustc 1.16.0-dev (0f8a29647 2017-01-28) rust-lang/rust-clippy#1485

Closed

oli-obk mentioned this pull request Feb 9, 2017

drop in Box<Box<T>> as Box<Trait> broken rust-lang/miri#129

Closed

eddyb mentioned this pull request Feb 21, 2017

Nightly/beta regression? Sized + UnwindSafe #40011

Closed

petrochenkov deleted the nobox branch March 16, 2017 19:43

petrochenkov mentioned this pull request Apr 8, 2020

Move str to libcore. #70911

Closed

Refactoring TyBox -> TyAdt #39230

Refactoring TyBox -> TyAdt #39230

Conversation

petrochenkov commented Jan 21, 2017

petrochenkov commented Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Jan 21, 2017 • edited Loading

eddyb commented Jan 21, 2017

petrochenkov commented Jan 28, 2017

eddyb commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

petrochenkov commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

petrochenkov commented Jan 28, 2017

petrochenkov commented Jan 28, 2017

alexcrichton commented Jan 28, 2017

petrochenkov commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

petrochenkov commented Jan 29, 2017

michaelwoerister commented Jan 30, 2017

petrochenkov commented Jan 30, 2017

petrochenkov commented Jan 30, 2017

petrochenkov commented Jan 30, 2017

bors commented Jan 30, 2017

bors commented Jan 31, 2017

bors commented Jan 31, 2017

petrochenkov commented Jan 21, 2017 •

edited

Loading

Manishearth commented Jan 21, 2017 •

edited

Loading