Offer fix-its to disambiguate based on a trailing closure's label. #4777
Conversation
|
@swift-ci Please test |
|
@nkcsgexi, @DougGregor, what do you think? @rintaro, are there any test cases I missed that came up when you were doing the rename fix-it? |
|
Build failed |
|
Cool stuff! I wish we had this before so that the migrator does not need to worry about ambiguity after renaming. |
|
Yeah, unfortunately it happens after the migrator's already made its changes, so it can't just be auto-applied. But I figure you have good experience with these kinds of diagnostics by this point to be a good reviewer. |
jrose-apple
force-pushed
the
fix-it-trailing-closure-ambiguity
branch
from
201b431 to
ebca8ee
Compare
|
@swift-ci Please test |
|
oh, You are right. We've no way to select which note's fixit to apply after wiping out the original code. I've briefly read the commit and it looks good to me. |
jrose-apple
force-pushed
the
fix-it-trailing-closure-ambiguity
branch
from
ebca8ee to
9424975
Compare
|
Added one more diagnostic cleanup: 71b2bd39fae831a2b68f3cb115f3f0cd8e102a21. Would someone look over that too? |
|
Previous tests passed, so I'm just going to smoke test the latest addition. @swift-ci Please smoke test |
|
@swift-ci Please smoke test |
| func overloadOnSomeDefaultArgsOnly2(_: Int, y: Bool = true, a: () -> Void) {} // expected-note {{found this candidate}} | ||
|
|
||
| func overloadOnSomeDefaultArgsOnly3(_: Int, x: Int = 0, a: () -> Void) {} // expected-note {{found this candidate}} | ||
| func overloadOnSomeDefaultArgsOnly3(_: Int, x: Bool = true, a: () -> Void) {} // expected-note {{found this candidate}} |
There was a problem hiding this comment.
Fixing
overloadOnLabelSomeDefaultArgs(
1, x: 2
) {
// some
}I prefer
overloadOnLabelSomeDefaultArgs(
1, x: 2, a: {
// some
})than
overloadOnLabelSomeDefaultArgs(
1, x: 2
, a: {
// some
})Edit: Sorry, wrong line for the comment. 😓
There was a problem hiding this comment.
That's a good point. Should be doable. (And at some point it's probably worth factoring out "call-related source location utilities", but I'm hoping to cherry-pick this and want to keep it scoped.)
Enables Chris's auto-apply-fixes mode for -verify: if an expected-* annotation has the wrong message, or if the expected fix-its are incorrect, this option will **edit the original file** to update them. This is a tool for compiler developers only; it doesn't affect normal diagnostic printing or normal fix-its.
...unless the argument labels are the same for every possible overload. Only affects diagnostics.
(by making it a normal argument with a label and not a trailing closure) Diagnostic part of rdar://problem/25607552. A later commit will keep us from getting in this situation quite so much when default arguments are involved.
jrose-apple
force-pushed
the
fix-it-trailing-closure-ambiguity
branch
from
9424975 to
e6d6e0e
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
Note: split into a separate Radar, rdar://problem/28325311, so that rdar://problem/25607552 can deal with the fact that many such calls shouldn't be ambiguous in the first place (because one of the overloads has default arguments and the other doesn't). |
(by making it a normal argument with a label and not a trailing closure)
Diagnostic part of rdar://problem/25607552. A later commit will keep us from getting in this situation quite so much when default arguments are involved.
This PR also sneaks in a new frontend option for compiler hackers, -verify-apply-fixes, to enable @lattner's auto-apply-fixes mode for -verify. That's not really the focus here, and I don't plan on cherry-picking that to the 3.0 branch.