-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Change error about unknown attributes to a warning #82702
Conversation
This comment has been minimized.
This comment has been minimized.
You'll need to deny warnings and to update stderr files to make it work. Also, I didn't think about it but maybe there is a lint about unknown attribute we could use here? |
If it's going to be a warning then it should trigger the |
I don't consider that to be an issue. If you try to use a rust feature on a compiler that doesn't support it you get an error. I don't see why rustdoc should behave any differently. The issue is that changing an attribute from doing nothing to doing something is a breaking change but changing an attribute from triggering an error to doing something is not. If #82662 caused too much breakage then we can switch to triggering the |
(Drive-by comment, as i was prompted to do so on discord) Making it a hard error (as is implemented in current nightly) is bad because having a bad attribute in a library makes dependent crates stop compiling, and the end-users can do nothing to work around the issue. ("File PRs to the crates you depend on" does not quite work for a multitude of reasons) Two approaches I see to bring matters to a more desirable state are:
Offloading this entirely to rustdoc (and not surfacing in rustc at all) is probably a bad idea, because that would lead to people publishing their crate, and get confused why docs.rs doesn't update. And then fix the problem, and need to publish yet another patch version to crates.io. Not the end of the world, but certainly annoying. |
At the very least, introducing a new hard error needs to go through a forward-compat-lint stage to give existing code a grace period to be fixed. |
I think we all agree on making this a warning instead of a hard error. Just waiting for @jyn514 to update the PR then. ;) |
Personally I think this should be a hard error in the future, other attributes emit hard errors for unknown values:
We just need to follow the standard deprecation process to introduce a new hard error. EDIT: And with Edition 2021 around the corner, now might be a perfect time to do it via an edition-specific hard error 😁 |
This prevents breakage across the ecosystem, since the error was just introduced recently without first having a warning period.
👍 this makes sense to me.
👍 I did that in this PR, and added a warning that it will likely break in the future.
I don't think this needs to be edition-specific. I would prefer to eventually make it a hard error consistently across editions. @moxian's suggestions are interesting, but I don't think we need to special case the warnings that much. Having a warning that eventually becomes a hard error seems fine to me. |
I agree eventually, to accelerate uptake I think it could start as a future-incompat-warning but be a hard error on edition 2021 before it is made a hard error on earlier editions (though maybe that should be a more general discussion that could apply to all current future-incompat-warnings). |
@bors r+ p=1000 I'm r+ing this because of the breakage (which basically affects anyone who uses Furthermore I am not convinced this should be an error in the future. Rust is not just used by people who are able to groom their dependencies. Old rust code should, as much as possible, continue to compile, and every time we break that must be a hard-considered decision. Even for deprecations. I don't mind making this a future incompat lint, but I feel like at best we should only break this in a future edition (could be 2021). In the future I'd also like to strongly caution folks against adding new hard errors for code that currently compilers. Figure out if it can be a warning instead, do a crater run, but such changes should have far more thought put into it. Breaking CI this way grinds tons of Rust projects to a halt. |
📌 Commit 4b2e4e6 has been approved by |
Thinking about it more: Existing unknown attributes are also just a warning. We do error on unknown |
Moving the discussion about a proper future incompat lint to #82730 Let's keep this PR discussion focused on getting this landed |
@bors p=100 This is important, but not more important than changes fixing CI itself. |
☀️ Test successful - checks-actions |
… r=Manishearth Warn on `#![doc(test(...))]` on items other than the crate root and use future incompatible lint Part of rust-lang#82672. This PR does multiple things: * Create a new `INVALID_DOC_ATTRIBUTE` lint which is also "future incompatible", allowing us to use it as a warning for the moment until it turns (eventually) into a hard error. * Use this link when `#![doc(test(...))]` isn't used at the crate level. * Make rust-lang#82702 use this new lint as well. r? `@jyn514`
… r=Manishearth Warn on `#![doc(test(...))]` on items other than the crate root and use future incompatible lint Part of rust-lang#82672. This PR does multiple things: * Create a new `INVALID_DOC_ATTRIBUTE` lint which is also "future incompatible", allowing us to use it as a warning for the moment until it turns (eventually) into a hard error. * Use this link when `#![doc(test(...))]` isn't used at the crate level. * Make rust-lang#82702 use this new lint as well. r? ``@jyn514``
Hard errors should go through a future-compatibility phase first, especially since these attributes only have no effect and don't actively cause bugs.
Follow-up to #82662. Fixes ecosystem breakage like rust-lang/rust-clippy#6832.
r? @GuillaumeGomez