Stability lint checks code from macro expansion. #17286
Conversation
|
cc @aturon |
|
Wow, this is great! I had no idea it would be so simple. Just to clarify, though: if a macro definition itself uses e.g. a deprecated item, does the macro definer or the macro user get the warning? What happens with the following test? I would assume no errors are generated. #[allow(deprecated)]
macro_rules! macro_test_allow(
() => (deprecated());
)
macro_test_allow!(); |
|
Well, the lint checks the code generated by the macro, rather than the macro itself. Thus only the user gets the warning, referring to the definition of the macro or the call, depending on whether the deprecated item is in the definition or the arguments. I don't known if it's possible to discriminate items depending on whether they come from the macro definition or the arguments, it would probably be the short-term fix. I'll look at this tomorrow, it far past midnight for me. |
|
Okay, after further search, I think we have 3 possible cases of
Ideally, this PR should detect cases 2 and 3. Currently, it detects the three cases, and for cases 1 and 3, the generated lint refers to the macro definition, rather than the call. It can be quite troubling to the user in cases where he doesn't know the macro definition.. Also, I can't distinguish cases 1 and 3 as the lint only checks the generated code, and contrarily to case 2, the deprecated item is not present before expansion (in case 3 the expansion must be done for the resolve to see that There is also no support at all for So right now, from what I saw the two "easy" possibilities are either stay this way or change to detect only case 2. @aturon What do you think ? |
|
@vberger Detecting only case 2 seems like a definite step in the right direction -- and in particular, I think it would resolve the original issue that was reported. So I'd be happy to land that for now! |
|
@aturon Should be good now. |
|
@aturon Oh, thanks to your commit d8dfe19 the bors test showed a case I forgot to handle properly : when a case 2 lint in generated by a case 3 macro, ie : I've corrected this (and corrected the remaining deprecated calls in the macros of |
|
@vberger Can you please add a test corresponding to this case? |
|
@aturon Done. |
|
@aturon Ok,I didn't run the full test suite (shame on me, but it's so long...) and apparently my changes raise a lot of warnings due to deprecated items in crate tests, and some of them are apparently denied, causing bors build to fail. I'll try to correct this today (changing syntax or adding |
|
@aturon Done. It was mainly outdated docs or docs of deprecated items. |
|
@aturon I rebased the PR, recent changes in librustc conflicted. |
Fallout of closing #17185.
Closes #17185. The stability lint will now check code generated by macro expansion. It will allow to detect : - arguments passed to macros using deprecated (and others) items - macro expansion generating code using deprecated items due to its arguments (hence the second commit, fixing such issue found in libcollections) Checking is still done at expansion, but it will also detect a macro explicitly using a deprecated item in its definition.
… r=pnkfelix Finishes the job of #17286. Now the stability lint will successfully detect patterns such as: ``` first_macro!(second_macro!(deprecated_function())); ``` ``` macro_rules! foo ( ($e: expr) => (bar!($e)) ) foo!(deprected_function()); ``` and ``` println!("{}", deprecated_function()); ``` even with more levels of nesting, such as ``` println!("{}", foo!(bar!(deprecated_function()))); ```
Uses the same approach as #17286 (and subsequent changes making it more correct), where the visitor will skip any pieces of the AST that are from "foreign code", where the spans don't line up, indicating that that piece of code is due to a macro expansion. If this breaks your code, read the error message to determine which feature gate you should add to your crate. Closes #18102 [breaking-change]
Uses the same approach as rust-lang/rust#17286 (and subsequent changes making it more correct), where the visitor will skip any pieces of the AST that are from "foreign code", where the spans don't line up, indicating that that piece of code is due to a macro expansion. If this breaks your code, read the error message to determine which feature gate you should add to your crate, and bask in the knowledge that your code won't mysteriously break should you try to use the 1.0 release. Closes #18102 [breaking-change]
Uses the same approach as rust-lang/rust#17286 (and subsequent changes making it more correct), where the visitor will skip any pieces of the AST that are from "foreign code", where the spans don't line up, indicating that that piece of code is due to a macro expansion. If this breaks your code, read the error message to determine which feature gate you should add to your crate. Closes #18102 [breaking-change]
Closes #17185.
The stability lint will now check code generated by macro expansion. It will allow to detect :
Checking is still done at expansion, but it will also detect a macro explicitly using a deprecated item in its definition.