[Refactoring] Fix an issue where refactoring misses to refactor references after prefix operators

[ahoppen]

The removed else if branch caused the argument to the - operator to be be reported as Mismatch. I couldn’t figure out what that branch was for, so I removed it.

@akyrtzi If you have any memory what the removed else if branch was for, I would be interested to hear it. But I assume that you don’t remember since it’s been nearly 5 years since you upstreamed local renaming.

rdar://91588948

[ahoppen]

@swift-ci Please smoke test

[akyrtzi]

This is syntactic rename, maybe @benlangmuir or @nathawes may remember.

[benlangmuir]

If the tests pass, I believe this case should be fine to remove. It was for renaming enum cases with labels, and was covered by syntactic-rename.swift test with the input from enum_case.in.json. From what I can tell, that test is still passing.

[nathawes]

Removing the branch seems fine if the enum case is still passing but I was expecting LabelRangeType to be None in this case so it wouldn't take that branch anyway. Assuming I'm remembering correctly, it might be worth looking into why NameMatcher is giving the reference to foo on line 2 of the new test a non-None LabelRangeType in case that's causing other problems too.

[ahoppen]

I did some more debugging and the following is happening:

We walk the argument list of -foo, which is foo. Usually argument lists start with a (, so it won’t match against the rename location of foo. But because prefix operators don’t have a parenthesis, we match the argument list to the rename location of the argument itself. Thus, we get the non-None LabelRangeType (it’s CallArg) and won’t match the actual argument anymore (because we have already matched that location).

AFAICT there’s no need to call tryResolve for the argument list itself because we‘ll never match the entire argument list against a rename location, we only want to match individual arguments. I think that code might have been added by @hamishknight when he introduced ArgumentList in #38836.

I kept the removal of the special handling for enum cases with labels in this PR as well because it seems like some good cleanup on its own accord. If anyone has feelings and wants to keep the case, I can add it back in.

[ahoppen]

@swift-ci Please smoke test

[hamishknight]

I believe this was trying to preserve the original logic for parens and tuples:

78d8d09#diff-5fb3c14eb2d2fd312df17ef5b7e60ab238f2ef17a9944c3feede1373bc64d391L385-L389

Though it seems that would have previously been skipped for implicit argument lists, which therefore would have been skipped for operator argument lists.

It seems like it was originally added in #30964 to handle callAsFunction cases, is this still needed @nathawes?

[nathawes]

Yeah I think we might need to keep it– indexing uses the open paren as the base location of the call for callAsFunction (since there's no function name).

[hamishknight]

Makes sense, we should probably restore the !isImplicit check in that case

[ahoppen]

Good idea 👍

[bnbarham]

Can we add a test for that case?

[nathawes]

I think it’s already covered by https://github.com/apple/swift/blob/main/test/refactoring/SyntacticRename/call-as-function.swift and a few others (which did fail before the latest commit).

[ahoppen]

@swift-ci Please smoke test

[ahoppen]

@swift-ci Please smoke test