Prohibit patterns in trait methods without bodies #37378

petrochenkov · Oct 24, 2016

They are not properly type checked

trait Tr {
    fn f(&a: u8); // <- This compiles
}

, mostly rejected by the parser already and generally don't make much sense.
This PR is kind of a missing part of #35015.

Given the statistics from crater, the effect of this PR is mostly equivalent to improving unused_mut lint.

cc #35078 (comment) #35015 rust-lang/rfcs#1685 #35203
r? @eddyb

eddyb · Oct 24, 2016

Started crater run.

eddyb · Oct 24, 2016

Crater run looks pretty bad (177 root regressions).

petrochenkov · Oct 24, 2016

Something is wrong with root/non-root partitioning in crater.
Two very popular crates - rustc-serialize and num-traits have this problem, most of others seem to break because of these dependencies.
I turned the error into a deny-by-default lint, @eddyb can you run crater once more to determine the real number of root regressions? (Unless you think it's waste of resources.)

Anyway, it looks like we can't make this an error in the nearest future, but I still hope to proceed with a warn-by-default lint.

eddyb · Oct 24, 2016

@petrochenkov Maybe they don't show themselves? They might be broken or missing?

EDIT: started a second run, only on the "after" half.

eddyb · Oct 25, 2016

Second crater report shows only 22 root regressions.

petrochenkov · Oct 25, 2016

mut arg - 19 crates (if only unused_mut worked in this context...).
&arg - 3 crates.

I've updated the PR with warning-by-default and tracking issue number.
Also #35203 needs to be reopened (I have no rights to do it).

eddyb · Oct 25, 2016

@bors r+

bors · Oct 25, 2016

📌 Commit 82d4200 has been approved by eddyb

Manishearth · Oct 26, 2016

Fails tidy, d230a7e

TimNN · Oct 26, 2016

The used error code (E0570) has been merged by a different PR (#36421) since this PR was opened (thus this PR currently fails travis when merged).

Manishearth · Oct 26, 2016

Manishearth reviewed Oct 26, 2016

View changes

Manishearth · Oct 26, 2016

src/librustc_passes/diagnostics.rs

@@ -228,4 +228,5 @@ pub impl Foo for Bar {
 register_diagnostics! {
    E0472, // asm! is unsupported on this target
    E0561, // patterns aren't allowed in function pointer types
+    E0570, // patterns aren't allowed in methods without bodies


This seems to be unused?

retep998 · Oct 26, 2016

I believe this caused #37416 to fail

https://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/2155

error: patterns aren't allowed in methods without bodies
   --> tests\cargotest\support/mod.rs:604:34
    |
604 |     fn tap<F: FnOnce(&mut Self)>(mut self, callback: F) -> Self;
    |                                  ^^^^^^^^
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #35203 <https://github.com/rust-lang/rust/issues/35203>
note: lint level defined here
   --> tests\cargotest\lib.rs:1:9
    |
1   | #![deny(warnings)]
    |         ^^^^^^^^

error: aborting due to previous error

petrochenkov · Oct 26, 2016

@bors r=eddyb

bors · Oct 26, 2016

📌 Commit 811a2b9 has been approved by eddyb

bors · Oct 29, 2016

⌛️ Testing commit 811a2b9 with merge e61c918...

bors · Oct 29, 2016

💔 Test failed - auto-win-msvc-64-cargotest

TimNN · Oct 29, 2016

This probably needs to update the sha at https://github.com/rust-lang/rust/blob/master/src/tools/cargotest/main.rs#L27 to at least rust-lang/cargo@806e3c3

petrochenkov · Oct 29, 2016

@bors r=eddyb

bors · Oct 29, 2016

📌 Commit 4ca11ce has been approved by eddyb

bors · Oct 29, 2016

⌛️ Testing commit 4ca11ce with merge 75a87c5...

bors · Oct 29, 2016

WaDelma · Nov 14, 2016

@petrochenkov
Hey that crater run that said 22 root regressions didn't fail to build my crate even though it quite few anonymous parameters in traits (poisson::algorithm::Creator and poisson::algorithm::Algorithm). Was the lint broken or is there something I am missing?

Edit. Ah the first one was broken already from rustc-serialize and num-traits and didn't reveal that the crate would fail to build even without them...

(And this is why I would really like central place to check crater runs...)

petrochenkov · Nov 14, 2016

@WaDelma
This lint reports patterns in parameters of trait methods without body

fn f(&arg: Type);
fn g(mut arg: Type);

, anonymous parameters

fn h(Type);

are not reported (yet, waiting for rust-lang/rfcs#1685).

WaDelma · Nov 14, 2016

Ah okay. That was the part I was missing. :D

rust-highfive assigned eddyb Oct 24, 2016

petrochenkov force-pushed the petrochenkov:nopat branch from f4cc508 to e41f9c9 Oct 24, 2016

bluss referenced this pull request Oct 24, 2016
Merged
traits: Remove pattern in trait's method signature #238

petrochenkov referenced this pull request Oct 25, 2016
Open
Tracking issue for patterns in functions without body #35203
3 of 5 tasks complete

petrochenkov force-pushed the petrochenkov:nopat branch from e41f9c9 to 82d4200 Oct 25, 2016

Manishearth referenced this pull request Oct 26, 2016
Closed
Rollup of 11 pull requests #37416

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 26, 2016

Remove duplicate error code (fixup rust-lang#37378)

d230a7e

TimNN referenced this pull request Oct 26, 2016
Merged
Fix rust-lang/rust#35203 warning/error #3231

Prohibit patterns in trait methods without bodies

Loading status checks…

811a2b9

petrochenkov force-pushed the petrochenkov:nopat branch from 82d4200 to 811a2b9 Oct 26, 2016

Update cargo sha for cargotest

Loading status checks…

4ca11ce

bors merged commit 4ca11ce into rust-lang:master Oct 29, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors referenced this pull request Oct 29, 2016
Merged
detect extra region requirements in impls #37167

bluss referenced this pull request Nov 1, 2016
Closed
Bounds parsing refactoring #37511

petrochenkov deleted the petrochenkov:nopat branch Mar 16, 2017

varkor referenced this pull request Aug 4, 2018
Open
Allow destructuring parameters in trait function declarations #53046

saleemrashid referenced this pull request Nov 19, 2018
Open
Error on compilation for verifying MPC on macOS #11

rust-lang/rust

Join GitHub today

Prohibit patterns in trait methods without bodies #37378

Conversation

petrochenkov commented Oct 24, 2016 • edited

rust-highfive assigned eddyb Oct 24, 2016

This comment has been minimized.

eddyb commented Oct 24, 2016

This comment has been minimized.

eddyb commented Oct 24, 2016

petrochenkov force-pushed the petrochenkov:nopat branch from f4cc508 to e41f9c9 Oct 24, 2016

This comment has been minimized.

petrochenkov commented Oct 24, 2016

This comment has been minimized.

eddyb commented Oct 24, 2016 • edited

bluss referenced this pull request Oct 24, 2016

homu added a commit to rust-num/num that referenced this pull request Oct 24, 2016

This comment has been minimized.

eddyb commented Oct 25, 2016

petrochenkov referenced this pull request Oct 25, 2016

petrochenkov force-pushed the petrochenkov:nopat branch from e41f9c9 to 82d4200 Oct 25, 2016

This comment has been minimized.

petrochenkov commented Oct 25, 2016

This comment has been minimized.

eddyb commented Oct 25, 2016

This comment has been minimized.

bors commented Oct 25, 2016

Manishearth referenced this pull request Oct 26, 2016

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 26, 2016

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 26, 2016

This comment has been minimized.

Manishearth commented Oct 26, 2016

This comment has been minimized.

TimNN commented Oct 26, 2016

Manishearth reviewed Oct 26, 2016 View changes

This comment has been minimized.

bors added a commit that referenced this pull request Oct 26, 2016

This comment has been minimized.

retep998 commented Oct 26, 2016

TimNN referenced this pull request Oct 26, 2016

bors added a commit to rust-lang/cargo that referenced this pull request Oct 26, 2016

petrochenkov force-pushed the petrochenkov:nopat branch from 82d4200 to 811a2b9 Oct 26, 2016

This comment has been minimized.

petrochenkov commented Oct 26, 2016

This comment has been minimized.

bors commented Oct 26, 2016

This comment has been minimized.

bors commented Oct 29, 2016

bors added a commit that referenced this pull request Oct 29, 2016

This comment has been minimized.

bors commented Oct 29, 2016

This comment has been minimized.

TimNN commented Oct 29, 2016

This comment has been minimized.

petrochenkov commented Oct 29, 2016

This comment has been minimized.

bors commented Oct 29, 2016

This comment has been minimized.

bors commented Oct 29, 2016

bors added a commit that referenced this pull request Oct 29, 2016

This comment has been minimized.

bors commented Oct 29, 2016

Hide details View details bors merged commit 4ca11ce into rust-lang:master Oct 29, 2016 2 checks passed

2 checks passed

bors referenced this pull request Oct 29, 2016

bluss referenced this pull request Nov 1, 2016

This comment has been minimized.

WaDelma commented Nov 14, 2016 • edited

This comment has been minimized.

petrochenkov commented Nov 14, 2016

This comment has been minimized.

WaDelma commented Nov 14, 2016

petrochenkov deleted the petrochenkov:nopat branch Mar 16, 2017

varkor referenced this pull request Aug 4, 2018

saleemrashid referenced this pull request Nov 19, 2018

petrochenkov commented Oct 24, 2016 •

edited

petrochenkov force-pushed the petrochenkov:nopat branch from `f4cc508` to `e41f9c9` Oct 24, 2016

eddyb commented Oct 24, 2016 •

edited

petrochenkov force-pushed the petrochenkov:nopat branch from `e41f9c9` to `82d4200` Oct 25, 2016

Manishearth reviewed Oct 26, 2016

View changes

petrochenkov force-pushed the petrochenkov:nopat branch from `82d4200` to `811a2b9` Oct 26, 2016

bors merged commit `4ca11ce` into rust-lang:master Oct 29, 2016
2 checks passed

WaDelma commented Nov 14, 2016 •

edited