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.

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

#[may_dangle] attribute #37117

Merged
merged 15 commits into from
Oct 19, 2016
Merged

#[may_dangle] attribute #37117

merged 15 commits into from
Oct 19, 2016

Conversation

pnkfelix
Copy link
Member

#[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
Copy link
Collaborator

r? @nrc

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

@pnkfelix
Copy link
Member Author

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned nrc Oct 12, 2016
///
/// Equivalent to RevisedTy with no change to the self type.
///
/// FIXME: this name may not be general enough; it should be
Copy link
Member Author

Choose a reason for hiding this comment

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

(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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

delete it, I say :)

/me slayer of FIXMEs

@pnkfelix
Copy link
Member Author

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

delete it, I say :)

/me slayer of FIXMEs

/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?!");

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes will fix


struct Foo;

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. (ui test)

#![feature(generic_param_attrs)]
#![feature(dropck_eyepatch)]

// The point of this test is to illustrate that the `#[may_dangle]`
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.)
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

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.)

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> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

I'll add a comment.

@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Oct 18, 2016

📌 Commit 10a58ac has been approved by nikomatsakis

eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016
…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.
eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016
…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.
bors added a commit that referenced this pull request Oct 19, 2016
@bors bors merged commit 10a58ac into rust-lang:master Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants