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 unused-parens lint comes to cover function and method args #46980

Conversation

zackmdavis
Copy link
Member

Resolves #46137.

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@zackmdavis zackmdavis force-pushed the and_the_case_of_the_needlessly_parenthesized_arguments branch from 92e9c0d to b7739f3 Compare December 24, 2017 04:23
@zackmdavis
Copy link
Member Author

Travis failure looks dubious

@arielb1
Copy link
Contributor

arielb1 commented Dec 24, 2017

r? @petrochenkov

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 24, 2017

📌 Commit b7739f3 has been approved by petrochenkov

@petrochenkov petrochenkov added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 24, 2017
@bors
Copy link
Contributor

bors commented Dec 25, 2017

⌛ Testing commit b7739f3 with merge f5f2de1248978ff21036e8aa81b21c66b50fd051...

@bors
Copy link
Contributor

bors commented Dec 25, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Dec 25, 2017

@bors retry #47002

@bors
Copy link
Contributor

bors commented Dec 26, 2017

⌛ Testing commit b7739f3 with merge 56e8ea3665d395a697e89b3a4ca23a9a3c14f866...

@bors
Copy link
Contributor

bors commented Dec 26, 2017

💔 Test failed - status-travis

@petrochenkov
Copy link
Contributor

@bors retry #47002

@bors
Copy link
Contributor

bors commented Dec 27, 2017

⌛ Testing commit b7739f3 with merge 5ccdeea...

bors added a commit that referenced this pull request Dec 27, 2017
…nthesized_arguments, r=petrochenkov

in which the unused-parens lint comes to cover function and method args

Resolves #46137.
@bors
Copy link
Contributor

bors commented Dec 27, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Dec 27, 2017

Legit.

error: unnecessary parentheses around function argument
    --> src\librustc_apfloat\ieee.rs:1437:56
     |
1437 |         let exp_change = cmp::min(cmp::max(exp as i32, (-max_change - 1)), max_change);
     |                                                        ^^^^^^^^^^^^^^^^^ help: remove these parentheses
     |
note: lint level defined here
    --> src\librustc_apfloat\lib.rs:45:9
     |
45   | #![deny(warnings)]
     |         ^^^^^^^^
     = note: #[deny(unused_parens)] implied by #[deny(warnings)]
   Compiling rustc_binaryen v0.0.0 (file:///C:/projects/rust/src/librustc_binaryen)
error: aborting due to previous error
error: Could not compile `rustc_apfloat`.

@kennytm kennytm 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 Dec 27, 2017
@zackmdavis zackmdavis force-pushed the and_the_case_of_the_needlessly_parenthesized_arguments branch from b7739f3 to bff573c Compare December 31, 2017 03:41
@zackmdavis
Copy link
Member Author

Yes, with regrets, it turns out that the "Eh, this looks right; I don't have to tie up my cores running the tests locally because Travis will catch any needed fixes for me without wasting any reviewer-minutes" PR strategy is less effective when Travis is broken. Force-push bff573c is more likely to be what we want (although my local test build is unrelatedly flaking late in the test process, after the ui/run-pass/compile-fail tests have apparently succeeded).

@zackmdavis zackmdavis force-pushed the and_the_case_of_the_needlessly_parenthesized_arguments branch from bff573c to 6dee797 Compare December 31, 2017 05:25
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 31, 2017

📌 Commit 6dee797 has been approved by petrochenkov

@kennytm kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 31, 2017
@bors
Copy link
Contributor

bors commented Jan 6, 2018

📌 Commit 8a94e4c has been approved by petrochenkov

@kennytm
Copy link
Member

kennytm commented Jan 6, 2018

@bors r-

Something’s wrong with the new cargo @rust-lang/cargo.

[00:34:36] Cargo Book (x86_64-unknown-linux-gnu) - cargo
[00:34:36] thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 30, message: "Read-only file system" } }', libcore/result.rs:916:5
[00:34:36] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:34:36] 
[00:34:36] 
[00:34:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rustbook" "build" "/checkout/src/tools/cargo/src/doc/book" "-d" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc/cargo"

@kennytm kennytm added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2018
@bors
Copy link
Contributor

bors commented Jan 10, 2018

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

@petrochenkov
Copy link
Contributor

Blocked on #47280

@zackmdavis zackmdavis force-pushed the and_the_case_of_the_needlessly_parenthesized_arguments branch from 8a94e4c to 14982db Compare January 18, 2018 16:36
@zackmdavis
Copy link
Member Author

no. 47280 has landed; rebased

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 18, 2018
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 18, 2018

📌 Commit 14982db has been approved by petrochenkov

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2018
@bors
Copy link
Contributor

bors commented Jan 20, 2018

⌛ Testing commit 14982db with merge 15a1e28...

bors added a commit that referenced this pull request Jan 20, 2018
…nthesized_arguments, r=petrochenkov

in which the unused-parens lint comes to cover function and method args

Resolves #46137.
@bors
Copy link
Contributor

bors commented Jan 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 15a1e28 to master...

@bors bors merged commit 14982db into rust-lang:master Jan 20, 2018
@zackmdavis zackmdavis deleted the and_the_case_of_the_needlessly_parenthesized_arguments branch January 20, 2018 23:24
zackmdavis added a commit to zackmdavis/rust that referenced this pull request Jan 31, 2018
In rust-lang#46980 ("in which the unused-parens lint..." (14982db)), 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 rust-lang#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 rust-lang#47775.
kennytm added a commit to kennytm/rust that referenced this pull request Feb 3, 2018
…ssary_unnecessary_parens, r=nikomatsakis

decline to lint technically-unnecessary parens in function or method arguments inside of nested macros

In rust-lang#46980 ("in which the unused-parens lint..." (14982db)), 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 rust-lang#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 rust-lang#47775.
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
…ssary_unnecessary_parens, r=nikomatsakis

decline to lint technically-unnecessary parens in function or method arguments inside of nested macros

In rust-lang#46980 ("in which the unused-parens lint..." (14982db)), 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 rust-lang#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 rust-lang#47775.
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
…ssary_unnecessary_parens, r=nikomatsakis

decline to lint technically-unnecessary parens in function or method arguments inside of nested macros

In rust-lang#46980 ("in which the unused-parens lint..." (14982db)), 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 rust-lang#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 rust-lang#47775.
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.

7 participants