[QoI] Handle out of order arguments by reordering them #10672
Conversation
There was a problem hiding this comment.
Thank you for the patch, @d-ronnqvist!
I think it would be great if we could use the old style errors + fix-its but do it right instead of asking users to drag label through every argument position, which is what is happening right now unfortunately :( It seems to me that errors, in their current form, are more "actionable" and remove some of cognitive load from users comparing to the new error messages, where users would have to parse label string to figure out differences between right and wrong positions, having fix-it which replaces all of the arguments together doesn't help either.
Could you please add couple more test-cases were arguments are more complex than single integer or string literals e.g. function calls and/or closures?
|
@xedin How about this? Now it has the same error message as it previously did, but the fix-it moves the out-of-place argument into the correct location in one go. The only "extra" source manipulation is to move a ", " along with the moved argument. I also added a few more new tests but wasn't entirely sure what to test. Suggestions are appreciated. ( I rebased my changes. Sorry if the review requests got lost in doing so. |
|
@d-ronnqvist Awesome! I'll take a closer look soon, just need to get few things done first. Thanks, again! |
|
@swift-ci please smoke test |
lib/Sema/CSSimplify.cpp
Outdated
| continue; | ||
| } | ||
| unsigned argIdx = 0; | ||
| // Enumerate the parameters and their bindings to see if any arguments is our of order |
| // expected-error @-1 {{argument 'b' must precede argument 'c'}} {{26-26=b: { (num: Int) -> String in return String(describing: num) }, }} {{38-101=}} | ||
| fun_31849281(a: { !$0 }, c: [nil, 42], b: { "\($0)" }) | ||
| // expected-error @-1 {{argument 'b' must precede argument 'c'}} {{26-26=b: { "\\($0)" }, }} {{38-54=}} | ||
|
|
There was a problem hiding this comment.
This is the example I've made which had a broken fix-it before but looks correct now:
func foo(x: Int, y: Int) {
}
foo(42, x: 0)
There was a problem hiding this comment.
Actually I'm not sure if its correct because it highlights the whole region, can you please verify?
There was a problem hiding this comment.
I think it's correct in highlighting the range that's being removed and the unlabeled argument it needs to precede. Having one argument in between shows that it's not the whole region:
(swift) func bar(x: Int, y: Int, z: Int) {}
(swift) bar(42, y: 10, x: 0)
error: argument 'x' must precede unnamed argument #1
bar(42, y: 10, x: 0)
~~ ~~^~~~
x: 0,
But when there's only two arguments the two ranges meet and it looks like the whole range. It might look better if it didn't highlight the preceding comma in the range that's being removed. But I don't know if the highlight when using fixItRemove can be changed.
There was a problem hiding this comment.
Sounds good, please add it to the tests for completeness.
|
@d-ronnqvist And I forgot to mention - could you also please run |
|
@xedin Did you want me to squash all commits or only the modifications to the second commit (this solution)? |
|
@d-ronnqvist Sorry, please squash all 3 into a single one, I'm going to nominate it for 4.0 as well. |
Change the fix-it to move the argument to its correct location in one go. This happens by removing it from one location and inserting it in the other (as opposed to the original implementation which swapped one argument with the preceding one). The commas separating the arguments are adjusted to match the moved argument. Add new tests for reordering regular arguments, variadic arguments, and function arguments. Resolves: SR-4715 rdar://problem/31849281
|
@swift-ci please smoke test |
|
@d-ronnqvist Thank you! |
Change handling of out of order arguments to reorder all arguments according to the parameter order instead of just performing one swap.
I'm not sure if this change is in the spirit of the original
outOfOrderArgumentmethod or not and if it's what @dabrahams had in mind for SR-4715 or not.As I see it, the new diagnostic message is more general - for better and worse - but the fix-it can rearrange multiple arguments at once. In many of the existing test cases where there are only two arguments that need to be swapped this new can be a step back in terms of clarity. At the same time I find that seeing listing all the arguments can provide additional context.
As for the fix-it; in simple cases it's likely unnecessary to modify all of the arguments, but it allows this changed code to deal with many arguments that are completely out of order.
Resolves SR-4715