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

Deprecate #![plugin] & #[plugin_registrar] #64675

Merged
merged 3 commits into from
Oct 4, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Sep 21, 2019

This PR deprecates #![plugin] and #[plugin_registrar].

A removal deadline is set: 1.44.0. This will be in 9 months from now and should give everyone who is still relying on the feature ample time to rid themselves of this dependency.

cc #29597

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

Implementation wise this looks reasonable, but we probably want to get sign-off from at least T-lang here, maybe T-compiler (in particular, if plugins are actively blocking any work, we can move removal date closer). So let's: @rustbot modify labels: +I-nominated T-lang T-compiler

Cc @rust-lang/lang @rust-lang/compiler

It might also be worth reaching out to servo directly as they're the ones we're "waiting on" to remove this functionality to make them aware of this timeline. I believe current consensus is that anyone using plugins today is advised to migrate to a custom rustc build (via a rustc_private dep on rustc_interface) which should be relatively fast, though not particularly stable, of course, but no less stable than plugins were/are.

@rustbot rustbot added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 21, 2019
@rust-highfive

This comment has been minimized.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 22, 2019

and should give everyone who is still relying on the feature ample time to rid themselves of this dependency.

Do we know who these are and what they are using plugins for? Or could we do a crater run with this set to error to at least find out ?

@Centril
Copy link
Contributor Author

Centril commented Sep 22, 2019

Or could we do a crater run with this set to error to at least find out ?

Folks have used it previously for the same thing as proc macros, they've also used it for custom lints and such. In any case, it has been clear for a long long time that this is to be removed eventually.

Also, I think we should use crater more sparingly, at least until we have more crater instances.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 22, 2019

We managed to stop using plugins last december, as alternatives for our use cases became available. That's not that long ago. I think it would make sense to at least try to find out who the current users of plugins are and why and whether they have something they can migrate to.

@Centril
Copy link
Contributor Author

Centril commented Sep 23, 2019

Last December was quite a while ago (9 months) and this will take some more 9 months which is quite a while. Most use cases are satisfied by procedural macros and others by what @Mark-Simulacrum noted in #64675 (comment). I also don't think we are obligated to satisfy every and each use case that is covered by plugins. Using crater on this (right now) is in my view a waste of project resources - people can complain by opening issues when they see the warning.

@Mark-Simulacrum
Copy link
Member

To be clear, I think a crater run on removal makes sense -- that'll be in 6 months I believe -- by then, we'll hopefully be at a point where ~no one who maintains their code at all is using plugins. Any non-maintained code, well, it is an unstable feature for a reason.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 23, 2019

I also don't think we are obligated to satisfy every and each use case that is covered by plugins.

The only thing I'm saying is that in my experience it makes sense to try to figure out who is using a feature and what for before starting the process of removing it, since it's hard to tell whether the "deadline" is appropriately long or short without having this information. For all I know nobody might be using this and a 6 weeks cycle of warnings might be ok.

@Centril
Copy link
Contributor Author

Centril commented Sep 23, 2019

For all I know nobody might be using this and a 6 weeks cycle of warnings might be ok.

I would suspect the deadline is possibly too long. A follow up PR can turn these into errors and use that to do a crater run. But I'd like the warnings to be in place sooner rather than later.

@nikomatsakis
Copy link
Contributor

cc @nox and @SimonSapin -- take a look at this comment by @Mark-Simulacrum, in particular this part:

It might also be worth reaching out to servo directly as they're the ones we're "waiting on" to remove this functionality to make them aware of this timeline. I believe current consensus is that anyone using plugins today is advised to migrate to a custom rustc build (via a rustc_private dep on rustc_interface) which should be relatively fast, though not particularly stable, of course, but no less stable than plugins were/are.

Any thoughts here?

@nox
Copy link
Contributor

nox commented Sep 24, 2019 via email

@jdm
Copy link
Contributor

jdm commented Sep 24, 2019

clippy appears to use plugin_registrar; what's the plan for allowing that popular tool to continue existing?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 24, 2019

what's the plan for allowing that popular tool to continue existing?

I've been wanting to nuke that since forever. We're already warning users that the plugin API will disappear since over a year. Clippy works via a custom driver, which is also the proposed solution for servo.

@Manishearth
Copy link
Member

In any case, it has been clear for a long long time that this is to be removed eventually.

This is not true: Every time this has been brought up Servo has mentioned that it would be pretty hard to move to a custom driver, and discussion AIUI hasn't progressed from there since no replacement was suggested.

The real path out for Servo to get rid of its lints requires significant refactoring to use a lintless GC design, which is on hold for now. An alternate route might be to build this on RLS but it's not 100% clear that that is feasible.

I don't see why this feature so desperately needs to be removed: the internal lint interface is still going to be used by clippy, the only thing you're removing is the ability to externally load lints. Lints are quite useful for usage in a non-release context.

Is there a major cost to keeping this feature around? My read was that it's largely not been a maintenance burden, it just exists without issue.

@nox
Copy link
Contributor

nox commented Sep 24, 2019 via email

@Manishearth
Copy link
Member

Previously there was a plan to add GC integration support to Rust, and that was since scrapped, and one of the reasons we've been okay with that was that we were fine relying on our lints.

@Manishearth
Copy link
Member

What exactly is a custom driver, and does that mean users can install nightly and compile just the custom driver, or do they end up building a rustc from scratch etc?

@nox @jdm They just compile the custom driver, not all of rustc. But doing so will make other tooling (like RLS) harder to use.

A custom driver is basically a drop-in rustc binary replacement, which is created by copying over rustc's main function and modifying some bits. This is essentially what Clippy is: see https://github.com/rust-lang/rust-clippy/blob/master/src/driver.rs

@Manishearth
Copy link
Member

Basically, I don't think enough work has been put into custom driver readiness for us to use it. I'd be okay with this if work had been put into making Cargo and RLS better aware of these things, but it hasn't. RLS works with clippy because it directly knows about clippy. And putting that work in is just replacing one less-used feature with another.

@Mark-Simulacrum
Copy link
Member

To my knowledge, using a custom driver is pretty easy today - it does require nightly (or otherwise bypassing stability...). It does not require a lengthy build process (it's "just" a binary depending on a sysroot crate, like all binaries that depend on std in some way), but does "become" your rustc; you'd use it by override the rustc cargo uses to your own. I believe Servo is already using a custom build tool (similar to x.py here in rust-lang/rust) so that shouldn't be that big of a hassle to do.

I do think making sure that Servo, Clippy have migration paths is necessary, but I don't think we need to do that before we schedule deletion to start in 9 months.

Is there a major cost to keeping this feature around? My read was that it's largely not been a maintenance burden, it just exists without issue.

In my experience, technically, no. However, plugins are somewhat of a hassle to support as they are "late registered" and are dynamically loaded at runtime via dlopen and such all of which is overall quite painful from a maintenance perspective. The late registration in particular has been a thorny issue in a few of the refactors I've been working on -- it's not great that we have to start parsing the crate and only then realize that we have lints and possibly macros that need to be registered via #![plugin(...)]. I believe @petrochenkov's work on cleaning up syntax contexts also wants to see us move away from plugins and the "direct plug" they provide to a TokenStream model.

But doing so [using plugins] will make other tooling (like RLS) harder to use.

I think this is somewhat true, but only marginally so. For the most part, if Servo is using them just for lints, then I would imagine that it would be not too hard to architect that in such a way that the lint warnings simply vanish when using RLS.

@Manishearth
Copy link
Member

For the most part, if Servo is using them just for lints, then I would imagine that it would be not too hard to architect that in such a way that the lint warnings simply vanish when using RLS.

I think cargo doesn't like it when you swap out compilers that way.

Like, this is buggy. This even is buggy for clippy, and part of the fix was moving cargo-clippy into cargo. Clippy is very much a special case: both RLS and Cargo know about it.

To be clear: we're totally okay with syntax extensions being removed: it's just the plugin lint interface we care about. That doesn't have the parsing lifecycle issues you talk about.

@Mark-Simulacrum
Copy link
Member

I would be okay deprecating just the plugin attribute then -- if we can make plugins loadable strictly through the CLI then that's already a step forward. -Zextra-plugins works for Servo, right? Y'all can presumably pass it via rustflags and such?

@jdm
Copy link
Contributor

jdm commented Sep 24, 2019

Yep, extra flags aren't a problem for Servo.

@jdm
Copy link
Contributor

jdm commented Sep 24, 2019

#62868 also contains a bunch of analysis that @SimonSapin has previously done for Servo's requirements, so we shouldn't need to spend much time rehashing them here.

@Manishearth
Copy link
Member

Oh, I see, y'all aren't yet planning to remove them, just deprecating them to avoid further use. That's fine.

Could you share more about the consensus reached in the compiler team meeting?

@Centril
Copy link
Contributor Author

Centril commented Oct 3, 2019

@Manishearth

Could you share more about the consensus reached in the compiler team meeting?

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 3, 2019

r=me with tests updated to pass ci

@Centril
Copy link
Contributor Author

Centril commented Oct 3, 2019

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Oct 3, 2019

📌 Commit d1f95ef has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 3, 2019
Deprecate `#![plugin]` & `#[plugin_registrar]`

This PR deprecates `#![plugin]` and `#[plugin_registrar]`.

~A removal deadline is set: 1.44.0. This will be in 9 months from now and should give everyone who is still relying on the feature ample time to rid themselves of this dependency.~

cc rust-lang#29597

r? @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this pull request Oct 3, 2019
Deprecate `#![plugin]` & `#[plugin_registrar]`

This PR deprecates `#![plugin]` and `#[plugin_registrar]`.

~A removal deadline is set: 1.44.0. This will be in 9 months from now and should give everyone who is still relying on the feature ample time to rid themselves of this dependency.~

cc rust-lang#29597

r? @Mark-Simulacrum
tmandry added a commit to tmandry/rust that referenced this pull request Oct 3, 2019
Deprecate `#![plugin]` & `#[plugin_registrar]`

This PR deprecates `#![plugin]` and `#[plugin_registrar]`.

~A removal deadline is set: 1.44.0. This will be in 9 months from now and should give everyone who is still relying on the feature ample time to rid themselves of this dependency.~

cc rust-lang#29597

r? @Mark-Simulacrum
tmandry added a commit to tmandry/rust that referenced this pull request Oct 3, 2019
Deprecate `#![plugin]` & `#[plugin_registrar]`

This PR deprecates `#![plugin]` and `#[plugin_registrar]`.

~A removal deadline is set: 1.44.0. This will be in 9 months from now and should give everyone who is still relying on the feature ample time to rid themselves of this dependency.~

cc rust-lang#29597

r? @Mark-Simulacrum
bors added a commit that referenced this pull request Oct 3, 2019
Rollup of 11 pull requests

Successful merges:

 - #61879 (Stabilize todo macro)
 - #64675 (Deprecate `#![plugin]` & `#[plugin_registrar]`)
 - #64690 (proc_macro API: Expose `macro_rules` hygiene)
 - #64706 (add regression test for #60218)
 - #64741 (Prevent rustdoc feature doctests)
 - #64842 (Disallow Self in type param defaults of ADTs)
 - #65004 (Replace mentions of IRC with Discord)
 - #65018 (Set RUST_BACKTRACE=0 in tests that include a backtrace in stderr)
 - #65055 (Add long error explanation for E0556)
 - #65056 (Make visit projection iterative)
 - #65057 (typo: fix typo in E0392)

Failed merges:

r? @ghost
@bors bors merged commit d1f95ef into rust-lang:master Oct 4, 2019
@Centril Centril deleted the deprecate-plugin branch October 4, 2019 04:56
@jplatte jplatte mentioned this pull request Oct 12, 2019
bors added a commit that referenced this pull request Nov 24, 2019
rustc_plugin: Remove support for syntactic plugins

This part of the plugin interface was successfully replaced by token-based procedural macros in theory and in practice.

cc #29597
cc #64675
r? @Centril
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.