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

Can lints provide new annotations? #529

Open
jdm opened this issue Dec 25, 2022 · 5 comments
Open

Can lints provide new annotations? #529

jdm opened this issue Dec 25, 2022 · 5 comments

Comments

@jdm
Copy link

jdm commented Dec 25, 2022

I'm experimenting with migrating Servo away from deprecated compiler plugins to using dylint. It's almost working, but I haven't figured out how to support a lint that looks for the presence of an annotation on a type (example). The original plugin is here, and I've found the following:

  • #[cfg_attr(dylint_lib = "lint", unrooted_must_root_lint::must_root)] allows the non-dylint build to success
  • the dylint build fails with use of undeclared crate or module "unrooted_must_root_lint"
  • if I add an rlib output to the lint crate and add a dependency on it, I get could not find "unrooted_must_root_lint" in "lint" when I try lint::unrooted_must_root_lint::must_rot

I have pushed my reduced testcase to https://github.com/jdm/dylint-test. Am I missing something, or is this not something that dylint can support?

@smoelius
Copy link
Collaborator

If I'm understanding the problem correctly, we need to find a way to make unrooted_must_root_lint::must_root a valid attribute.

Offhand, I can think of two ways to do this:

  • Create a dummy procedural macro named must_root within a module named unrooted_must_root_lint. An easy way to test this idea is to use the noop-attr crate. More specifically, you could add noop-attr as a dependency to app, and add something like the following to main.rs:
    mod unrooted_must_root_lint {
        pub use noop_attr::noop as must_root;
    }
  • Use register_tool. This would essentially give you a namespace over which you had complete control. Specifically, you could add something like the following to main.rs:
    #![cfg_attr(dylint_lib = "lint", feature(register_tool))]
    #![cfg_attr(dylint_lib = "lint", register_tool(unrooted_must_root_lint))]

As you can see from the tracking issue, register_tool has an uncertain future. So, if it were up to me, I would prefer the first option. I would not rely on noop-attr as a long term solution, however; I would implement my own no-op procedural macro.

Please let me know if I have misunderstood the problem.

@jdm
Copy link
Author

jdm commented Dec 25, 2022

That sounds promising! I'll give noop-attr a try.

@jdm
Copy link
Author

jdm commented Dec 25, 2022

It's not clear to me if the dummy proc macvro will work. jdm/dylint-test@816eaee is my attempt at it, and I see checking U#0 app/src/main.rs:9:1: 11:2 (#0) []. This means that the lint doesn't see any attributes attached to the item, so the no-op proc macro has apparently caused the attribute to be removed from the AST.

@smoelius
Copy link
Collaborator

The following seems to work to recover the attributes:

fn attr_def_ids(mut span: rustc_span::Span) -> Vec<rustc_hir::def_id::DefId> {
    use rustc_span::hygiene::{walk_chain, ExpnKind, MacroKind};
    use rustc_span::{ExpnData, SyntaxContext};

    let mut def_ids = Vec::new();
    while span.ctxt() != SyntaxContext::root() {
        let expn_data = span.ctxt().outer_expn_data();
        if let ExpnData {
            kind: ExpnKind::Macro(MacroKind::Attr, _),
            macro_def_id: Some(def_id),
            ..
        } = expn_data
        {
            def_ids.push(def_id);
        }
        span = walk_chain(span, SyntaxContext::root());
    }
    def_ids
}

When I run it in a fork of your example like this:

        println!(
            "checking {:?} {:?} {:?} {:?}",
            item.ident,
            item.span,
            attrs,
            attr_def_ids(item.span)
        );

I see:

checking U#5 app/src/main.rs:4:1: 4:38 (#5) [] [DefId(20:4 ~ unrooted_must_root_lint[77b3]::must_root)]

Note, however, that I had to change the procedural macro from noop-attr to the below. It appears that for the output of a procedural macro to be considered "expanded," its span must have changed.

pub fn must_root(
    _: proc_macro::TokenStream,
    body: proc_macro::TokenStream,
) -> proc_macro::TokenStream {
    body.into_iter()
        .map(|mut tree| {
            tree.set_span(proc_macro::Span::call_site());
            tree
        })
        .collect()
}

Another approach, alternative to using ExpnData like above, would be to have an additional pre-expansion pass lint that does nothing but fill a static map from Spans to Vec<Attribute>s. Not having tested this idea, I suspect you could access the attributes through rustc_ast::ast::Item's attr's field. Such an approach could use a simpler procedural macro and avoid the span-changing hack. Having said that, pre-expansion pass lints also have an uncertain future (e.g., see rust-lang/rust#94397).

Yet another approach I briefly considered was simulating register_tool inside register_lints. However, this appears to be a dead end. The code involved in register_tool (e.g., here) would not seem to make this possible/easy. Moreover, a non-trivial change would seem to be required to support our use case.

Sorry that there is no obvious, one, "right" solution to this problem (at least not right now).

@jdm
Copy link
Author

jdm commented Dec 26, 2022

Thank you so much!

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

No branches or pull requests

2 participants