-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Remove support for compiler plugins. #116412
Conversation
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in src/tools/rust-analyzer cc @rust-lang/rust-analyzer Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
I decided to implement the removal to see what it looked like, and to encourage a decision. I did a lot of grepping to catch as much relevant code as possible. There are still plenty of "plugin" mentions for things like linker plugins, which are different. An FCP will be needed before this can merge, at the minimum. Cargo will need some adjustment, to remove the Servo is often given as a reason for keeping plugin support, but @SimonSapin said this 18 months ago:
Servo has become more active again recently. I don't know if that has any impact on the decision. Four years of deprecation is a long time! |
@@ -262,24 +262,6 @@ pub const INERT_ATTRIBUTES: &[BuiltinAttribute] = &[ | |||
naked_functions, experimental!(naked) | |||
), | |||
|
|||
// Plugins: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is a no-op and we're currently unable to do syncs, would you mind dropping this change and optionally filing it against the RA repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, if this PR is approved I can do that.
This comment has been minimized.
This comment has been minimized.
5079310
to
0fcfd98
Compare
This comment has been minimized.
This comment has been minimized.
I think the PR description justifies the removal, but also see this zulip thread for justification. @rfcbot fcp merge |
Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:
Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@rfcbot concern servo-buy-in The main (and only?) reason why this feature wasn't removed several years ago is its servo use case. |
@rfcbot concern servo-buy-in |
previous attempt at this: #64675 |
☔ The latest upstream changes (presumably #116455) made this pull request unmergeable. Please resolve the merge conflicts. |
cc some Servo people for input: @mrobinson, @emilio, @mukilan, @sagudev, @jdm, @Manishearth |
It's still used in servo for GC safety, yes, and people have been actively working on Servo again. It is possible servo could move to one of the custom lint frameworks out there, or potentially turn this into a clippy restriction lint (sufficiently generic with attributes so that others can use it) |
Yeah, I don't think anything has really changed here from what's been previously articulated in #64675. Obviously it would be easier for Servo if this removal didn't happen, but I recognize that the project holds much less influence over the process than it once did (and that's probably a good thing, to be honest). We had a discussion about whether a linter alternative could work in https://servo.zulipchat.com/#narrow/stream/263398-general/topic/Dylint. |
Right, so we're in a "nothing has changed in three years" situation. I'm really hoping to avoid having this long-deprecated, long-unmaintained feature kept around indefinitely. |
Servo will have 100% buy-in on putting in the necessary maintenance work, one way or another, if it is removed now and they wish to use a stable version of 1.75, or the nightly immediately after this lands. There are many reasons to smooth a transition path for users of nightly features, but the existing grace period has been exceptional. |
I will say, while servo is the only permanent user that I know of, having a permaunstable plugin API like this has been useful at various times and imo is a good thing to have. Similarly, clang has the unstable clang plugin API that a lot of tools use. I'd be surprised if this were a maintenance burden: to me "it's been around for years and nobody has tried to remove it yet" feels more due to it not getting in the way of anything, but I could be wrong. servo hasn't had major pull in the rust project for ages so I'd be surprised if the reason was that it did get in the way but people decided not to remove it because of servo. (while I'm still somewhat biased here; I will note that I haven't written code on servo/servo for ages and wouldn't be directly affected by this decision. I'm just a fan of us having such an API. I've found both the rust and clang plugin APIs to be super useful for various operations) |
It is actually absurdly annoying to have around, and I have complained about it periodically, because the tests are almost always failing on actual local builds on platforms that aren't x86-64 Linux in my experience, which tricks contributors into thinking they broke something. And I have no idea why that only happens in local builds and not in the test suite, It Just Does, somehow. It greatly inconvenienced me the entire time I was trying to contribute to Rust while using Windows as my OS. I at some point stopped using Windows, but I have had to repeatedly reassure other contributors that no, they didn't break anything. If I had the courage and knowledge to do so, then, I would've opened this very same PR years ago. So yes, this is exactly why. Literally, petrochenkov raised a concern based on this reason. No one is maintaining this interface, that I am aware of, and rustc wrappers like your very own clippy actually work in a cross-platform way, and are capable of more besides. |
Okay, thanks, in which case I feel comfortable about having this removed. I think such a feature would be valuable to have in the long run, I agree that it should be maintained and should not be retained if it is at all a current maintenance burden, which it clearly is. (And future attempts to re-add this can refer to this thread to understand what things need to be resolved for such a feature to be Good) |
I think servo should no longer be problematic, I made prototype in servo/servo#30508 (with all of our lints ported), now I only need to polish integration with servo. It would be nice if this PR waited for servo/servo#30508 to land in servo (which should be in a week). |
Finished benchmarking commit (5020f7c): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 635.435s -> 634.085s (-0.21%) |
Perf improvements are mostly noise. |
84: Automated pull from upstream `master` r=Dajamante a=github-actions[bot] This PR pulls the following changes from the upstream repository: * rust-lang/rust#117585 * rust-lang/rust#117576 * rust-lang/rust#96979 * rust-lang/rust#117191 * rust-lang/rust#117179 * rust-lang/rust#117574 * rust-lang/rust#117537 * rust-lang/rust#117608 * rust-lang/rust#117596 * rust-lang/rust#117588 * rust-lang/rust#117524 * rust-lang/rust#116017 * rust-lang/rust#117504 * rust-lang/rust#117469 * rust-lang/rust#116218 * rust-lang/rust#117589 * rust-lang/rust#117581 * rust-lang/rust#117503 * rust-lang/rust#117590 * rust-lang/rust#117583 * rust-lang/rust#117570 * rust-lang/rust#117562 * rust-lang/rust#117534 * rust-lang/rust#116894 * rust-lang/rust#110340 * rust-lang/rust#113343 * rust-lang/rust#117579 * rust-lang/rust#117094 * rust-lang/rust#117566 * rust-lang/rust#117564 * rust-lang/rust#117554 * rust-lang/rust#117550 * rust-lang/rust#117343 * rust-lang/rust#115274 * rust-lang/rust#117540 * rust-lang/rust#116412 * rust-lang/rust#115333 * rust-lang/rust#117507 * rust-lang/rust#117538 * rust-lang/rust#117533 * rust-lang/rust#117523 * rust-lang/rust#117520 * rust-lang/rust#117505 * rust-lang/rust#117434 * rust-lang/rust#117535 * rust-lang/rust#117510 * rust-lang/rust#116439 * rust-lang/rust#117508 Co-authored-by: Ben Wiederhake <[email protected]> Co-authored-by: SabrinaJewson <[email protected]> Co-authored-by: J-ZhengLi <[email protected]> Co-authored-by: koka <[email protected]> Co-authored-by: bjorn3 <[email protected]> Co-authored-by: Joshua Liebow-Feeser <[email protected]> Co-authored-by: lengyijun <[email protected]> Co-authored-by: Zalathar <[email protected]> Co-authored-by: Oli Scherer <[email protected]> Co-authored-by: Philipp Krones <[email protected]> Co-authored-by: y21 <[email protected]> Co-authored-by: bors <[email protected]> Co-authored-by: bohan <[email protected]>
Compiler plugins were removed in rust-lang/rust#116412, so we don't need these tests. As for the `plugin` field on build-targets, it appears to be [stable-but-deprecated](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#the-plugin-field), so I left it alone
Compiler plugins were removed in rust-lang/rust#116412, so we don't need these tests. As for the `plugin` field on build-targets, it appears to be [stable-but-deprecated](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#the-plugin-field), so I left it alone
tests: Remove plugin tests Compiler plugins were removed in rust-lang/rust#116412, so we don't need these tests. As for the `plugin` field on build-targets, it appears to be [stable-but-deprecated](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#the-plugin-field), so I left it alone
Compiler plugins were removed in rust-lang/rust#116412, so we don't need these tests. As for the `plugin` field on build-targets, it appears to be [stable-but-deprecated](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#the-plugin-field), so I left it alone
Postscript: @bjorn3's suggestion to move lint registration earlier was very useful. I did that in #117649, and it led to more clean-ups in #117993 and #118002. Combined with some recent related improvements I made in #117268, #117376, and #117475, and |
Rust compiler plugins have been entirely removed after having been deprecated for years [1]. This caused the build of `aw-server-rust` to break because its `aw-server` crate still contained the respective crate attribute to enable the feature. This attribute has already been removed upstream along with other no longer used unstable feature opt-ins [2]. This change pulls in the respective commit with `fetchpatch` and filters it down to only the changes in `aw-server/src/lib.rs`, which fixes the build for now. The patch should be removable with the next release (0.12.3). [1]: rust-lang/rust#116412 [2]: https://github.com/ActivityWatch/aw-server-rust/commit/e1cd761d2f0a9309eb851b59732c2567a7ae2d3a.patch
* Typo Typo, filename is now file_path. * rst restore `value` * Fix broken nostarch URL * Change CamelCase to UpperCamelCase Brings the terminology in line with the rest of the project, see <rust-lang/rfcs#2389>. Connects to rust-lang#2194. * Update to Rust 1.66.1 * Update to Rust 1.67.1 * Removed "," typo on ch03-01 line 85 Removed extra comma on line 85. We’ll cover types and type annotations in the next section, “Data Types`,`”, so don’t worry about the details right now. * simplify COPYRIGHT file * Fix grammar Add the word 'of' to fix the grammar. * Update copyright in LICENSE-APACHE Looks like we forgot to fill this in when we added the license file. ;) * Correct `i32` formatting in ch19-05 * Remove adjective about what kind of number this is * Improve sentence * Small wording changes * redirects: change link for `#![no_std]` tutorial * Fix cargo doc links * Prepare for removal of compiler plugin support. rust-lang/rust#116412 will remove support for compiler plugins from rustc, which includes the entry in The Rust Unstable Book. This commit removes a link to that entry so it won't be broken when that PR merges. * Fix mdBook links * Fixed 'troubleshooting' link * Update ch01-02-hello-world.md * Update ch01-02-hello-world.md --------- Co-authored-by: abiphone <[email protected]> Co-authored-by: printfn <[email protected]> Co-authored-by: Carol (Nichols || Goulding) <[email protected]> Co-authored-by: Mateus Rodolfo <[email protected]> Co-authored-by: Pietro Albini <[email protected]> Co-authored-by: Vishal Lama <[email protected]> Co-authored-by: Jason Walton <[email protected]> Co-authored-by: Shinya Fujino <[email protected]> Co-authored-by: Jaime Terreu <[email protected]> Co-authored-by: Mike Krisher <[email protected]> Co-authored-by: kadiwa <[email protected]> Co-authored-by: Eric Huss <[email protected]> Co-authored-by: Nicholas Nethercote <[email protected]> Co-authored-by: Marcus Stollsteimer <[email protected]> Co-authored-by: Aryan Malik <[email protected]>
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * For an external LLVM, set dependency of llvm >= 16, in accordance with the upstream changes. * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported. * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it, resulting in an illegal instruction fault during the build. Ref. rust-lang/rust#117231 Upstream changes: Version 1.75.0 (2023-12-28) ========================== - [Stabilize `async fn` and return-position `impl Trait` in traits.] (rust-lang/rust#115822) - [Allow function pointer signatures containing `&mut T` in `const` contexts.] (rust-lang/rust#116015) - [Match `usize`/`isize` exhaustively with half-open ranges.] (rust-lang/rust#116692) - [Guarantee that `char` has the same size and alignment as `u32`.] (rust-lang/rust#116894) - [Document that the null pointer has the 0 address.] (rust-lang/rust#116988) - [Allow partially moved values in `match`.] (rust-lang/rust#103208) - [Add notes about non-compliant FP behavior on 32bit x86 targets.] (rust-lang/rust#113053) - [Stabilize ratified RISC-V target features.] (rust-lang/rust#116485) Compiler -------- - [Rework negative coherence to properly consider impls that only partly overlap.] (rust-lang/rust#112875) - [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.] (rust-lang/rust#116493) - [Consider alias bounds when computing liveness in NLL.] (rust-lang/rust#116733) - [Add the V (vector) extension to the `riscv64-linux-android` target spec.] (rust-lang/rust#116618) - [Automatically enable cross-crate inlining for small functions] (rust-lang/rust#116505) - Add several new tier 3 targets: - [`csky-unknown-linux-gnuabiv2hf`] (rust-lang/rust#117049) - [`i586-unknown-netbsd`] (rust-lang/rust#117170) - [`mipsel-unknown-netbsd`] (rust-lang/rust#117356) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.] (rust-lang/rust#96979) - [Implement `BufRead` for `VecDeque<u8>`.] (rust-lang/rust#110604) - [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.] (rust-lang/rust#110729) - [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.] (rust-lang/rust#113747) - [Implement `Default` for `ExitCode`.] (rust-lang/rust#114589) - [Guarantee representation of None in NPO] (rust-lang/rust#115333) - [Document when atomic loads are guaranteed read-only.] (rust-lang/rust#115577) - [Broaden the consequences of recursive TLS initialization.] (rust-lang/rust#116172) - [Windows: Support sub-millisecond sleep.] (rust-lang/rust#116461) - [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl] (rust-lang/rust#100806) - [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.] (rust-lang/rust#115108) Stabilized APIs --------------- - [`Atomic*::from_ptr`] (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr) - [`FileTimes`] (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html) - [`FileTimesExt`] (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html) - [`File::set_modified`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified) - [`File::set_times`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times) - [`IpAddr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical) - [`Ipv6Addr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical) - [`Option::as_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice) - [`Option::as_mut_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice) - [`pointer::byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add) - [`pointer::byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset) - [`pointer::byte_offset_from`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from) - [`pointer::byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub) - [`pointer::wrapping_byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add) - [`pointer::wrapping_byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset) - [`pointer::wrapping_byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub) These APIs are now stable in const contexts: - [`Ipv6Addr::to_ipv4_mapped`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped) - [`MaybeUninit::assume_init_read`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read) - [`MaybeUninit::zeroed`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed) - [`mem::discriminant`] (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html) - [`mem::zeroed`] (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html) Cargo ----- - [Add new packages to `[workspace.members]` automatically.] (rust-lang/cargo#12779) - [Allow version-less `Cargo.toml` manifests.] (rust-lang/cargo#12786) - [Make browser links out of HTML file paths.] (rust-lang/cargo#12889) Rustdoc ------- - [Accept less invalid Rust in rustdoc.] (rust-lang/rust#117450) - [Document lack of object safety on affected traits.] (rust-lang/rust#113241) - [Hide `#[repr(transparent)]` if it isn't part of the public ABI.] (rust-lang/rust#115439) - [Show enum discriminant if it is a C-like variant.] (rust-lang/rust#116142) Compatibility Notes ------------------- - [FreeBSD targets now require at least version 12.] (rust-lang/rust#114521) - [Formally demote tier 2 MIPS targets to tier 3.] (rust-lang/rust#115238) - [Make misalignment a hard error in `const` contexts.] (rust-lang/rust#115524) - [Fix detecting references to packed unsized fields.] (rust-lang/rust#115583) - [Remove support for compiler plugins.] (rust-lang/rust#116412)
…ouxu [Refactor] Rename `Lint` and `LintGroup`'s `is_loaded` to `is_externally_loaded` The field being named `is_loaded` was very confusing. Turns out it's true for lints that are registered by external tools like Clippy (I had to look at rust-lang#116412 to know what the variable meant). So I renamed `is_loaded` to `is_externally_loaded` and added some docs.
Rollup merge of rust-lang#124522 - blyxyas:refactor-is-loaded, r=jieyouxu [Refactor] Rename `Lint` and `LintGroup`'s `is_loaded` to `is_externally_loaded` The field being named `is_loaded` was very confusing. Turns out it's true for lints that are registered by external tools like Clippy (I had to look at rust-lang#116412 to know what the variable meant). So I renamed `is_loaded` to `is_externally_loaded` and added some docs.
[Refactor] Rename `Lint` and `LintGroup`'s `is_loaded` to `is_externally_loaded` The field being named `is_loaded` was very confusing. Turns out it's true for lints that are registered by external tools like Clippy (I had to look at rust-lang/rust#116412 to know what the variable meant). So I renamed `is_loaded` to `is_externally_loaded` and added some docs.
Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.
They've been deprecated for four years.
This commit includes the following changes.
rustc_plugin_impl
crate.compiler/rustc_driver_impl/src/lib.rs
andcompiler/rustc_lint/src/context.rs
. External lints are now called "loaded" lints, rather than "plugins" to avoid confusion with the old plugins. This only has a tiny effect on the output of-W help
.plugin
feature was moved from "active" to "removed".tests/ui-fulldeps/plugin/
.Closes #29597.
r? @ghost