Warn on anon params in 2015 edition #53272
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Also, I still need to make sure tests pass... |
3 tasks
|
Also, I need to add a migration suggestion that rustfix can use. |
|
The job Click to expand the log.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 |
|
The job Click to expand the log.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 |
kennytm
added
T-lang
mark-i-m
changed the title
src/librustc_lint/builtin.rs
Outdated
| "detects anonymous parameters", | ||
| Edition::Edition2018 => Warn | ||
| Warn, | ||
| "detects anonymous parameters" |
There was a problem hiding this comment.
So, I think that the policy in general is that we only warn in migration mode -- but this is an unconditional warning, no? I don't really care either way, I just want to Do What's Right.
There was a problem hiding this comment.
We talked about this a bit yesterday on Discord; I think our conclusion was that warnings only during migration was the policy but that in this case an unconditional warning would be OK? Did you discuss this further?
There was a problem hiding this comment.
I believe if we just wanted to warn in migration mode, we would change this back to Allow and add it to the FutureCompatibilityLints array. I don't really see why it should be different from the other migration lints, but I don't have a strong preference.
There was a problem hiding this comment.
I don't see a strong reason to treat this differently from other migration lints either.
Well, wait, I just realized something -- isn't this supposed to be a hard error in the new Edition? I thought that was the whole idea here. =) That is, @Centril wanted to go from "just linting" to giving a "hard error", so as to make space for some other syntax changes we had in mind, so we decided to up the schedule (roughly along the lines of what I think you originally proposed, @mark-i-m)
There was a problem hiding this comment.
Yes, but I need to learn how to block the syntax in the new edition, whereas I already know how to change the lint. I was thinking that after this PR, we could submit another that removes the syntax in 2018 edition (I will need some help with that). In the mean time, we can at least start warning as early as possible, right?
There was a problem hiding this comment.
we could of course always do that in a follow-up PR...
There was a problem hiding this comment.
Anyway I basically don't care if we warn in 2015 always or only when migrating. I thought the typical policy was only the latter, though. The only reason to do otherwise would be to give a kind of "extra nudge" -- the fact that we accepted a separate RFC (pre-edition) which deprecated this syntax might be enough justification. This would be analogous to deprecating some API, which obviously doesn't have to occur on an edition boundary.
There was a problem hiding this comment.
Ok, I opened #53612. This might actually be a lot easier than I expected.
| @@ -125,7 +125,7 @@ trait InTraitDefnReturn { | |||
| // Allowed and disallowed in trait impls | |||
| trait DummyTrait { | |||
| type Out; | |||
There was a problem hiding this comment.
do we have already have a rustfix unit test for this? if not, can we add one?
There was a problem hiding this comment.
What do you mean by "this"? This particular file? Or the _: Type suggestion? I had previously added https://github.com/rust-lang/rust/pull/48309/files#diff-cff76de2568555816771ee1e45d8225f in #48309. Is that what you mean?
mark-i-m
force-pushed
the
anon_param_error_now
branch
from
8474bbd
to
c4f7289
Compare
|
☔ The latest upstream changes (presumably #52953) made this pull request unmergeable. Please resolve the merge conflicts. |
mark-i-m
force-pushed
the
anon_param_error_now
branch
from
c4f7289
to
e1bff31
Compare
1 task
|
I guess maybe it's orthogonal? @bors r+ |
|
📌 Commit e1bff318921b3d96afcd12997aa23db175eacfaf has been approved by |
bors
added
S-waiting-on-bors
|
Yes, they are orthogonal. This PR enables the warning in 2015. The other PR disables anonymous params in 2018. |
|
⌛ Testing commit e1bff318921b3d96afcd12997aa23db175eacfaf with merge 79ce3148a8f9f5ab6c8f58a8a16e40eda0aae5fa... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
Huh? Sorry, I didn't notice that Travis failed. |
|
The failure is from AppVeyor (Windows), not Travis 🙃 |
kennytm
added
S-waiting-on-author
bors
added a commit
that referenced
this pull request
Aug 25, 2018
…enkov Remove anonymous trait params from 2018 and beyond cc @Centril @nikomatsakis cc #41686 rust-lang/rfcs#2522 #53272 This PR removes support for anonymous trait parameters syntactically in rust 2018 and onward. TODO: - [x] Add tests
|
Thanks! Also, I think I forgot to change this to Allow-by-default as discussed in #53272 (comment). |
mark-i-m
force-pushed
the
anon_param_error_now
branch
from
9e41e9d
to
548f28e
Compare
|
@bors r+ |
|
📌 Commit 548f28e has been approved by |
bors
added
S-waiting-on-bors
|
@nikomatsakis I think this should be ready for one last review. Also since #53612 merged two days ago, this is already an error in 2018 edition. |
|
Oh, you beat me to it! Thanks :) |
bors
added a commit
that referenced
this pull request
Aug 28, 2018
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
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc #41686 rust-lang/rfcs#2522
cc @Centril @nikomatsakis
TODO:
_ : FooEDIT: It seems I already did the last two in #48309