`#[may_dangle]` attribute #37117

pnkfelix · 2016-10-12T16:53:15Z

#[may_dangle] attribute

Second step of #34761. Last big hurdle before we can work in earnest towards Allocator integration (#32838)

Note: I am not clear if this is also a syntax-breaking change that needs to be part of a breaking-batch.

In particular, as far as I can tell from the error diagnostics, the former test for E0199 was actually a test for E0198, and there was no test for E0198. (I am assuming that one of my previous changes to the `unsafe impl` checking fixed a latent bug in how these two cases were differentiated.)

rust-highfive · 2016-10-12T16:53:28Z

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

pnkfelix · 2016-10-12T16:53:37Z

r? @nikomatsakis

pnkfelix · 2016-10-12T16:57:38Z

src/librustc_typeck/check/dropck.rs

+    ///
+    /// Equivalent to RevisedTy with no change to the self type.
+    ///
+    /// FIXME: this name may not be general enough; it should be


(given the followup note in the paragraph below this FIXME, I'm tempted to just delete this fixme and the comment below it. I think I added the fixme in response to a concern that @arielb1 had raised, but I am no longer sure.)

delete it, I say :)

/me slayer of FIXMEs

pnkfelix · 2016-10-12T21:37:20Z

Hmm travis failed because I need to add appropriate feature gates to the example code in the diagnostic

nikomatsakis · 2016-10-13T13:43:34Z

src/librustc/hir/mod.rs

@@ -95,6 +95,7 @@ impl fmt::Debug for Lifetime {
 pub struct LifetimeDef {
    pub lifetime: Lifetime,
    pub bounds: HirVec<Lifetime>,
+    pub pure_wrt_drop: bool,


seems like a comment would be nice here (and below); perhaps including a link to the RFC

ah, I see there are comments on the ty data structures

nikomatsakis · 2016-10-13T13:45:03Z

src/librustc/infer/error_reporting.rs

@@ -1295,7 +1296,9 @@ impl<'a, 'gcx, 'tcx> Rebuilder<'a, 'gcx, 'tcx> {
        let mut lifetimes = Vec::new();
        for lt in add {
            lifetimes.push(hir::LifetimeDef { lifetime: *lt,
-                                              bounds: hir::HirVec::new() });
+                                              bounds: hir::HirVec::new(),
+                                              pure_wrt_drop: false,


Nit: Not that I care all that much, but the formatting here seems odd. I'd expect to either put the } on this line, or else move lifetime: to the next line, no?

nikomatsakis · 2016-10-13T13:47:09Z

src/librustc_typeck/check/dropck.rs

+    ///
+    /// Equivalent to RevisedTy with no change to the self type.
+    ///
+    /// FIXME: this name may not be general enough; it should be


delete it, I say :)

/me slayer of FIXMEs

nikomatsakis · 2016-10-13T13:47:50Z

src/librustc_typeck/check/dropck.rs

+    /// Assume all borrowed data access by dtor occurs as if Self has the
+    /// type carried by this variant. In practice this means that some
+    /// of the type parameters are remapped to `()`, because the developer
+    /// has asserted that the destructor will not access their contents.


"...and some region parameters mapped to 'static"?

nikomatsakis · 2016-10-13T13:48:53Z

src/librustc_typeck/check/dropck.rs

+            let dtor_method = if let Some(dtor_method) = opt_dtor_method {
+                dtor_method
+            } else {
+                return DropckKind::BorrowedDataMustStrictlyOutliveSelf;


can you put a comment here as to what this else represents? I think the answer is: struct does not implement Drop itself?

Having dug a bit deeper, I think that if the type is_dtorck, then it must have a destructor, so it seems to me that you could do let dtor_method = adt_def.destructor().expect("dtorck type without destructor?!");

nikomatsakis · 2016-10-13T13:52:39Z

src/librustc_typeck/check/dropck.rs

+                                          } else {
+                                              substs.type_for_def(def)
+                                          });
+            let revised_ty = tcx.mk_adt(adt_def, &substs);


This seems wrong. These substs are suitable for the impl, but you are applying them to the type -- I know that we require drop impls and types to have a close correspondence, but we do allow reordering. I think what you want to do instead is to get the self-type from the impl and apply this substitution to it, no?

yes will fix

nikomatsakis · 2016-10-13T13:55:56Z

src/test/compile-fail/E0198.rs

+
+struct Foo;
+
+unsafe impl !Clone for Foo { } //~ ERROR negative implementations are not unsafe [E0198]


Nit: Can you move this to a ui test? I am of the opinion that those are the new hotness now.

I'll look into moving over all the error code checking tests into ui/, but lets make that a different PR maybe? For now I'd prefer to let this fix sit on its own.

nikomatsakis · 2016-10-13T13:56:06Z

src/test/compile-fail/E0199.rs

@@ -12,7 +12,8 @@

 struct Foo;

-unsafe impl !Clone for Foo { } //~ ERROR E0199
+trait Bar { }
+unsafe impl Bar for Foo { } //~ ERROR implementing the trait `Bar` is not unsafe [E0199]


Same here. (ui test)

nikomatsakis · 2016-10-13T13:58:42Z

src/test/compile-fail/auxiliary/dropck_eyepatch_extern_crate.rs

+#![feature(generic_param_attrs)]
+#![feature(dropck_eyepatch)]
+
+// The point of this test is to illustrate that the `#[may_dangle]`


Funny to have this comment in the auxiliary crate, and not the main test.

Also, why not make a ui/dropck_eyepatch/ directory and collect tests there, and then have ui/dropck_eyepatch/auxiliary? (Could also be compile-fail, though I would like to move everything to ui at some point.)

pnkfelix · 2016-10-17T13:28:36Z

Regarding the src/test/ui/ feedback: I freely admit that I was unaware of the introduction of the ui/ subtree.

(My gut instinct is that these tests work better with the expected errors attached inline in the source, rather than having separate files that are blindly auto-generated, but I will go through the exercise of translating the tests to the ui/ structure now, and if I don't see any glaring obvious problems with using it, then I'll probably use ui/ rather than try to stubbornly stick to a position that I don't personally feel strongly attached to...)

…s subst. This addresses issue pointed out by niko that prior code would break if the declaration order for generics does not match how they are fed into the instantiation of the type itself. (Added some tests exercising this scenario.)

nikomatsakis

Looks good to me modulo nits. Mainly the comment. I don't really care about the placement of ->. (Can't wait till the glorious rustfmt future.)

nikomatsakis · 2016-10-17T17:39:46Z

src/librustc_typeck/check/dropck.rs

+fn revise_self_ty<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
+                                  adt_def: ty::AdtDef<'tcx>,
+                                  impl_id: DefId,
+                                  substs: &Substs<'tcx>) -> Ty<'tcx> {


Nit: not that I care all that much, the -> goes on the next line usually, no?

Well, at least one other fn definition in this file uses this style (I think I may have even double checked about that when I first wrote this helper), but I'm not wedded to the style.

nikomatsakis · 2016-10-17T17:45:24Z

src/librustc_typeck/check/dropck.rs

+
+    // Walk `substs` + `self_substs`, build new substs appropriate for
+    // `adt_def`; each non-dangling param reuses entry from `substs`.
+    let substs = Substs::for_item(


Egads. =) I see why you warned me this was kind of gross. But I see the logic in it and don't immediately see a better way to do it. I guess it only makes sense because of the limitations we put onto drop impls in the first place, right? Otherwise things like impl<#[may_dangle] A> Drop for Foo<Vec<A>> wouldn't work -- more specifically, the attribute would have no effect, right? -- but we don't support that anyhow. If you agree, I think it is worth leaving a comment in here to this effect.

Yes, this technique is crucially relying on details of how we constrain Drop impls.

I'll add a comment.

pnkfelix · 2016-10-18T12:09:46Z

@bors r=nikomatsakis

bors · 2016-10-18T12:09:47Z

📌 Commit 10a58ac has been approved by nikomatsakis

…sakis `#[may_dangle]` attribute `#[may_dangle]` attribute Second step of rust-lang#34761. Last big hurdle before we can work in earnest towards Allocator integration (rust-lang#32838) Note: I am not clear if this is *also* a syntax-breaking change that needs to be part of a breaking-batch.

Rollup of 23 pull requests - Successful merges: #36964, #37108, #37117, #37124, #37161, #37176, #37182, #37193, #37198, #37202, #37208, #37218, #37221, #37224, #37230, #37231, #37233, #37236, #37240, #37254, #37257, #37265, #37267 - Failed merges: #37213, #37220, #37261

pnkfelix added 7 commits October 10, 2016 15:27

Include attributes on generic parameter bindings in pretty printer.

b0eee76

Add feature gate for dropck_eyepatch feature (RFC 1327).

4bb68be

Thread pure_wrt_drop field through lifetime and type parameters.

e8ccc68

Code for enforcing #[may_dangle] attribute.

7d2d5bc

Require destructors using #[may_dangle] to use unsafe impl.

9a649c3

tests for #[may_dangle] attribute.

fa7f69c

rust-highfive assigned nrc Oct 12, 2016

rust-highfive assigned nikomatsakis and unassigned nrc Oct 12, 2016

pnkfelix commented Oct 12, 2016

View reviewed changes

pnkfelix added 2 commits October 12, 2016 23:39

placate diagnostic checking tests by fixing sample code in E0569.

8dd9493

fix typo in diagnostic sample code

c239fee

nikomatsakis approved these changes Oct 13, 2016

View reviewed changes

pnkfelix added 5 commits October 17, 2016 16:14

remove fixme that i no longer think appropriate.

818ac08

Review feedback: add linebreak and reindent to make braces match better.

85d2e4d

Review feedback: expand comment and simplify some code.

0d8f716

Moved new dropck-eyepatch compile-fail tests to the ui/ subtree.

4124d8e

nikomatsakis approved these changes Oct 17, 2016

View reviewed changes

Incorporate review feedback: code formatting fixes, expand a comment.

10a58ac

eddyb mentioned this pull request Oct 18, 2016

Rollup of 16 pull requests #37262

Closed

eddyb mentioned this pull request Oct 19, 2016

Rollup of 23 pull requests #37269

Merged

bors merged commit 10a58ac into rust-lang:master Oct 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`#[may_dangle]` attribute #37117

`#[may_dangle]` attribute #37117

pnkfelix commented Oct 12, 2016

rust-highfive commented Oct 12, 2016

pnkfelix commented Oct 12, 2016

pnkfelix Oct 12, 2016

nikomatsakis Oct 13, 2016

pnkfelix commented Oct 12, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

pnkfelix Oct 13, 2016

nikomatsakis Oct 13, 2016

pnkfelix Oct 17, 2016

nikomatsakis Oct 13, 2016

nikomatsakis Oct 13, 2016

pnkfelix commented Oct 17, 2016

nikomatsakis left a comment

nikomatsakis Oct 17, 2016

pnkfelix Oct 18, 2016

nikomatsakis Oct 17, 2016

pnkfelix Oct 18, 2016

pnkfelix commented Oct 18, 2016

bors commented Oct 18, 2016


		struct Foo;

		unsafe impl !Clone for Foo { } //~ ERROR negative implementations are not unsafe [E0198]

#[may_dangle] attribute #37117

#[may_dangle] attribute #37117

Conversation

pnkfelix commented Oct 12, 2016

rust-highfive commented Oct 12, 2016

pnkfelix commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Oct 12, 2016

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

pnkfelix commented Oct 17, 2016

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

pnkfelix commented Oct 18, 2016

bors commented Oct 18, 2016

`#[may_dangle]` attribute #37117

`#[may_dangle]` attribute #37117