RFC: Rearchitect the attribute-usage lint

[sfackler]

No description provided.

[huonw]

I like this, but being able to modify used would make the AST non-immutable. Maybe it would be better as a side table?

[sfackler]

It seems like it'd be a pain to properly link things in the side table. A Cell<bool> would be easier but maybe a bit immoral.

[alexcrichton]

This sounds like a great idea to me, but I agree with @huonw that a side table may be the way to go for now (it's what almost everything else uses).

Things like unused-unsafe, unused-mut, etc, could also all use Cell<bool>. It would be nice to keep the AST Share I think to leave open parallel opportunities in the future (sharing the AST in parallel).

[sfackler]

That seems reasonable to me. I think this will require a bit more infrastructure than unused_unsafe & co. since we can't use NodeIds for the mapping and the side table has to be accessible in all stages of compilation. Maybe toss it in a task-local like we do for the ident interner?

[huonw]

The parallelism was my concern too, although I guess Cell is still Isolate, so maybe it wouldn't be that bad? (Even so, I would still prefer keeping it Share.)

(Doesn't TLD go against parallelism too? I guess the ident interner needs to change for parallelism, anyway.)

[huonw]

we can't use NodeIds for the mapping

Why not? (We can either give attributes nodeids, or have a special attribute id doing a similar thing.)

the side table has to be accessible in all stages of compilation

I don't think it has to be the side table: just each stage has to have access to a side table, and then the lint has to look at all of them (or just merge all the tables before doing any look-ups).

[sfackler]

NodeIds aren't assigned until after expansion, but expansion needs to be able to mark attributes as used. Using a separate ID system for attributes will be straightforward (though yet more task local state for the counter!)

[huonw]

Oh, of course. it doesn't have to be task-local at all: the parser can just keep track of it... (Although then we hit the problems that caused us to reassign nodeids like we do now: macro expansion has to be careful reassign everything or things end up with duplicated ids.)

[huonw]

Well... actually maybe that's not so much of a problem for attributes. In fact, maybe duplicated IDs is exactly what we want, so that we don't get "spurious" warnings, where one expansion of a macro legitimately uses some attribute but another doesn't.

[sfackler]

I would want to minimize the amount of anti-"spurious" warning logic because it won't work in all cases. It seems to me that it's a macro's responsibility to mark all the attributes it's okay with as used even if it isn't using them at that point. There are cases I can imagine like "don't use this attribute unless you've already used this attribute on the struct" or something that should still get attribute usage warnings.

[nikomatsakis]

Some thoughts:

1. I am strongly opposed to add internal mutation into the AST and would prefer a side-table.
2. If the primary goal is to make the whitelist more extensible, we can add the ability to query the loaded extensions for the names (and suggested usage locations) of attributes and incorporate them into the whitelist.
3. If we had a system more like Java, where attributes were declared, we could also declare where they are expected to be used (as Java does). This would address this problem in a more declarative way. On the other hand, I think nobody has the stomach to invent such a system pre-1.0 and I guess that after that it may be too late (well, maybe not, we could just permit any attribute but have a lint -- defaulting to error -- for use of "undeclared" attributes).

[sfackler]

I'd like to avoid the whitelist approach entirely since it won't be able (I don't think) warn in cases like this, where the attribute is valid but ignored since it's missing the attribute on the Item that actually does the work:

#[feature(phase)];
#[phase(syntax, link)]
extern crate some_orm;

// #[ormify]
pub struct Foo {
    #[column(foo_)]
    #[primary_key]
    foo: int
}

I think the Java-style approach would also run into this.

How does this sound:

pub type AttrId = uint; // u32?

pub struct Attribute_ {
    id: AttrId,
    style: AttrStyle,
    value: @MetaItem,
    is_sugared_doc: bool,
}

We can adjust the ParseSess to generate AttrIds. We could alternatively make AttrId an opaque struct to enforce that they're created the right way if we wanted.

Do people have preferences on the storage location of the side table? A task-local store would be the most convenient, and allow for the syntax::attr infrastructure to automatically mark attributes when they're searched for without API changes (that may not be a good thing). It shouldn't pose a parallelization issue since each task can have its own table and the compiler can take the union of them for the lint pass. We could also pass a table explicitly to all compilation stages that need it.

[sfackler]

I've updated the RFC to describe the side table approach.

[nikomatsakis]

Can we make AttrId a newtype?

You are correct that any Java-style scheme is going to be more limited than what we can achieve with custom coding. Talking it over in the meeting I actually kind of came around -- an imperative scheme is more flexible, and we could probably layer a more declarative scheme on top if desired.

[sfackler]

Making AttrId a newtype seems like a good idea. Should we make it entirely opaque (pub struct AttrId { priv val: uint }) or is that excessive?

[sfackler]

Updated with AttrId as a newtype.

[sfackler]

I do think that a Java-style scheme would be worth thinking about as well, as this proposal won't handle something I've run into recently:

#[deriving(Ord(a, c))]
pub struct Foo {
    a: int,
    b: int,
    c: int
}

I thought the (a, c) part would customize the Ord impl, but it's actually silently ignored.

We could potentially handle that with this proposal by pushing the IDs out from Attribute_ to MetaItem.

[nikomatsakis]

@sfackler isn't that the fault of #[deriving()] for not giving you a warning ?

[sfackler]

It is, but in the same way that it's also the fault of #[deriving] that it's not folding the whole AST to track down uses in places that are ignored. It's probably a lot more reasonable to have #[deriving] handle this extra arguments case though than it would be to force it to do its own AST scan.

[nikomatsakis]

Right. In this case, it's scanning the argument list anyway, I think it's the job of deriving to validate its format.

[alexcrichton]

It sounds like we're all in general agreement that this is the right direction to go in. Details may change along the way, but that's expected from an RFC. I'm going to merge this.

[alexcrichton]

Committed in 1245e64