rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). #41349

eddyb · 2017-04-17T21:39:47Z

InteriorUnsafe / interior_unsafe was replaced with a private lang-item Freeze auto trait in libcore.

OwnsDtor / needs_drop was replaced with a specialized traversal that doesn't avoid caching results in case of a cycle, as the only cycles left can only occur in erroneous "types with infinite sizes", references and raw pointers not having destructors. Also, Copy is now checked at every step of the recursion.

r? @nikomatsakis

arielb1 · 2017-04-18T09:13:30Z

src/libcore/marker.rs

+
+unsafe impl Freeze for .. {}
+
+impl<T: ?Sized> !Freeze for UnsafeCell<T> {}


Is a Box<T> freeze?

Yes, as it only contains pointers and PhantomData.

nikomatsakis

I'm concerned about the caching strategy, see comments below. =)

nikomatsakis · 2017-04-18T19:32:08Z

src/librustc/ty/util.rs

+        //
+        // Consider the type as not needing drop in the meanwhile to avoid
+        // further errors.
+        if visited.replace(self).is_some() {


not that I care very much...but why replace(self).is_some() and not insert(self)`?

Also, I think it's worth expanding the comment to clarify that this condition indicates a cycle -- specifically because there is no CACHED flag set. At first I was confused because a "cross-edge" in the graph (i.e., visiting something you already completely visited before) seemed like it would also be true here.

I cargo-culted it from @arielb1, actually.

The cargo cult is if let Some(_) = visited.replace(self). The reason is because I don't remember the sense of HashSet::insert.

Makes sense. I find the sense easy enough to remember, but it is somewhat arbitrary. (I think of it as "return true if you inserted, false if you did a no-op").

nikomatsakis · 2017-04-18T19:37:19Z

src/librustc/ty/util.rs

+    pub fn needs_drop(&'tcx self, tcx: TyCtxt<'a, 'tcx, 'tcx>,
+                    param_env: &ty::ParameterEnvironment<'tcx>) -> bool {
+        if self.flags.get().intersects(TypeFlags::NEEDS_DROP_CACHED) {
+            return self.flags.get().intersects(TypeFlags::NEEDS_DROP);


It seems wrong to me to cache the NEEDS_DROP flag on the type when the property depends on the environment. After all, the same type might be "drop or not drop" depending on the current environment. Consider:

impl<T> Foo<T> { fn method1(&self, t: T) where T: Copy { .. } fn method2(&self, t: T) { .. } }

We do this for the other properties, there's a condition that the type has no type parameters or Self - I didn't invent this scheme.

nikomatsakis · 2017-04-19T12:00:48Z

src/librustc/ty/util.rs

+            }
+        };
+
+        if !self.has_param_types() && !self.has_self_ty() {


OK, I see that we are not caching in the case where it uses parameters. I take back my assertion that the code is wrong, though I definitely think some comments are in order (I'd like to revamp how we do caching of these sorts of properties, but this is ok for now).

However, I think then that the visited set is insufficient. Consider a case like this:

struct Foo<T> { b: Bar<T>, c: Bar<T> } struct Bar<T> { x: T }

Now Bar<T> will not be cached, but it will be considered "visited". So then when we visit the next field, instead of computing true for "needs-drop", we get back false. Maybe it doesn't matter because we are always taking the union of all results, so if we get back true, we'll always get true back for the overall structure?

I'd prefer to see either (a) the entry in the set removed when we return (or else changed to a map that remembers the final result) or (b) some comments written around that check above that document the rather subtle reasons why a set is sufficient (it doesn't matter if we spuriously return false so long as we returned true at some point in the past).

The result is completely irrelevant as no structural cycle is valid Rust code, as mentioned by:

// This should be reported as an error by `check_representable`

Oh, thinking about this some more: we can actually cache a lot of cases, even if they have parameters (for example, Box<T> needs drop). So we could store a result that is true/false along with a separate boolean for whether the result is cachable (did we use the environment to derive it?). We'd have to propagate that value back up, though, and it may not be worth it. =)

That sounds like a very good reason for having everything expressed in terms of traits and doing that tracking automatically ;).

The result is completely irrelevant as no structural cycle is valid Rust code, as mentioned by:

My example did not have a structural cycle. Repeated:

struct Foo<T> { b: Bar<T>, c: Bar<T> } struct Bar<T> { x: T }

Note that Bar<T> does not reach Foo<T>, just T.

Uh oh I can't read. Yeah that logic was only valid before I expanded it to not always cache.
I'll turn that detection into a "set-backed stack" then.

nikomatsakis

The latest diffs look reasonable to me.

nikomatsakis · 2017-04-20T16:53:21Z

@bors r+

bors · 2017-04-20T16:53:22Z

📌 Commit 99c3579 has been approved by nikomatsakis

bors · 2017-04-20T17:17:19Z

⌛ Testing commit 99c3579 with merge 4d9385f...

bors · 2017-04-20T17:33:55Z

💔 Test failed - status-appveyor

eddyb · 2017-04-20T17:47:17Z

@bors r=nikomatsakis

bors · 2017-04-20T17:47:18Z

📌 Commit 6528f11 has been approved by nikomatsakis

@nikomatsakis

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). `InteriorUnsafe` / `interior_unsafe` was replaced with a private lang-item `Freeze` auto trait in libcore. `OwnsDtor` / `needs_drop` was replaced with a specialized traversal that *doesn't* avoid caching results in case of a cycle, as the only cycles left can only occur in erroneous "types with infinite sizes", references and raw pointers not having destructors. Also, `Copy` is now checked at every step of the recursion. r? @nikomatsakis

@nikomatsakis

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). `InteriorUnsafe` / `interior_unsafe` was replaced with a private lang-item `Freeze` auto trait in libcore. `OwnsDtor` / `needs_drop` was replaced with a specialized traversal that *doesn't* avoid caching results in case of a cycle, as the only cycles left can only occur in erroneous "types with infinite sizes", references and raw pointers not having destructors. Also, `Copy` is now checked at every step of the recursion. r? @nikomatsakis

@nikomatsakis

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). `InteriorUnsafe` / `interior_unsafe` was replaced with a private lang-item `Freeze` auto trait in libcore. `OwnsDtor` / `needs_drop` was replaced with a specialized traversal that *doesn't* avoid caching results in case of a cycle, as the only cycles left can only occur in erroneous "types with infinite sizes", references and raw pointers not having destructors. Also, `Copy` is now checked at every step of the recursion. r? @nikomatsakis

bors · 2017-04-21T11:14:14Z

⌛ Testing commit 6528f11 with merge 69b19db...

arielb1 · 2017-04-21T11:28:03Z

error[E0522]: definition of an unknown language item: `freeze`.
  --> C:\projects\rust\src/rtstartup\rsbegin.rs:38:1
   |
38 | trait Freeze {}
   | ^^^^^^^^^^^^^^^
error: aborting due to previous error

@bors r- retry

arielb1 · 2017-04-21T11:35:14Z

I think you need to add .arg("--cfg").arg(format!("stage{}", compiler.stage)) there.

eddyb · 2017-04-21T12:48:52Z

@bors r=nikomatsakis

bors · 2017-04-21T12:48:52Z

📌 Commit 89bd3f3 has been approved by nikomatsakis

bors · 2017-04-21T14:02:43Z

⌛ Testing commit 89bd3f3 with merge 5695c3e...

@nikomatsakis

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). `InteriorUnsafe` / `interior_unsafe` was replaced with a private lang-item `Freeze` auto trait in libcore. `OwnsDtor` / `needs_drop` was replaced with a specialized traversal that *doesn't* avoid caching results in case of a cycle, as the only cycles left can only occur in erroneous "types with infinite sizes", references and raw pointers not having destructors. Also, `Copy` is now checked at every step of the recursion. r? @nikomatsakis

bors · 2017-04-21T16:36:38Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 5695c3e to master...

eddyb requested a review from nikomatsakis April 17, 2017 21:39

rust-highfive assigned nikomatsakis Apr 17, 2017

arielb1 reviewed Apr 18, 2017

View reviewed changes

eddyb force-pushed the ty-contents branch from d68606b to de91d7a Compare April 18, 2017 10:09

nikomatsakis requested changes Apr 18, 2017

View reviewed changes

nikomatsakis reviewed Apr 19, 2017

View reviewed changes

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 19, 2017

eddyb added 4 commits April 20, 2017 14:39

rustc: simplify TypeContents drastically.

3f5c311

rustc: replace interior_unsafe with a Freeze trait.

6563374

rustc: replace TypeContents::needs_drop with Ty::may_drop.

9ad3b94

rustc: combine type_needs_drop_given_env and may_drop into needs_drop.

0adfd81

eddyb force-pushed the ty-contents branch from de91d7a to 99c3579 Compare April 20, 2017 11:45

nikomatsakis approved these changes Apr 20, 2017

View reviewed changes

eddyb added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 20, 2017

eddyb force-pushed the ty-contents branch from 99c3579 to 6528f11 Compare April 20, 2017 17:46

frewsxcv mentioned this pull request Apr 20, 2017

Rollup of 4 pull requests #41428

Closed

frewsxcv mentioned this pull request Apr 21, 2017

Rollup of 4 pull requests #41436

Closed

TimNN mentioned this pull request Apr 21, 2017

Rollup of 8 pull requests #41440

Closed

TimNN mentioned this pull request Apr 21, 2017

Rollup of 7 pull requests #41441

Closed

Update #[no_core] users with the "freeze" lang item.

89bd3f3

eddyb force-pushed the ty-contents branch from 6528f11 to 89bd3f3 Compare April 21, 2017 12:48

bors merged commit 89bd3f3 into rust-lang:master Apr 21, 2017

eddyb deleted the ty-contents branch April 21, 2017 17:07

dwrensha mentioned this pull request Apr 23, 2017

update for latest rustc nightly rust-lang/miri#159

Merged

kennytm mentioned this pull request Nov 16, 2017

Tracking issue for RFC 1861: Extern types #43467

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). #41349

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). #41349

eddyb commented Apr 17, 2017

arielb1 Apr 18, 2017

eddyb Apr 18, 2017

nikomatsakis left a comment

nikomatsakis Apr 18, 2017

eddyb Apr 18, 2017

arielb1 Apr 19, 2017

nikomatsakis Apr 19, 2017

nikomatsakis Apr 18, 2017

eddyb Apr 18, 2017

nikomatsakis Apr 19, 2017

eddyb Apr 19, 2017

nikomatsakis Apr 19, 2017

eddyb Apr 19, 2017

nikomatsakis Apr 19, 2017 •

edited

Loading

eddyb Apr 19, 2017

nikomatsakis left a comment

nikomatsakis commented Apr 20, 2017

bors commented Apr 20, 2017

bors commented Apr 20, 2017

bors commented Apr 20, 2017

eddyb commented Apr 20, 2017

bors commented Apr 20, 2017

bors commented Apr 21, 2017

arielb1 commented Apr 21, 2017

arielb1 commented Apr 21, 2017

eddyb commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017


		unsafe impl Freeze for .. {}

		impl<T: ?Sized> !Freeze for UnsafeCell<T> {}

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). #41349

rustc: replace TypeContents with two independent properties (is_freeze / needs_drop). #41349

Conversation

eddyb commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a 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

nikomatsakis Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Apr 20, 2017

bors commented Apr 20, 2017

bors commented Apr 20, 2017

bors commented Apr 20, 2017

eddyb commented Apr 20, 2017

bors commented Apr 20, 2017

bors commented Apr 21, 2017

arielb1 commented Apr 21, 2017

arielb1 commented Apr 21, 2017

eddyb commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017

bors commented Apr 21, 2017

nikomatsakis Apr 19, 2017 •

edited

Loading