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

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

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

zackmdavis
Copy link
Member

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.

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2018
@@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not well-thought-out comment deleted)

@petrochenkov petrochenkov assigned petrochenkov and unassigned eddyb Apr 3, 2018
@petrochenkov 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
@petrochenkov
Copy link
Contributor

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
Copy link
Contributor

bors commented Apr 6, 2018

📌 Commit 2bf8b09 has been approved by petrochenkov

@bors 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
Copy link
Contributor

bors commented Apr 6, 2018

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

@bors 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
@bors
Copy link
Contributor

bors commented Apr 9, 2018

🔒 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
Copy link
Member Author

(rebased)

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 9, 2018

📌 Commit a1d90a2 has been approved by petrochenkov

@bors 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 added a commit to kennytm/rust that referenced this pull request Apr 11, 2018
…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.
bors added a commit that referenced this pull request Apr 11, 2018
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:
@bors bors merged commit a1d90a2 into rust-lang:master Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants