[Diagnostics] Fix diagnostics to produce valid rename fixit for SubscriptExpr #39091
Conversation
mininny
force-pushed
the
fix-rename-subscript
branch
from
e669592 to
a5093d4
Compare
mininny
force-pushed
the
fix-rename-subscript
branch
from
a5093d4 to
51176d4
Compare
mininny
changed the title
lib/Sema/TypeCheckAvailability.cpp
Outdated
| // replace square brackets with parens. | ||
|
|
||
| diag.fixItReplace(SE->getIndex()->getStartLoc(), | ||
| ("." + baseReplace.str()).str()); |
There was a problem hiding this comment.
Could the leading . be added to the string as a very first thing instead of concatenating here?
There was a problem hiding this comment.
I don't think that can happen since subscripts should always have self or a variable before the square bracket if my understanding is correct.
But this could happen in function → subscript rename, where the self. is implicitly created before the function.
function(param: 1) // implicit self.
→ should not be fixed to
[param: 1]
So I added an extra check in such scenario to check if the base expression of the function's call expression is implicit, and add self if it is. I'm assuming that if the function's base expression can be made implicit, it will always be self and be inside the same decl context. Is it correct, or should have to be checking for the decl context as well?
There was a problem hiding this comment.
I'm only suggesting to append "." to a string itself instead of using concatenation in the diagnose which would just end up allocating even more strings.
|
|
||
| func testUnavailableSubscripts(_ x: UnavailableSubscripts) { | ||
| _ = self[old: 3] // expected-error {{'subscript(old:)' has been renamed to 'subscript(new:)'}} {{14-17=new}} | ||
| _ = x[old: 3] // expected-error {{'subscript(old:)' has been renamed to 'subscript(new:)'}} {{11-14=new}} |
There was a problem hiding this comment.
Could you please add a couple of examples where subscript is renamed into a function?
There was a problem hiding this comment.
There is a test case for subscript→function few lines below on 1213-1214 :)
There was a problem hiding this comment.
Ah, I completely missed [ fix-it for that case...
lib/Parse/Parser.cpp
Outdated
| @@ -1405,6 +1405,11 @@ ParsedDeclName swift::parseDeclName(StringRef name) { | |||
| baseName = baseName.substr(7); | |||
| } | |||
|
|
|||
| // If the base name is prefixed by "subscript", it's an subscript. | |||
| if (baseName.startswith("subscript")) { | |||
There was a problem hiding this comment.
baseName == "subscript" is more relevant? At least, we should not treat, for example, subscriptTo(_:) as isSubscript
There was a problem hiding this comment.
Thanks :) I changed it to compare it directly
mininny
force-pushed
the
fix-rename-subscript
branch
from
64e72f4 to
47a95b7
Compare
mininny
force-pushed
the
fix-rename-subscript
branch
from
47a95b7 to
3e8b076
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Windows platform |
1 similar comment
|
@swift-ci please smoke test Windows platform |
Current fixit for renamed subscripts isn't working.
The resulting fix-it changes
self[42]intoself.subscript, which loses information, and doesn't even compile.This PR adds support for renaming between subscripts, as well as between subscript and function calls by adding special handling for subscripts.
Resolves SR-15091