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. |
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. |
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 |
test/expr/closure/trailing.swift
Outdated
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.
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.