[SR-8147] Fixit corrects .eventTrackingRunLoopMode to .RunLoop.Mode.eventTracking with invalid leading period

[jckarter]

Previous ID	SR-8147
Radar	rdar://problem/41545427
Original Reporter	@jckarter
Type	Bug

Attachment: Download

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, DiagnosticsQoI, StarterBug
Assignee	None
Priority	Medium

md5: 8dc18aea989964e226ca7f5bd1d9df9f

Issue Description:

Compiling the following code emits a fixit to change ‘.eventTrackingRunLoopMode’ to ‘.RunLoop.Mode.eventTracking’ with a leading period which does not compile since the type is fully specified already.

import AppKit

func foo(timer: Timer) {
  RunLoop.current.add(timer, forMode: .eventTrackingRunLoopMode)
}

[swift-ci]

Comment by Mani Ramezan (JIRA)

Hi @jckarter, I'm interested in taking this bug if possible with some help and guidance if possible.

[jckarter]

Great! @nkcsgexi or @benlangmuir, any pointers to help Mani get started?

[nkcsgexi]

Mani (JIRA User), what is the error message emitted from the compiler in this case? One trick is to search the message to find the culprit diagnostics and fixits to start digging.

[swift-ci]

Comment by Mani Ramezan (JIRA)

@nkcsgexi So it seems to be Swift 4.2 issue, it works fine on Xcode 9.4.1. The error message is

'eventTrackingRunLoopMode' has been renamed to 'RunLoop.Mode.eventTracking'

Seems by searching the codebase for "has been renamed to" I should be able to find the spot it's set and fix the fixit there.

[nkcsgexi]

That seems to me the right way to go🙂

[swift-ci]

Comment by Mani Ramezan (JIRA)

@nkcsgexi Hi, I finally got the time to get back to this. So I guess I have a possible solution for it. Here is where the error is set that triggers the fixitReplace. It already has the check for space, so I tried following which worked as expected:

if (!Str.empty()) {
    if (extractCharBefore(SM, charRange.getStart()) == '.')
        charRange = toCharSourceRange(SM, SourceRange(R.Start.getAdvancedLoc(-1), R.End));
}

Let me know if that works or the fix should happen in some other parts.

[swift-ci]

Comment by Mani Ramezan (JIRA)

Hi @nkcsgexi,

Let me know whenever you get a chance to check my above comment and if that's something that you think is safe to do for this fix. I'd probably need some guidance on adding unit tests for it also if the decision was made to go with this approach.

Thanks a lot

[swift-ci]

Comment by Mani Ramezan (JIRA)

Hi @jckarter / @benlangmuir,

I was wondering if you could help me on this. Trying to figure out if the above solution is something that I should move forward and open up a PR and try to figure out where to add unit tests for it or if I should start looking into another parts of the code and fix it there.

Appreciate your help.

[nkcsgexi]

Mani (JIRA User), i think your approach makes sense. That means we always replace contextual enum kinds with fully typed names, right?

[swift-ci]

Comment by Mani Ramezan (JIRA)

@nkcsgexi Yes, it replaces .eventTrackingRunLoopMode with RunLoop.Mode.eventTracking. I guess it has its cons and pros. Like it gives more info about namespacing that has changed vs. missing user's code convention .eventTracking

[nkcsgexi]

Sounds great! Could you open a PR to fix this fixit so that we can iterate from there?

[swift-ci]

Comment by Mani Ramezan (JIRA)

Hi @nkcsgexi,

Opened up the PR for this here: #20915

[swift-ci]

Comment by Mani Ramezan (JIRA)

CodaFi (JIRA User) Thank you for closing the existing PR. I'm still interested in giving this another try, but would probably need some mentoring / help if you or anybody that you know would be able to help. Totally understand this might not be possible or take some time and would be also more than happy to unassign this from myself so others could take a shot.

[CodaFi]

The problem with the first approach was that it changed the DiagnosticEngine. You should be able to adjust the source range for the fixit itself to remove the dot. Changing the DiagnosticEngine changes its behavior for every diagnostic, which is undesirable in general.

[swift-ci]

Comment by Mani Ramezan (JIRA)

@CodaFi Thanks for the details and your help. I investigated it more and found the source of fixit. Would something like this be a possible solution? I built the toolchain and seems working. Would probably need to add guards maybe for make sure -1 is a valid location probably.

[swift-ci]

Comment by Mani Ramezan (JIRA)

Hi @CodaFi, happy holidays! I went ahead and fixed some issues with the solution and added unit tests also for confirming. At the moment, all tests are passing. However, there's one issue with this fix which is if you have `RunLoop.Mode.eventTrackingRunLoopMode`, it ends up with `RunLoop.Mode.RunLoop.Mode.eventTrackingRunLoopMode`. I'm not familiar with LLVM and available classes to see if it's possible to indicate that it's a qualified name and update range accordingly.

[CodaFi]

I don't think you should have to bend over backwards like that in Sema. This code is missing handling for enum cases in general. You can check if renamedDecl is an EnumElementDecl then check if you have a DotSyntaxCallExpr and adjust the range back if needed.

[swift-ci]

Comment by Mani Ramezan (JIRA)

I might be wrong, but unless it's translated to enum somewhere during compile time, this is a static let member. renamedDecl is type of VarDecl. I might've checked it wrong also, but it seems call is null. I checked for enums and it's working as expected. _let : NSTextAlignment = .Center was the case that I tested.

[CodaFi]

Ah, you're right. In that case, do you have access to a `MemberRefExpr`? If so, then you can use the dot loc there to push the range back.

[swift-ci]

Comment by Mani Ramezan (JIRA)

That's what I initially wanted to do, but couldn't find a way to get `MemberRefExpr` from existing data. `call` is null, and I wasn't able to find a way to get `MemberRefExpr` from existing arguments.

[swift-ci]

Comment by Mani Ramezan (JIRA)

@CodaFi I also posted the question in forums, but no luck. Should I try maillist or is there anybody specific that you know I can reach out for help on how to access DotSyntaxExpr in this part of code?