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

The help for deprecated panic_implementation attribute is bad #54246

Closed
nagisa opened this issue Sep 15, 2018 · 4 comments
Closed

The help for deprecated panic_implementation attribute is bad #54246

nagisa opened this issue Sep 15, 2018 · 4 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@nagisa
Copy link
Member

nagisa commented Sep 15, 2018

warning: use of deprecated attribute `panic_implementation`: This attribute was renamed to `panic_handler`. See https://github.com/rust-lang/rust/issues/44489#issuecomment-415140224
  --> test.rs:13:1
   |
13 | #[panic_implementation]
   | ^^^^^^^^^^^^^^^^^^^^^^^ help: remove this attribute
   |
   = note: #[warn(deprecated)] on by default

Namely,

   | ^^^^^^^^^^^^^^^^^^^^^^^ help: remove this attribute

will not help at all in resolving the issue. The help should either not exist at all or suggest replacement to #[panic_handler] (moving the suggestion from the warning itself).

@nagisa nagisa added the A-diagnostics Area: Messages for errors, warnings, and lints label Sep 15, 2018
@varkor varkor added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Sep 15, 2018
@varkor
Copy link
Member

varkor commented Sep 15, 2018

If anyone'd like to take this on, it should be straightforward.

https://github.com/rust-lang/rust/blob/6bd2d5703d5944def902ec9dca6ea8aae07c8963/src/libsyntax/feature_gate.rs#L720-L724

I think the clearest way to address this is to modify the Deprecated type to have a second Option<&'static str> field, which is None if there is no suggested replacement and Some(replacement_feature) if the feature has been deprecated in favour of replacement_feature.

You'd then need to update the occurrences of Deprecated (in the case of panic_implementation, it needs Some("panic_handler") — others may also have sensible substitutes) and make this lint:
https://github.com/rust-lang/rust/blob/6bd2d5703d5944def902ec9dca6ea8aae07c8963/src/librustc_lint/builtin.rs#L793-L798
suggest replacing the feature attribute if a substitute exists.

Some tests might also need updating afterwards.

(Also, This attribute should be this attribute in the panic_implementation deprecation notice.)

@illfygli
Copy link
Contributor

I would like to try this one!

@varkor
Copy link
Member

varkor commented Sep 16, 2018

@Snaedis: great! Feel free to ask here if you need any help with the fix!

@illfygli illfygli mentioned this issue Sep 17, 2018
@illfygli
Copy link
Contributor

@varkor first stab is ready; feedback would be much appreciated!

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 18, 2018
Issue 54246

I added the option of providing a help message for deprecated features, that takes precedence over the default `help: remove this attribute` message, along with messages for the features that mention replacements in the reason for deprecation.

Fixes rust-lang#54246.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

No branches or pull requests

3 participants