in which the non-shorthand patterns lint keeps its own counsel in macros #49614

zackmdavis · 2018-04-03T03:20:17Z

In issue #49588, Michael Lamparski pointed out a scenario in which the
non-shorthand-field-patterns lint could be triggered by a macro-expanded
pattern, in a way which was direly unwieldy for the macro author to guard
against and unreasonable to expect the macro user to take into account. We can
avoid this by not linting patterns that come from macro-expansions. Although
this entails accepting "false negatives" where the lint could genuinely improve
macro-templated code, avoiding the reported "true-but-super-annoying positive"
may be worth the trade? (Some precedent for these relative priorities exists as
no. 47775 (5985b0b).)

Resolves #49588.

petrochenkov · 2018-04-03T09:08:57Z

src/librustc_lint/builtin.rs

@@ -173,6 +173,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NonShorthandFieldPatterns {
                }
                if let PatKind::Binding(_, _, ident, None) = fieldpat.node.pat.node {
                    if ident.node == fieldpat.node.name {
+                        if let Some(_) = fieldpat.span.ctxt().outer().expn_info() {


Value { $a: value } is also possible, so checking that any of the spans is from a macro would be more correct.

(The proper way to fix this issue is to use hygienic comparison, but we don't have necessary information available in HIR right now.)

we don't have necessary information available in HIR right now

Is this planned or being worked on? (If there's a plan, we can add a # FIXME: do hygenic comparison when this-and-such is complete comment.)

There's a plan, I'm working on some identifier/span changes that would include the fix for this issue and I was aware about it before #49588 was submitted, but there's no issue that can be referred to in FIXME.

(not well-thought-out comment deleted)

petrochenkov · 2018-04-05T23:59:59Z

Parser assigns some spans incorrectly, so #49614 (comment) can't be done without fixing the parser, but I don't want to delay landing this PR over that.
@bors r+ rollup

bors · 2018-04-06T00:00:00Z

📌 Commit 2bf8b09 has been approved by petrochenkov

bors · 2018-04-06T12:14:44Z

☔ The latest upstream changes (presumably #49154) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2018-04-09T08:38:44Z

🔒 Merge conflict

In issue rust-lang#49588, Michael Lamparski pointed out a scenario in which the non-shorthand-field-patterns lint could be triggered by a macro-expanded pattern, in a way which was direly unwieldy for the macro author to guard against and unreasonable to expect the macro user to take into account. We can avoid this by not linting patterns that come from macro-expansions. Although this entails accepting "false negatives" where the lint could genuinely improve macro-templated code, avoiding the reported "true-but-super-annoying positive" may be worth the trade? (Some precedent for these relative priorities exists as no. 47775 (5985b0b).) Resolves rust-lang#49588.

zackmdavis · 2018-04-09T15:54:08Z

(rebased)

petrochenkov · 2018-04-09T17:29:13Z

@bors r+

bors · 2018-04-09T17:29:14Z

📌 Commit a1d90a2 has been approved by petrochenkov

…trochenkov in which the non-shorthand patterns lint keeps its own counsel in macros In issue rust-lang#49588, Michael Lamparski pointed out a scenario in which the non-shorthand-field-patterns lint could be triggered by a macro-expanded pattern, in a way which was direly unwieldy for the macro author to guard against and unreasonable to expect the macro user to take into account. We can avoid this by not linting patterns that come from macro-expansions. Although this entails accepting "false negatives" where the lint could genuinely improve macro-templated code, avoiding the reported "true-but-super-annoying positive" may be worth the trade? (Some precedent for these relative priorities exists as no. 47775 (5985b0b).) Resolves rust-lang#49588.

Rollup of 14 pull requests Successful merges: - #49525 (Use sort_by_cached_key where appropriate) - #49575 (Stabilize `Option::filter`.) - #49614 (in which the non-shorthand patterns lint keeps its own counsel in macros) - #49665 (Small nits to make couple of tests pass on mips targets.) - #49781 (add regression test for #16223 (NLL): use of collaterally moved value) - #49795 (Properly look for uninhabitedness of variants in niche-filling check) - #49809 (Stop emitting color codes on TERM=dumb) - #49856 (Do not uppercase-lint #[no_mangle] statics) - #49863 (fixed typo) - #49857 (Fix "fp" target feature for AArch64) - #49849 (Add --enable-debug flag to musl CI build script) - #49734 (proc_macro: Generalize `FromIterator` impl) - #49730 (Fix ICE with impl Trait) - #48270 (Replace `structurally_resolved_type` in casts check.) Failed merges:

rust-highfive assigned eddyb Apr 3, 2018

zackmdavis mentioned this pull request Apr 3, 2018

#[warn(non_shorthand_field_patterns)] is a menace to pattern macros #49588

Closed

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2018

petrochenkov reviewed Apr 3, 2018

View reviewed changes

petrochenkov assigned petrochenkov and unassigned eddyb Apr 3, 2018

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2018

ExpHP mentioned this pull request Apr 3, 2018

hlist_pat can trigger #[warn(non_shorthand_field_patterns)] lloydmeta/frunk#107

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2018

zackmdavis force-pushed the the_phantom_menace branch from 2bf8b09 to a1d90a2 Compare April 9, 2018 15:53

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2018

kennytm mentioned this pull request Apr 11, 2018

Rollup of 14 pull requests #49875

Merged

bors merged commit a1d90a2 into rust-lang:master Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

in which the non-shorthand patterns lint keeps its own counsel in macros #49614

in which the non-shorthand patterns lint keeps its own counsel in macros #49614

zackmdavis commented Apr 3, 2018

petrochenkov Apr 3, 2018

zackmdavis Apr 3, 2018

petrochenkov Apr 3, 2018

zackmdavis Apr 5, 2018

petrochenkov commented Apr 5, 2018

bors commented Apr 6, 2018

bors commented Apr 6, 2018

bors commented Apr 9, 2018

zackmdavis commented Apr 9, 2018

petrochenkov commented Apr 9, 2018

bors commented Apr 9, 2018

in which the non-shorthand patterns lint keeps its own counsel in macros #49614

in which the non-shorthand patterns lint keeps its own counsel in macros #49614

Conversation

zackmdavis commented Apr 3, 2018

petrochenkov Apr 3, 2018

Choose a reason for hiding this comment

zackmdavis Apr 3, 2018

Choose a reason for hiding this comment

petrochenkov Apr 3, 2018

Choose a reason for hiding this comment

zackmdavis Apr 5, 2018

Choose a reason for hiding this comment

petrochenkov commented Apr 5, 2018

bors commented Apr 6, 2018

bors commented Apr 6, 2018

bors commented Apr 9, 2018

zackmdavis commented Apr 9, 2018

petrochenkov commented Apr 9, 2018

bors commented Apr 9, 2018