Copy link

Member

This comment has been hidden.

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?

ollie27 commented 12 days ago

If it's going to be a warning then it should trigger the unused_attributes lint.

ollie27 commented 12 days ago

This allows adding more attributes in the future. Previously, using a
new attribute would cause the crate to fail to compile on a toolchain
before the attribute was introduced.

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 unused_attributes lint for now but ultimately it should be hard error.

moxian commented 12 days ago

(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:

• make this a (rustc) error, but only when compiling your own crate - ignore the problem for dependencies. I know this approach was invoked previously for something but i can't remember for what exactly.
• make this a warning for rustc, and an error for rustdoc. However, this might need to combine the approach from the point above in that rustdoc should keep working for your own crate, even if the crates it depends on are borked.

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.

Nemo157 commented 12 days ago

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. ;)

Nemo157 commented 12 days ago •

Personally I think this should be a hard error in the future, other attributes emit hard errors for unknown values:

error[E0552]: unrecognized representation hint
 --> lib.rs:1:8
  |
1 | #[repr(bar)]
  |        ^^^

error: aborting due to previous error

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

jyn514 commented 11 days ago

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.

this makes sense to me.

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 did that in this PR, and added a warning that it will likely break in the future.

EDIT: And with Edition 2021 around the corner, now might be a perfect time to do it via an edition-specific hard error grin

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.

Nemo157 commented 11 days ago

I don't think this needs to be edition-specific. I would prefer to eventually make it a hard error consistently across editions.

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).

Manishearth commented 11 days ago •

@bors r+ p=1000

I'm r+ing this because of the breakage (which basically affects anyone who uses criterion, breaking tons of CI everywhere), but this is also not how future incompat is done: you have to have a separate future incompat lint so that the tooling behaves appropriately.

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.

bors commented 11 days ago

Commit 4b2e4e6 has been approved by Manishearth

Manishearth commented 11 days ago

Thinking about it more: Existing unknown attributes are also just a warning. We do error on unknown repr()s, etc. We should figure out where we land on that scale, but I feel like editions are the best mechanism for this kind of breakage.

Manishearth commented 11 days ago

Moving the discussion about a proper future incompat lint to #82730

Let's keep this PR discussion focused on getting this landed

jyn514 commented 11 days ago

@bors p=100

This is important, but not more important than changes fixing CI itself.

bors commented 11 days ago

Testing commit 4b2e4e6 with merge 1c77a1f...

bors commented 11 days ago

Test successful - checks-actions
Approved by: Manishearth
Pushing 1c77a1f to master...