Added lint for #[deriving] structs and enums with unsafe pointers

[pongad]

Fixes #13032

[huonw]

Awesome work! (I'll review.)

(Don't worry about the travis failure, it seems unrelated.)

[huonw]

This line is too long; maybe you could factor out the error message to a static like

fn check_raw_pointer_deriving(...) {
    static MSG: &'static str = "use of `#[deriving]` with a raw pointer";

    fn check_struct_def(...) {
         /* ... */
                 ast::TyPtr(..) => cx.span_lint(RawPointerDeriving, field.span, MSG)
         /* ... */
     }
     /* ... */
}

(Even with this factored out the line below will be too long, you can break it over multiple lines via

ast::TyPtr(..) => {
    cx.span_lint(RawPointerDeriving, variant.span, MSG);
}

[huonw]

Oh, also, this would preferably point to the type itself, i.e., field.node.ty.span, rather than field.span. (And below with var_arg.ty.span.)

[huonw]

Looks pretty good; although I have some comments, and this needs some tests, example; it should be a test that includes the for casts: structs, tuple structs, enums and struct enum variants, i.e.

#[feature(struct_variants)];
#[allow(dead_code)];
#[deny(raw_pointer_deriving)];

#[deriving(Clone)]
struct Foo {
     x: *int //~ ERROR #[deriving] with raw pointer
}

#[deriving(Clone)]
struct Bar(*mut int); //~ ERROR #[deriving] with raw pointer

enum Baz {
     A(*int),  //~ ERROR #[deriving] with raw pointer
     B { x: *mut int }  //~ ERROR #[deriving] with raw pointer
}
fn main() {}

[huonw]

Oh something I just thought about: I guess this really needs to walk the types, not just look at their outer layers, e.g. the following should probably be warned about

struct Foo {
    x: (*int, *uint)
}

This could be done by creating a new Visitor ("visits" the AST by walking the tree) just for this, like

struct RawPtrDerivingVisitor<'a> {
     cx: &'a mut Context
}
impl<'a> Visitor<()> for RawPtrDerivingVisitor<'a> {
     fn visit_ty(&mut self, ty: &ast::Ty, _: ()) {
           /* check `ty` is a TyPtr, if so, span_lint on this type */

           // recurse, to walk the interiors of other types
           visit::walk_ty(self, ty, ());
     }
     // explicit override to a no-op these to reduce code bloat
     fn visit_expr(&mut self, e: &ast::Expr, _: ()) {}
     fn visit_block(&mut self, e: &ast::Block, _: ()) {}
}

fn check_raw_ptr_deriving(cx: &mut Context, item: &Item) {
     // ... do the attribute check ...
     match item {
          ItemStruct(..) | ItemEnum(..) => {
               let mut visitor = RawPtrDerivingVisitor { cx: cx };
               visit::walk_item(&mut visitor, item, ());
          }
     }
}

This has a few advantages:

• firstly, it walks the types, so that the tuple example above will warn.
• Secondly, If you look at the definition of the Visitor trait in libsyntax/visit.rs you can see that it's using default methods to call various walk_... functions. This means that the ones that we don't override explicitly in the impl will have the definition from the trait, and hence will automatically manage iterating over fields and variants etc.

(Sorry for changing to a completely different tack to the one I talked to you about on IRC. :( )

[pongad]

@huonw I'll work on these the next time I have free time :D. By the way, if I have structs A and B, both of which contain struct X. Will X be visited once or three times (X itself, through A, then through B)? If ti's three times, we might get exponential run time... I don't know enough about Visitor to tell.

[huonw]

By the way, if I have structs A and B, both of which contain struct X

Oh, sorry, I wasn't clear: it's just visiting the AST itself, not resolving the definitions and then visiting those too, so for something like

struct X { r: int }

struct A { s: X, t: *int }
struct B { u: Option<*uint>, v: X, w: A }

A visitor would, in order (sorry for the hard-to-read formatting, I've tried to write it to look like the tree of calls that the visitor would perform):

• visit X
  • visit the field r
    • visit the type int, seeing a TyPath where the Path is int
• visit A
  • visit s
    • visit X, seeing a TyPath where the Path is X
  • visit t
    • visit *int, seeing a TyPtr(..)
• visit B
  • visit u
    • visit Option<*uint>, seeing a TyPath with a type parameter where the Path is Option
      • visit *uint, seeing a TyPtr(..)
  • visit v
    • visit X, seeing a TyPath where the Path is X
  • visit w
    • visit A, seeing a TyPath where the Path is A

(enums are similar, except with an extra layer to visit each variant too.)

That is, the visitor only visits things written directly inline in the struct and enum definitions, not "following" type signatures to see what they refer to. If one does want a visitor that follows definitions, one has to work out what the Paths are referring to manually. But...

The "shallow-visiting" is exactly what we want here: we assume that raw pointers that are "hidden" inside some other struct/enum definitions are managed by a correct Clone, Eq, ... implementation (and #[deriving] is just deferring to those implementations). It's only those * pointers exposed directly to #[deriving] that matter.

(This new scheme does create a some rare false positives, for example:

#[deriving(Clone)]
struct Foo {
    x: Rc<*int>
}

The clone implementation for Rc doesn't clone its interior, it just increases the reference count. I.e. one isn't actually touching the *int when calling clone. However, I think these will be sufficiently rare that people can just tell the compiler "I know what I'm doing" with an #[allow(raw_pointer_deriving)] attribute where necessary.)

[pongad]

Fixed!