Warn when macro shadows previous definition

[dirk]

Also contains small whitespace fix in same file (libsyntax/ext/tt/macro_rules.rs).

(related to #20192)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[alexcrichton]

Thanks for the PR! I think that unconditionally warning about using a feature of the macro system may not be a great idea, however. I've used this time to time in other projects and I think the error should at least be configurable via a lint (and I would personally prefer allow-by-default).

Could you also add some tests for this change if you'd like to pursue it?

[dirk]

@alexcrichton Sure thing! Was planning on adding configurability if this feature PR was welcome.

Test-wise were you looking for both a) testing that it issues the warning by default and b) testing that the configuration flag/lint properly silences the warning?

[alexcrichton]

Indeed! (in terms of testing)

[dirk]

So I'm working on implementing this as feature as a lint in the librustc built-in linters, however I'm struggling to figure out if macros defined by macro_rules! ever make it from the syntax context to the linter context (or the type-context it points out to). Any ideas if they do or do not?

[alexcrichton]

The definitions themselves live in the AST, and I believe macro definitions will live all the way through until translation.

As a lint, you'll probably want to modify the list of lints we have and then call .add_lint on the Session so it can be processed at the end of compilation.

[dirk]

Gotcha, I was digging through the LintPass hook API yesterday and was struggling to figure out which hook function would be triggered by a macro_rules! in the AST. Do you know off the top of your head if that gets fully translated into an ast::Mac (and therefore handled by an fn check_mac(...)), or would it end up as an ast::ItemMac or ast::StmtMac? (Or could it in theory end up as any of those depending on where it occurs?)

[alexcrichton]

I believe that the check_mac visitor function will hit all macros, and macro_rules! itself is a macro so it should be hit there. The ItemMac and StmtMac productions should also be covered by check_mac (they're the recursive call)

[dirk]

So I've added the following pass to the built-in set of lint passes (in librustc/lint/builtin.rs and in the body of register_builtin(...) in librustc/lint/context.rs).

impl LintPass for ShadowingMacrosPass {
    fn get_lints(&self) -> LintArray {
        lint_array!(SHADOWING_MACROS)
    }

    #[allow(unused_must_use)]
    fn check_mac(&mut self, cx: &Context, mac: &ast::Mac) {
        // Pull out the macro identifier out of the definition
        let ast::MacInvocTT(ref path, _, _) = mac.node;
        let id: ast::Ident = path.segments.last().unwrap().identifier;

        let id_str = id.name.as_str();
        io::stderr().write(format!("ShadowingMacrosPass#check_mac: {}\n", id_str).as_bytes());
    }
}

However, the check_mac(...) function seems to never be called. I did some digging and it looks like there are a few places where the compiler is skipping macro visiting; could those be possible culprits for why check_mac is being skipped over, or have I just done something dump somewhere?

[emberian]

Macros are gone after expansion. They're kept around in ast::Crate::exported_macros.

[dirk]

@cmr Gotcha, thanks!

So would the appropriate approach (for the time being) be to continue with the current strategy of having the shadow-check in macro-rule expansion code (what this PR is currently doing) and control whether or not that check happens via a crate-level attribute?

[emberian]

Yes, I think so. I think you'd just move the logic in that method into check_crate.

[alexcrichton]

@dirk you probably want to do the analysis where you're currently detecting it, but you can call the add_lint method on the Session (I think it's available) for the actual lint and node id to be processed later.

[dirk]

Woops, looks like it failed the tidy check. Will fix that with the next commit.

[alexcrichton]

Due to this being a known feature of the macro system, this should be Allow by default.

[dirk]

Gotcha, fixed!

[alexcrichton]

Closing due to inactivity, but feel free to reopen with comments addressed!

[dirk]

Oh, sorry! Was on a business trip last week and let this lapse. Will get back to it shortly.