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

RFC: Generalize #[macro_registrar] to #[plugin_registrar] #86

Merged
merged 4 commits into from Jun 4, 2014

Conversation

Projects
None yet
7 participants
@kmcallister
Copy link
Contributor

commented May 22, 2014

No description provided.

@mcpherrinm

This comment has been minimized.

Copy link

commented May 22, 2014

This looks basically sane to me -- In particular, it allows for future new types of plugins to be added in a backwards-compatible way.

We could imagine adding new types of plugins in the future, like optimization passes on the AST, or ones that instrument code, or provide different types of output during compilation.

If generalizations of existing passes are added, the current methods on the registry could be transparently implemented in terms of the new ones, so the design should be a reasonably future-proof way to interact with rustc.

The only real concern I have is that a proliferation of compiler plugins may make creating a new compiler, interpreter, or other external static analysis tools hard or impossible to write.

@lilyball

This comment has been minimized.

Copy link
Contributor

commented May 22, 2014

I am very much in favor of this RFC. Anything to make loadable lints a reality.


Naming bikeshed.

Should `Registry` be provided by `libsyntax`, when it's used to register more than just syntax extensions?

This comment has been minimized.

Copy link
@lilyball

lilyball May 22, 2014

Contributor

Good question. Whatever library provides it will need to be linked by all syntax extensions. If it stays in libsyntax, that seems odd for a lint registry, although lints presumably need to use the AST and therefore need to link against libsyntax anyway.

Of course, supporting lints also poses a problem. I haven't really looked at them before, but they apparently use a Context type defined in librustc. Since librustc links against libsyntax, libsyntax can't very well link against librustc. Extracting the registry to a separate library doesn't help, because it would have to link against librustc to get the same Context type, and librustc would have to link against it.

I see a few options here:

  1. Go with the alternative of a separate registrar per type. That's kind of crappy though.
  2. Extract rustc::middle::lint::Context out of librustc and put it in libsyntax or a brand new library. This probably isn't doable, it has a few other librustc types embedded in it.
  3. Define the registry in a separate library and use trait magic to make it generic. Both librustc and libsyntax can then provide trait implementations for lints and syntax extensions.

The third is the most complicated, but allows this little library to stand on its own, and for syntax extensions to only link against either libsyntax or librustc as needed.

@lilyball

This comment has been minimized.

Copy link
Contributor

commented May 22, 2014

Regarding the 3rd option in my above comment, I'm thinking of an API that looks vaguely like the following (inspired by std::local_data):

// libregistry/lib.rs
pub struct Registry { ... }

impl Registry {
    pub fn get<'a, V: Default>(&'a mut self, name: RegistryName<V>) -> &'a mut V { ... }
    // V: Default is because the registry value type needs to be created somehow
}

pub type RegistryName<V> = &'static RegistryValue<V>;
pub struct RegistryValue<V>;

Then in libsyntax we'd have something like

// syntax::ext
use registry::RegistryValue;

#[deriving(Default)]
pub struct Registry { ... }

impl Registry {
    pub fn register_macro(&mut self, name: Name, ext: SyntaxExtension) { ... }
}

pub static MacroRegistry: RegistryName<Registry> = &RegistryValue;

With this you should be able to do something like

extern crate registry;
extern crate syntax;

use syntax::ext::MacroRegistry;
use syntax::parse::token;
use syntax::ext::base::{BasicMacroExpander, NormalTT};

#[plugin_registrar]
pub fn plugin_registrar(reg: &mut registry::Registry) {
    reg.get(MacroRegistry).register_macro(token::intern("named_entities"),
        NormalTT(box BasicMacroExpander {
            expander: named_entities::expand,
            span: None
        },
        None));
}

And you can create whatever convenience methods you want on this as well. Similarly, librustc can provide its own implementation for LintRegistry.

@kmcallister

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2014

On IRC we discussed putting Registry in librustc. The drawback is that syntax extensions need to link librustc as well as libsyntax, increasing compile time. But it's very simple and seems fine for now.

@Valloric

This comment has been minimized.

Copy link

commented May 23, 2014

Loadable lints are a killer feature; I don't care much how support for them is implemented, but having the support is killer. Big corps like Facebook, Google etc often have mandatory style guides enforced by custom lint tools. This feature would make those easy to build and far more correct because they would use the actual compiler.

@kmcallister

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2014

This is now approximately implemented (still needs docs).

@huonw

This comment has been minimized.

Copy link
Member

commented May 25, 2014

@Valloric fwiw, it's not very hard to write external libraries using libsyntax and librustc (and all the analysis done by them) right now. Examples: a doc comment spell checker, a tool for listing/categorising unsafe code.

Of course, building it directly into the compilation step (rather than needing a separate pass) has big advantages.

@kmcallister

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2014

I documented rustc::plugin in my branch. It's ready for a PR once this RFC is accepted.

@alexcrichton alexcrichton merged commit 21aacba into rust-lang:master Jun 4, 2014

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 4, 2014

Ah, I forgot to mention, but this was discussed at yesterday's meeting and the tracking issue is at rust-lang/rust#14637

bors added a commit to rust-lang/rust that referenced this pull request Jun 6, 2014

auto merge of #14554 : kmcallister/rust/plugin_registrar, r=cmr
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.

bors added a commit to rust-lang/rust that referenced this pull request Jun 7, 2014

auto merge of #14554 : kmcallister/rust/plugin_registrar, r=cmr
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.

bors added a commit to rust-lang/rust that referenced this pull request Jun 9, 2014

auto merge of #14554 : kmcallister/rust/plugin_registrar, r=cmr
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.

withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017

Merge pull request rust-lang#86 from nelsonjchen/patch-1
Correct link to Hello World loop's run() in TUTORIAL.md

@chriskrycho chriskrycho referenced this pull request May 23, 2017

Closed

Document all features #9

18 of 48 tasks complete

wycats pushed a commit to wycats/rust-rfcs that referenced this pull request Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.