Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jckarter opened this issue Jun 29, 2018 · 21 comments

Comments

@jckarter
Copy link
Member

@jckarter jckarter commented Jun 29, 2018

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
Copy link
Collaborator

@swift-ci swift-ci commented Jun 29, 2018

Comment by Mani Ramezan (JIRA)

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

@jckarter
Copy link
Member Author

@jckarter jckarter commented Jul 5, 2018

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

@nkcsgexi
Copy link
Member

@nkcsgexi nkcsgexi commented Jul 5, 2018

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
Copy link
Collaborator

@swift-ci swift-ci commented Jul 5, 2018

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
Copy link
Member

@nkcsgexi nkcsgexi commented Jul 5, 2018

That seems to me the right way to go🙂

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Oct 25, 2018

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
Copy link
Collaborator

@swift-ci swift-ci commented Nov 1, 2018

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
Copy link
Collaborator

@swift-ci swift-ci commented Nov 13, 2018

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
Copy link
Member

@nkcsgexi nkcsgexi commented Nov 14, 2018

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

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Nov 14, 2018

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
Copy link
Member

@nkcsgexi nkcsgexi commented Nov 14, 2018

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

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Nov 30, 2018

Comment by Mani Ramezan (JIRA)

Hi @nkcsgexi,

Opened up the PR for this here: #20915

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Nov 19, 2019

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
Copy link
Member

@CodaFi CodaFi commented Nov 21, 2019

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
Copy link
Collaborator

@swift-ci swift-ci commented Dec 5, 2019

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
Copy link
Collaborator

@swift-ci swift-ci commented Dec 26, 2019

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
Copy link
Member

@CodaFi CodaFi commented Jan 3, 2020

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
Copy link
Collaborator

@swift-ci swift-ci commented Jan 3, 2020

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
Copy link
Member

@CodaFi CodaFi commented Jan 3, 2020

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
Copy link
Collaborator

@swift-ci swift-ci commented Jan 3, 2020

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
Copy link
Collaborator

@swift-ci swift-ci commented Feb 10, 2020

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?

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants