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

Don't consider macro-expansions in stability analysis? #15703

Closed
brson opened this issue Jul 16, 2014 · 8 comments · Fixed by #15726
Closed

Don't consider macro-expansions in stability analysis? #15703

brson opened this issue Jul 16, 2014 · 8 comments · Fixed by #15726

Comments

@brson
Copy link
Contributor

brson commented Jul 16, 2014

The expansion of fail! and println! both currently cause code to fail the stability check because they expand to reveal unstable implementation details. A seemingly reasonable solution to this would be to just not run the analysis on any code that has been macro-expanded.

cc @aturon

@brson brson added the A-libs label Jul 16, 2014
@aturon
Copy link
Member

aturon commented Jul 16, 2014

I definitely agree with this approach: we should be able to annotate a macro with a stability level, and that's what should count for its users.

Do we have a way of telling what part of an AST was generated via macro expansion by the time we get to linting?

@huonw
Copy link
Member

huonw commented Jul 16, 2014

Mostly; there's the expn_info field of a span: None for normal code, Some(...) for macro generated code (basically containing the backtrace of macro invocations that led to that point).

@aturon
Copy link
Member

aturon commented Jul 16, 2014

@huonw OK, that makes this sound like a pretty easy fix. I'll take a quick look at it tomorrow (unless @brson wants to?)

@huonw
Copy link
Member

huonw commented Jul 16, 2014

Hm, this seems slightly subtle, since the contents of a macro would then never be checked for stability. This differs to a conventional function, e.g. given #[stable] pub fn foo() { deprecated() }, foo() won't warn, but the contents of foo is still checked, while foo!() where #[stable] macro_rules! foo { () => { deprecated() } } is just never checked.

(Of course, macros and phasing are both feature gated so you have to opt-in to instability to use non-standard macros anyway.)

@aturon
Copy link
Member

aturon commented Jul 16, 2014

@huon Good point. Also, it looks tricky with the current infrastructure to even lint on the macro invocation -- the backtrace of macro expansion does not appear to link to the def_id of the macro (or give any other way of getting at the macro's stability).

Nevertheless, I think we should bail out of the lint when we hit the results of expansion; those should be checked, if anywhere, at the site of macro definition.

One other question: do syntax extensions also produce the span information you mentioned above? I ask because @brson's original problematic cases are written syntax extensions. If not, is there some other way of telling that an expansion happened?

@aturon
Copy link
Member

aturon commented Jul 16, 2014

@brson @huonw I have a patch ready that bails of out the lint on expanded code. This also seems to mask uses of println!.

If y'all are ok with this strategy for now, I'll file a PR.

@brson
Copy link
Contributor Author

brson commented Jul 16, 2014

@aturon Sounds good to me. Thanks!

aturon added a commit to aturon/rust that referenced this issue Jul 16, 2014
This small patch causes the stability lint to bail out when traversing
any AST produced via a macro expansion. Ultimately, we would like to
lint the contents of the macro at the place where the macro is defined,
but regardless we should not be linting it at the use site.

Closes rust-lang#15703
@aturon
Copy link
Member

aturon commented Jul 16, 2014

cc #15728

bors added a commit that referenced this issue Jul 18, 2014
This small patch causes the stability lint to bail out when traversing
any AST produced via a macro expansion. Ultimately, we would like to
lint the contents of the macro at the place where the macro is defined,
but regardless we should not be linting it at the use site.

Closes #15703
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants