Generalize #[macro_registrar] to #[plugin_registrar]

[kmcallister]

This implements the design in rust-lang/rfcs#86. It shouldn't be merged until that RFC is accepted, but it would be great if somebody has time to review the code before then.

[alexcrichton]

Having syntax extensions forcibly depend on librustc seems to go against one of the major goals of refactoring syntax out of rustc, as well as the current design. I didn't see any wording about this in the RFC, can you explain why this dependency has been picked up?

[kmcallister]

This was discussed mainly in @kballard's comment and then on IRC (linked on the RFC thread too).

The issue is that we want to register all plugins through a single interface, so that interface needs to use types from both libsyntax (for syntax extensions) and librustc (for lints etc.). Putting it in librustc was the simplest way to accomplish this.

It makes sense to me that syntax extensions are rustc plugins not libsyntax plugins. You're telling rustc::driver to open some crates, read their metadata, and pass some of the results to libsyntax.

Metadata is defined within librustc; the old implementation only worked because of a CrateLoader trait defined in libsyntax and implemented by librustc. A third-party user of libsyntax would need to implement this trait, as the old test cases in expand.rs did. This isn't too different from the new situation, where you pass a vector of exported macros and a vector of syntax extensions to expand_crate.

I'm not against moving plugin stuff to its own crate (along with metadata reading?) and implementing something like @kballard's type-keyed map to break the unconditional dependency on librustc and libsyntax. But I don't think that design work should block landing plugins and loadable lints. Plugins are feature-gated and the interface can continue to evolve as we get a better idea what external users of libsyntax want.

[alexcrichton]

By introducing a dependency on librustc, we're essentially exposing the entire compiler as the interface that syntax extensions have to work with. It's bad enough exposing all of libsyntax where it itself doesn't have a stable interface.

I don't think I understand the use case for depending on librustc to have a unified interface for registering plugins. Does this patch currently have examples of how a syntax extension now depends on librustc? The cited example is lints, but I can imagine a world where pluggable lints work without librustc.

I think the idea of moving the syntax extension interface to its own crate is a great one because it can be a concrete stable API for future syntax extensions. That said, it probably won't happen for some time.

Also, while I agree that this is all feature gated and everything, it doesn't mean that we should throw caution to the wind and accidentally take steps backwards. We should always consider the future direction to go in, regardless of whether it is a large guiding presence at this time or not. I'm not saying that this should block for the perfect design, but "we want this now, not later" doesn't mean that we should basically accept anything that works.

[kmcallister]

By introducing a dependency on librustc, we're essentially exposing the entire compiler as the interface that syntax extensions have to work with.

I don't think so. It's already the case that syntax extension crates can link librustc if they want to. There's a convention that you don't do so. And in the new world there's a convention that you don't use modules outside of rustc::plugin. We don't enforce either convention, but we don't yet ensure the #[macro_registrar] function has the right type, either.

If you think that convention should be documented more explicitly, I can certainly do that.

It's bad enough exposing all of libsyntax where it itself doesn't have a stable interface.

Yeah, that's part of why I don't think this is much of a regression. We're not currently getting some huge benefit from a stable, clean libsyntax API. In fact there's no way to load syntax extension crates at all without importing the metadata parsing from librustc or reimplementing it yourself.

I don't think I understand the use case for depending on librustc to have a unified interface for registering plugins.

Take a look at my lint_prototype branch and the demo. (This is not merge-quality code! I'm working on implementing it "for real" right now.) A #[plugin_registrar] gets a Registry; a Registry has methods for registering lints; a lint depends on librustc types. So writing a #[plugin_registrar] requires linking librustc to get Registry, unless we do tricks like @kballard proposed. Which, again, I'm not against doing, but I don't consider it necessary for v1 of this feature.

Nothing in this branch requires Registry to be defined within librustc. The whole point of the branch is to clear the way for future work which will. But this isn't premature overgeneralization; you can already see the necessity in the prototype above.

I can imagine a world where pluggable lints work without librustc

Maybe some of them, but a lot of the existing lints talk to the typechecker, for example. The lint I want to write for Servo will need to know the inferred types of let variables to ensure they don't contain JS<T>. Preventing pluggable lints from talking to the typechecker would severely limit the utility of the feature.

Also, while I agree that this is all feature gated and everything, it doesn't mean that we should throw caution to the wind and accidentally take steps backwards.

This isn't accidental; it was discussed on the RFC thread and on IRC and again by us here.

"we want this now, not later" doesn't mean that we should basically accept anything that works

Sure, and of course I agree in general. We're just disagreeing on whether this particular tradeoff is acceptable or not. I think it is, because we're not losing any major capability that we have today. Getting those capabilities would require a stable libsyntax API, as well as factoring out metadata reading into its own crate. If somebody is doing that work, it's not much marginal work to also move the plugin registrar at that time. It is an (easily resolved) breaking change for user code, and that's why the feature-gated aspect is important.

[huonw]

Running at this phase implies that a macro cannot load new plugins internally, right?

[kmcallister]

Ah, do you mean something like

#[macro_export]
macro_rules! foo ( () => (
    #[phase(plugin)] extern crate log;
));

That's true, this wouldn't work as an exported macro. I hadn't thought of that.

Do you think there is a compelling use case for this pattern or is it an acceptable limitation? The alternative I guess is to alternate between expansion and plugin loading until a fixed point is reached.

[huonw]

We could just have three passes: load, expand, load again, meaning macros can introduce lints etc., but not other macros.

(I think this is actually close to the only thing that works, since any macro that's defined in a crate loaded from another macro will be undefined on the first expansion run... unless additional plugins are loaded while expanding.)

The approach you have here is certainly the most conservative, and should work for now. I guess other approaches should go through an RFC.

[kmcallister]

Rebased.

[kmcallister]

Sorry about the repeated breakage. I was having trouble running make check before, but I can do it now and this branch passes.