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.

Sign up for GitHub

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

Jump to bottom

warn -> deny duplicate match bindings #59394

Merged
merged 1 commit into from
Mar 29, 2019
Merged

warn -> deny duplicate match bindings #59394

merged 1 commit into from
Mar 29, 2019

Conversation

mark-i-m
Copy link
Member

@mark-i-m mark-i-m commented Mar 24, 2019 •
edited
Loading

This is the next step of #57742

r? @Centril

  • Decide whether to go to deny-by-default or hard error.
    • My preference is to make this deny-by-default, rather than going straight to a hard error. The CI should fail because I haven't updated the ui test yet. I'll update it when we decide which to do.
  • Update test
  • Crater run see warn -> deny duplicate match bindings #59394 (comment)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2019
@Centril
Copy link
Contributor

Centril commented Mar 24, 2019

@mark-i-m Crater had just 1 regression so I think moving this to a hard error directly is sensible. Indeed it would have been fine to jump to a hard error immediately based on that 1 single regression.

@Centril Centril 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 Mar 27, 2019
@Centril
Copy link
Contributor

Centril commented Mar 28, 2019

@mark-i-m Thoughts on ^ ?

@mark-i-m
Copy link
Member Author

Sorry, I’ve been a bit swamped lately.

Personally, my preference is for us to just go through the normal process. I don’t think this change is very time-sensitive, the existing lint already gives a large portion of the benefit of this lint. Also, I still think we in general should move towards being more strict about breakage as Rust matures. IMHO, pushing this small change through really fast might set a bad precedent.

That said, I dont have a very strong opinion.

Making the change itself will be pretty easy, so i can do that later today if we decide what to do.

@Centril
Copy link
Contributor

Centril commented Mar 28, 2019

Also, I still think we in general should move towards being more strict about breakage as Rust matures.

We should distinguish between fixing bugs, soundness holes, and non-bugs. Since this is a bug-fix I think other considerations should apply (i.e. I don't even want to call this "breakage"). I also disagree with the general notion of being more strict with maturity. If anything, as time goes by and more code exists, we will need to become more loose to not drown in technical debt. We are already more strict than most language communities in my view (including C++, JS, ...), becoming even stricter is too much.

IMHO, pushing this small change through really fast might set a bad precedent.

This is a precedent I would prefer to set. It is a good idea to base how we handle future-compat procedures based on real world crater data. I think it's also a good idea to speed up the handling of these lints as compared to the 1+ year it might take now. In my view, we should take tech debt seriously and get rid of it as soon as we can reasonably do so.

Making the change itself will be pretty easy, so i can do that later today if we decide what to do.

That said, I don't mind going for Deny on this PR. I leave it up to you which strategy to pick... :)

@mark-i-m
Copy link
Member Author

@Centril Thanks :)

I think this gets into a larger discussion... which I can open an internals thread on if you want. I think it's an important and interesting discussion.

While I do agree that this is a bug fix, the crater run shows that making this a hard error could cause existing code to stop compiling. I'm a bit torn, but still leaning towards going through the Deny first. I will update this PR shortly with that change.

However, your point about tech debt is also good, so I have also prepared this branch to make the change to a hard error, so we can open a PR with it in a cycle or so...

@Centril
Copy link
Contributor

Centril commented Mar 28, 2019

I think this gets into a larger discussion... which I can open an internals thread on if you want.

Not just now.. ;) Too many other things to attend to 😂.

the crater run shows that making this a hard error could cause existing code to stop compiling.

(Right, my point was that we fixed the singular regression that was found, so presumably crater would show up 0 now)

However, your point about tech debt is also good, so I have also prepared this branch to make the change to a hard error, so we can open a PR with it in a cycle or so...

Excellent!

@mark-i-m mark-i-m force-pushed the dup-matcher-bindings-2 branch from 2c70c55 to f059bc5 Compare March 28, 2019 19:26
@mark-i-m mark-i-m force-pushed the dup-matcher-bindings-2 branch from f059bc5 to 9f14e14 Compare March 28, 2019 19:27
@mark-i-m mark-i-m marked this pull request as ready for review March 28, 2019 19:27
@mark-i-m
Copy link
Member Author

@Centril This should be ready. It is passing locally.

@Centril
Copy link
Contributor

Centril commented Mar 28, 2019

Looks good. I think we can dispense with crater...

r=me with green travis.

@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 28, 2019
@mark-i-m
Copy link
Member Author

@Centril Thanks :) Travis is green now.

@mark-i-m
Copy link
Member Author

Do we want a crater run first?

@Centril
Copy link
Contributor

Centril commented Mar 29, 2019

Nah; I'm happy with the first one... @bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 29, 2019

📌 Commit 9f14e14 has been approved by Centril

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 29, 2019
@Centril
Rollup merge of rust-lang#59394 - mark-i-m:dup-matcher-bindings-2, r=…
f9262af 
…Centril

warn -> deny duplicate match bindings

This is the next step of rust-lang#57742

r? @Centril

- [x] Decide whether to go to deny-by-default or hard error.
     - My preference is to make this deny-by-default, rather than going straight to a hard error. The CI should fail because I haven't updated the ui test yet. I'll update it when we decide which to do.
- [x] Update [test](https://github.com/mark-i-m/rust/blob/c25d6b83441e0c060ee0273193ef27b29e1318cd/src/test/ui/macros/macro-multiple-matcher-bindings.rs)
- [ ] ~Crater run~ see rust-lang#59394 (comment)
@Centril Centril mentioned this pull request Mar 29, 2019
Merged
bors added a commit that referenced this pull request Mar 29, 2019
@bors
Auto merge of #59513 - Centril:rollup, r=Centril
41dee75 
Rollup of 11 pull requests

Successful merges:

 - #58019 (Combine all builtin late lints and make lint checking parallel)
 - #59358 (Use `track_errors` instead of hand rolling)
 - #59394 (warn -> deny duplicate match bindings)
 - #59401 (bootstrap: build crates under libtest with -Z emit-stack-sizes)
 - #59423 (Visit path in `walk_mac`)
 - #59468 (musl: build toolchain libs with -fPIC)
 - #59476 (Use `SmallVec` in `TokenStreamBuilder`.)
 - #59496 (Remove unnecessary with_globals calls)
 - #59498 (Use 'write_all' instead of 'write' in example code)
 - #59503 (Stablize {f32,f64}::copysign().)
 - #59511 (Fix missed fn rename in #59284)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Mar 29, 2019
@bors
Auto merge of #59513 - Centril:rollup, r=Centril
003382e 
Rollup of 11 pull requests

Successful merges:

 - #58019 (Combine all builtin late lints and make lint checking parallel)
 - #59358 (Use `track_errors` instead of hand rolling)
 - #59394 (warn -> deny duplicate match bindings)
 - #59401 (bootstrap: build crates under libtest with -Z emit-stack-sizes)
 - #59423 (Visit path in `walk_mac`)
 - #59468 (musl: build toolchain libs with -fPIC)
 - #59476 (Use `SmallVec` in `TokenStreamBuilder`.)
 - #59496 (Remove unnecessary with_globals calls)
 - #59498 (Use 'write_all' instead of 'write' in example code)
 - #59503 (Stablize {f32,f64}::copysign().)
 - #59511 (Fix missed fn rename in #59284)

Failed merges:

r? @ghost
@bors bors merged commit 9f14e14 into rust-lang:master Mar 29, 2019
@mark-i-m mark-i-m mentioned this pull request Mar 29, 2019
Closed
3 tasks
@mark-i-m mark-i-m deleted the dup-matcher-bindings-2 branch May 25, 2019 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Centril Centril

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mark-i-m @Centril @bors @rust-highfive