RFC: loadable lints

[kmcallister]

No description provided.

[mcpherrinm]

My first concern is that different types of lints may want to run at different phases of compilation. We could probably extend the #[phase(foo)] stuff to make this work, but we should make sure that what we choose right now is the right name so we don't have to make all the existing ones rename, if possible.

For example, some lints may want to run before macro expansion, after macro expansion, after identifiers are resolved, before or after borrow checking.... (I'm not an expert in rustc internals, so I'm not sure entirely how phases are organized; my examples may be bad)

[huonw]

I don't have time to read right now, but:

• 👍 for loadable lints
• I really want lints to run in phases where each lint runs as early as it can (i.e. when it has all the information that it needs), so that they are useful as possible. Presumably loadable lints would like this too, but at the very least, loadable lints shouldn't interfere with this refactoring. (For example, many lints, such as the camel-case types one, only need the raw AST and so can/should run before type checking/match checking.) (edit: this is the same as what @mcpherrinm said.)

[lilyball]

I think it should. Without a crate filename, it would look like it does today. With a crate filename it could then have a second block of lints that will be used when compiling that crate.

[kmcallister]

@mcpherrinm: I don't think #[phase(foo)] is the right place for that, since it's just telling the compiler to dlopen that crate. Maybe something on a per-lint basis, like

reg.register_lint("letter_e",
    lint::warn,
    lint::AfterTypeCheck,
    box Lipogram as Box<Lint>);

[mcpherrinm]

My second concern is that if you have a lot of lints, calling check_item (or check_expr or check_fn or whatever) for N lints on M nodes could be a lot of extra work.

Some sort of more intelligent invocation of lints may be of interest. But that's a problem for the future, since we'll certainly want the option of examining every expr or whatever. I'm not sure what that looks like right now: CSS selectors are what pop into my head right now.

[lilyball]

I am a huge fan of loadable lints. I agree with @huonw, I'd love to see lints be able to run at different times, so they can flag things earlier.

I'm also concerned that there's no way to require the use of a lint when using a particular library. For my rust-lua use-case, I need to require the lint to be used, and set it to forbid (so it can't be overridden).

This isn't a hard requirement, because I have a macro that's used to define the exported Rust functions and those functions are where I really need this lint. So if the user has the macro, they'll presumably have the lint too (unless we split #[phase] into 3, so you can opt for macros without lints, but I don't see the benefit in doing that). But it would be nice if I could still enforce this somehow for clients that don't use the macro, because they can still build exported Rust functions themselves, it's just more complicated.

[lilyball]

The only way I can think of to deal with that, though, is to have some sort of crate attribute that requires clients of the crate to pull in its lints too, and not just link against it.

[kmcallister]

I was talking with @lifthrasiir about inferring phase for an extern crate by opening the crate and looking at its metadata. I think it is useful to let the user override with an explicit phase(...) attribute, but we could also let the library forbid the override :)

I think it makes sense to design phase inference and mandatory lints at the same time.

@kballard: A mandatory, always-forbid lint seems really aggressive, when the user can still do arbitrary bad things inside unsafe { ... }. Mandatory default-deny makes sense to me.

[kmcallister]

Actually that raises another interesting possibility: Lint levels like

• allow in unsafe, deny otherwise
• allow in unsafe, warn otherwise
• warn in unsafe, deny otherwise

This seems orthogonal to loadable lints, but it might become much more useful in that context.

[lilyball]

@kmcallister The particular lint I want doesn't actually make sense to have configurable levels. If you need to do something unsafe, the library already provides a facility to get an unsafe API to do it. The whole point of the lint is to ensure that the safe API is in fact safe.

[brson]

Discussed at https://github.com/rust-lang/rust/wiki/Meeting-weekly-2014-06-17#rfc-pr-89-loadable-lints-httpsgithubcomrust-langrfcspull89-.

There continues to be wide discomfort about the user-visible mechanisms involved in our plugin system, but agreement that we want loadable lints, and begrudging acceptance that we should continue down this feature-gated path to get the functionality we need.

Accepted as RFC 29.