From 5985b0b03523f9fead022d55e8ecaaf001731d0b Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Tue, 30 Jan 2018 17:13:05 -0800 Subject: [PATCH] wherein the parens lint keeps its own counsel re args in nested macros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In #46980 ("in which the unused-parens lint..." (14982db2d6)), the unused-parens lint was made to check function and method arguments, which it previously did not (seemingly due to oversight rather than willful design). However, in #47775 and discussion thereon, user–developers of Geal/nom and graphql-rust/juniper reported that the lint was seemingly erroneously triggering on certain complex macros in those projects. While this doesn't seem like a bug in the lint in the particular strict sense that the expanded code would, in fact, contain unncecessary parentheses, it also doesn't seem like the sort of thing macro authors should have to think about: the spirit of the unused-parens lint is to prevent needless clutter in code, not to give macro authors extra heartache in the handling of token trees. We propose the expediency of declining to lint unused parentheses in function or method args inside of nested expansions: we believe that this should eliminate the petty, troublesome lint warnings reported in the issue, without forgoing the benefits of the lint in simpler macros. It seemed like too much duplicated code for the `Call` and `MethodCall` match arms to duplicate the nested-macro check in addition to each having their own `for` loop, so this occasioned a slight refactor so that the function and method cases could share code—hopefully the overall intent is at least no less clear to the gentle reader. This is concerning #47775. --- src/librustc_lint/unused.rs | 37 ++++++++++++----- ...775-nested-macro-unnecessary-parens-arg.rs | 40 +++++++++++++++++++ ...nested-macro-unnecessary-parens-arg.stderr | 15 +++++++ 3 files changed, 83 insertions(+), 9 deletions(-) create mode 100644 src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs create mode 100644 src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index ef6475f9ee4c7..56f863ab3aa84 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -302,19 +302,38 @@ impl EarlyLintPass for UnusedParens { Assign(_, ref value) => (value, "assigned value", false), AssignOp(.., ref value) => (value, "assigned value", false), InPlace(_, ref value) => (value, "emplacement value", false), - Call(_, ref args) => { - for arg in args { - self.check_unused_parens_core(cx, arg, "function argument", false) + // either function/method call, or something this lint doesn't care about + ref call_or_other => { + let args_to_check; + let call_kind; + match *call_or_other { + Call(_, ref args) => { + call_kind = "function"; + args_to_check = &args[..]; + }, + MethodCall(_, ref args) => { + call_kind = "method"; + // first "argument" is self (which sometimes needs parens) + args_to_check = &args[1..]; + } + // actual catch-all arm + _ => { return; } } - return; - }, - MethodCall(_, ref args) => { - for arg in &args[1..] { // first "argument" is self (which sometimes needs parens) - self.check_unused_parens_core(cx, arg, "method argument", false) + // Don't lint if this is a nested macro expansion: otherwise, the lint could + // trigger in situations that macro authors shouldn't have to care about, e.g., + // when a parenthesized token tree matched in one macro expansion is matched as + // an expression in another and used as a fn/method argument (Issue #47775) + if e.span.ctxt().outer().expn_info() + .map_or(false, |info| info.call_site.ctxt().outer() + .expn_info().is_some()) { + return; + } + let msg = format!("{} argument", call_kind); + for arg in args_to_check { + self.check_unused_parens_core(cx, arg, &msg, false); } return; } - _ => return, }; self.check_unused_parens_core(cx, &value, msg, struct_lit_needs_parens); } diff --git a/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs b/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs new file mode 100644 index 0000000000000..b4e6c5074e3d3 --- /dev/null +++ b/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs @@ -0,0 +1,40 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// must-compile-successfully + +#![warn(unused_parens)] + +macro_rules! the_worship_the_heart_lifts_above { + ( @as_expr, $e:expr) => { $e }; + ( @generate_fn, $name:tt) => { + #[allow(dead_code)] fn the_moth_for_the_star<'a>() -> Option<&'a str> { + Some(the_worship_the_heart_lifts_above!( @as_expr, $name )) + } + }; + ( $name:ident ) => { the_worship_the_heart_lifts_above!( @generate_fn, (stringify!($name))); } + // ↑ Notably, this does 𝘯𝘰𝘵 warn: we're declining to lint unused parens in + // function/method arguments inside of nested macros because of situations + // like those reported in Issue #47775 +} + +macro_rules! and_the_heavens_reject_not { + () => { + // ↓ But let's test that we still lint for unused parens around + // function args inside of simple, one-deep macros. + #[allow(dead_code)] fn the_night_for_the_morrow() -> Option { Some((2)) } + //~^ WARN unnecessary parentheses around function argument + } +} + +the_worship_the_heart_lifts_above!(rah); +and_the_heavens_reject_not!(); + +fn main() {} diff --git a/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr b/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr new file mode 100644 index 0000000000000..097ec1b1c8010 --- /dev/null +++ b/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr @@ -0,0 +1,15 @@ +warning: unnecessary parentheses around function argument + --> $DIR/issue-47775-nested-macro-unnecessary-parens-arg.rs:32:83 + | +32 | #[allow(dead_code)] fn the_night_for_the_morrow() -> Option { Some((2)) } + | ^^^ help: remove these parentheses +... +38 | and_the_heavens_reject_not!(); + | ------------------------------ in this macro invocation + | +note: lint level defined here + --> $DIR/issue-47775-nested-macro-unnecessary-parens-arg.rs:13:9 + | +13 | #![warn(unused_parens)] + | ^^^^^^^^^^^^^ +