Make anon params lint warn-by-default by mark-i-m · Pull Request #48309 · rust-lang/rust

mark-i-m · 2018-02-17T23:36:37Z

This is intended as a followup on anonymous parameters deprecation.

Cross-posting from #41686:

After having read a bit more of the discussion that I can find, I propose a more aggressive deprecation strategy:
We make the lint warn-by-default as soon as possible
We make anon parameters a hard error at the epoch boundary

cc @matklad @est31 @aturon

rust-highfive · 2018-02-17T23:36:40Z

r? @estebank

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

matklad · 2018-02-18T11:21:01Z

I personally don't feel that we need to rush this into the new epoch. What about turning the warning one in the new epoch instead, and then turning it into a hard error in the latter epoch?

Actually, if the epoch includes some drastic change with significant amount of churn, like the rethinking of absolute paths, I would prefer even to not turn a warning on for this very first epoch. It seems interesting to make the very first epoch as small as possible in terms of the actual changes, gain experience with epochs as a thing, and postpone more churn-prone changes to latter epochs.

mark-i-m · 2018-02-18T13:42:33Z

Hmm... That's definitely an important consideration. I guess the question is do we want to minimize trivially rustfix-able changes as much as more substantial ones like path changes?

One could also argue that by warning now, we would minimize churn in the new epoch...

estebank · 2018-02-18T20:42:10Z

cc @rust-lang/lang

nikomatsakis · 2018-02-19T17:06:34Z

r? @nikomatsakis

petrochenkov · 2018-02-19T18:28:51Z

Nit: error/warning messages conventionally start with lowercase.

nikomatsakis · 2018-02-22T01:20:00Z

cc @rust-lang/lang -- I'd like to get some further thoughts on the right "stabilization schedule" here.

est31 · 2018-02-22T13:01:21Z

and postpone more churn-prone changes to latter epochs.

I don't think this particular change is churn prone, as in it doesn't affect many crates. We could turn it into an error though and make a crater run but I doubt the number of crates would be large.

I think a good idea would be to put this lint into the "pre-epoch" group of lints (is there such a group yet?) but otherwise to keep it allow by default. On the epoch switch, we can then turn it cleanly into a new error.

matklad · 2018-02-22T13:21:58Z

I don't think this particular change is churn prone, as in it doesn't affect many crates. We could turn it into an error though and make a crater run but I doubt the number of crates would be large.

FWIW, we've already measured that in the RFC (using IntelliJ and not crater :) ) At that time, 416 out of 5560 crates were affected (and, wow, we now have 14115 crates)

https://github.com/matklad/rfcs/blob/anon-params/text/0000-deprecate-anonymous-parameters.md#backward-compatibility

mark-i-m · 2018-02-22T17:42:21Z

(and, wow, we now have 14115 crates)

Total? Or that use anon params?

mark-i-m · 2018-03-05T20:45:50Z

Ping @rust-lang/lang

cramertj · 2018-03-05T21:02:57Z

This change seems fine to me, although this seems like more of a policy issue than a language design issue. IMO we should feel free to introduce warnings whenever we want, rather than waiting to bundle a bunch of new warnings with the next epoch.

nikomatsakis · 2018-03-09T23:47:54Z

We've been reviewing general epoch policy, and it's worth considering how this would fit in (cc #48796). The basic idea is that we divide changes into two categories:

Epochal errors are those things that will become a hard error at the turn of the epoch (of course, old code will continue to work as ever).
Idiom shifts are those things that will become deprecated at the turn of the epoch
- This might be through a deny-by-default lint or maybe warn-by-default, not yet decided.
- The important point is that Epoch 2018 code can continue using the features, possibly by marking the lint as 'allow'.

In Rust 2015 code, these two errors are treated differently:

Epochal errors will be warned against by default.
Idiom shifts will be "allow by default", but you can opt into warnings via a rust_2018_idiom lint group. (The idea is not to pester people who are on Rust 2015 and annoy them unless they want the errors.)

I think that in general we prefer to avoid hard errors in the new epoch.

Considered on the merits, it feels like this error should probably be an "idiom shift" sort of lint for the new epoch -- there is no particular reason we can't recover from it, given that we must continue parsing anyhow. OTOH, I'm pretty sure we can get away with making it a hard error.

Still, I think that my vote would be for idiom shift.

nikomatsakis · 2018-03-12T17:15:38Z

Nominating to discuss in lang team meeting and make a decision on how to proceed.

nikomatsakis · 2018-03-22T20:49:18Z

We discussed this in the @rust-lang/lang meeting but didn't reach a firm conclusion. I think I will start an internals thread to discuss this and other similar questions about how aggressive to be in the Rust 2018 transition. Personally, I lean towards saying that in the Rust 2018 edition we can classify this as a "unforced deprecation" -- and therefore start warning/linting, rather than giving a hard error right away.

rust-highfive · 2018-05-23T17:31:11Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:00:05] ....................................................................................................
[01:00:08] ....................................................................................................
[01:00:14] ....................................................................................................
[01:00:20] ....................................................................................................
[01:00:25] ............................................................................................F.......
[01:00:38] .....................................................i..............................................
[01:00:43] .........................................................................ii.........................
[01:00:50] ....................................................................................................
[01:00:58] .............................................................................i.................iiiii
[01:00:58] .............................................................................i.................iiiii
[01:01:00] iiii...................................................
[01:01:00] failures:
[01:01:00] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[01:01:00] [01:01:00] ---- [ui] ui/lint-anon-param-edition.rs stdout ----
[01:01:00] diff of stderr:
[01:01:00] [01:01:00] 1 warning: anonymous parameters are deprecated and will be removed in the next edition.
[01:01:00] - --> $DIR/lint-anon-param-edition.rs:18:12
[01:01:00] + --> $DIR/lint-anon-param-edition.rs:18:11
[01:01:00] 3 |
[01:01:00] 4 LL | fn foo(u8);
[01:01:00] - | ^^ help: Try naming the parameter or explicitly ignoring it: `_: u8`
[01:01:00] + | ^ help: Try naming the parameter or explicitly ignoring it: `_: u8`
[01:01:00] 6 |
[01:01:00] 7 = note: #[warn(anonymous_parameters)] on by default
[01:01:00] 8 = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
[01:01:00] [01:01:00] The actual stderr differed from the expected stderr.
[01:01:00] The actual stderr differed from the expected stderr.
[01:01:00] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint-anon-param-edition/lint-anon-param-edition.stderr
[01:01:00] diff of fixed:
[01:01:00] [01:01:00] 15 // run-rustfix
[01:01:00] 16 [01:01:00] 17 trait Foo {
[01:01:00] - fn foo(_: u8);
[01:01:00] + fn foo_: u8u8);
[01:01:00] 19 //^ WARN anonymous parameters are deprecated
[01:01:00] 20 //| WARN this was previously accepted
[01:01:00] 21 }
[01:01:00] [01:01:00] The actual fixed differed from the expected fixed.
[01:01:00] The actual fixed differed from the expected fixed.
[01:01:00] Actual fixed saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint-anon-param-edition/lint-anon-param-edition.fixed
[01:01:00] To update references, rerun the tests and pass the `--bless` flag
[01:01:00] To only update this specific test, also pass `--test-args lint-anon-param-edition.rs`
[01:01:00] error: 2 errors occurred comparing output.
[01:01:00] status: exit code: 0
[01:01:00] status: exit code: 0
[01:01:00] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/lint-anon-param-edition.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint-anon-param-edition/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--edition=2018" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint-anon-param-edition/auxiliary" "-A" "unused"
[01:01:00] ------------------------------------------
[01:01:00] [01:01:00] ------------------------------------------
[01:01:00] stderr:
[01:01:00] stderr:
[01:01:00] ------------------------------------------
[01:01:00] {"message":"anonymous parameters are deprecated and will be removed in the next edition.","code":{"code":"anonymous_parameters","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/ui/lint-anon-param-edition.rs","byte_start":638,"byte_end":639,"line_start":18,"line_end":18,"column_start":11,"column_end":12,"is_primary":true,"text":[{"text":" fn foo(u8);","highlight_start":11,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(anonymous_parameters)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!","code":null,"level":"warning","spans":[],"children":[],"rendered":null},{"message":"for more information, see issue #41686 <https://github.com/rust-lang/rust/issues/41686>","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"Try naming the parameter or explicitly ignoring it","code":null,"level":"help","spans":[{"file_name":"/checkout/src/test/ui/lint-anon-param-edition.rs","byte_start":638,"byte_end":639,"line_start":18,"line_end":18,"column_start":11,"column_end":12,"is_primary":true,"text":[{"text":" fn foo(u8);","highlight_start":11,"highlight_end":12}],"label":null,"suggested_replacement":"_: u8","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"warning: anonymous parameters are deprecated and will be removed in the next edition.\n --> /checkout/src/test/ui/lint-anon-param-edition.rs:18:11\n |\nLL | fn foo(u8);\n | ^ help: Try naming the parameter or explicitly ignoring it: `_: u8`\n |\n = note: #[warn(anonymous_parameters)] on by default\n = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!\n = note: for more information, see issue #41686 <https://github.com/rust-lang/rust/issues/41686>\n\n"}
[01:01:00] ------------------------------------------
[01:01:00] [01:01:00] thread '[ui] ui/lint-anon-param-edition.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3044:9
[01:01:00] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[01:01:00] test result: FAILED. 1438 passed; 1 failed; 16 ignored; 0 measured; 0 filtered out
[01:01:00] [01:01:00] [01:01:00] [01:01:00] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:01:00] [01:01:00] [01:01:00] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:01:00] Build completed unsuccessfully in 0:03:14
[01:01:00] Build completed unsuccessfully in 0:03:14
[01:01:00] Makefile:58: recipe for target 'check' failed
[01:01:00] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:03d3f164
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@mark-i-m

Fix span for type-only arguments Currently it points to the comma or parenthesis before the type, which is broken cc @mark-i-m this is what broke rust-lang#48309 r? @estebank

estebank · 2018-05-24T16:19:09Z

@mark-i-m you can rebase to master now and yes, squashing your commits would be best.

mark-i-m · 2018-05-24T17:39:30Z

Oh gosh... I messed up the rebase :/

Will fix and repush...

mark-i-m · 2018-05-24T17:41:37Z

Ok, much better.

Also, this passed travis after I rebased on master but before I squashed, so it should pass now too.

@nikomatsakis Time for one last review?

bors · 2018-05-25T16:10:36Z

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

nikomatsakis · 2018-05-25T20:58:07Z

@bors delegate=mark-i-m

bors · 2018-05-25T20:58:07Z

✌️ @mark-i-m can now approve this pull request

nikomatsakis · 2018-05-25T20:58:29Z

@mark-i-m r=me post rebase; I delegated to you so you should be able to instruct bors with r=nikomatsakis and it should listen to you :)

mark-i-m · 2018-05-27T19:15:15Z

@bors r=nikomatsakis

bors · 2018-05-27T19:15:15Z

📌 Commit 0e53b78 has been approved by nikomatsakis

bors · 2018-05-27T22:28:17Z

⌛ Testing commit 0e53b78 with merge 5f308ee...

@matklad

Make anon params lint warn-by-default This is intended as a followup on anonymous parameters deprecation. Cross-posting from #41686: > After having read a bit more of the discussion that I can find, I propose a more aggressive deprecation strategy: > - We make the lint warn-by-default as soon as possible > - We make anon parameters a hard error at the epoch boundary cc @matklad @est31 @aturon

bors · 2018-05-28T00:35:44Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 5f308ee to master...

@Centril

Warn on anon params in 2015 edition cc #41686 rust-lang/rfcs#2522 cc @Centril @nikomatsakis TODO: - [x] Make sure the tests pass. - [x] Make sure there is rustfix-able suggestion. Current plan is to just suggest `_ : Foo` - [x] Add a rustfix ui test. EDIT: It seems I already did the last two in #48309

rust-highfive assigned estebank Feb 17, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2018

rust-highfive assigned nikomatsakis and unassigned estebank Feb 19, 2018

petrochenkov reviewed Feb 19, 2018
View reviewed changes

emilyalbini added T-lang Relevant to the language team S-waiting-on-team and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2018

emilyalbini 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-team labels Mar 12, 2018

nikomatsakis added the I-nominated label Mar 12, 2018

nikomatsakis added S-waiting-on-team and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2018

nikomatsakis removed the I-nominated label Mar 22, 2018

emilyalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team labels Apr 16, 2018

mark-i-m force-pushed the anon_param_lint branch from c958bb9 to c296c8eCompare May 24, 2018 15:13

mark-i-m force-pushed the anon_param_lint branch from c296c8e to 7a62425Compare May 24, 2018 17:38

mark-i-m force-pushed the anon_param_lint branch from 7a62425 to 4a16855Compare May 24, 2018 17:40

Make anon params lint warn-by-default
0e53b78

mark-i-m force-pushed the anon_param_lint branch from 4a16855 to 0e53b78Compare May 27, 2018 19:08

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-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2018

mark-i-m mentioned this pull request May 27, 2018
Tracking issue for RFC#1685: Deprecate anonymous parameters #41686
Closed
3 tasks

bors merged commit 0e53b78 into rust-lang:masterMay 28, 2018

bors mentioned this pull request May 28, 2018
don't ask what edition we are in; ask what edition a span is in #50999
Closed

mati865 mentioned this pull request May 29, 2018
Update to nightly 2018-05-28 rust-lang/rust-clippy#2817
Merged

Centril mentioned this pull request Aug 10, 2018
RFC: Generalized Type Ascription rust-lang/rfcs#2522
Closed

mark-i-m mentioned this pull request Aug 11, 2018
Warn on anon params in 2015 edition #53272
Merged
3 tasks

mark-i-m deleted the anon_param_lint branch November 14, 2018 16:40

Uh oh!

Uh oh!

Conversation

mark-i-m commented Feb 17, 2018

Uh oh!

rust-highfive commented Feb 17, 2018

Uh oh!

matklad commented Feb 18, 2018

Uh oh!

mark-i-m commented Feb 18, 2018

Uh oh!

estebank commented Feb 18, 2018

Uh oh!

nikomatsakis commented Feb 19, 2018

Uh oh!

petrochenkovFeb 19, 2018

Choose a reason for hiding this comment

Uh oh!

mark-i-mFeb 19, 2018

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Feb 22, 2018

Uh oh!

est31 commented Feb 22, 2018

Uh oh!

matklad commented Feb 22, 2018

Uh oh!

mark-i-m commented Feb 22, 2018

Uh oh!

mark-i-m commented Mar 5, 2018

Uh oh!

cramertj commented Mar 5, 2018

Uh oh!

nikomatsakis commented Mar 9, 2018

Uh oh!

nikomatsakis commented Mar 12, 2018

Uh oh!

nikomatsakis commented Mar 22, 2018

Uh oh!

rust-highfive commented May 23, 2018

Uh oh!

estebank commented May 24, 2018

Uh oh!

mark-i-m commented May 24, 2018

Uh oh!

mark-i-m commented May 24, 2018

Uh oh!

bors commented May 25, 2018

Uh oh!

nikomatsakis commented May 25, 2018

Uh oh!

bors commented May 25, 2018

Uh oh!

nikomatsakis commented May 25, 2018

Uh oh!

mark-i-m commented May 27, 2018

Uh oh!

bors commented May 27, 2018

Uh oh!

bors commented May 27, 2018

Uh oh!

bors commented May 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants