[Ready] Swift Programming Language Completions

[jerrymarino]

Fundamental Swift programming language support

This patch implements fundamental Swift support in YCMD. It includes elemental features needed to get completions working for Swift in Vim:

• Swift completion support via a Swift Completer implementation.
• Compilation database support to setup the compiler stack on the backend.

It integrates with core Swift tooling through an HTTP service, SwiftySwiftVim, a cross platform service founded to meet the needs of this project. The backend leverages the highest level Swift tooling APIs: a client forsourcekitd.

What it doesn't address:

• diagnostic support - ( follow up task )
• additional semantic features like GoToDefinition
• generating compilation databases - this the work of a build systems i.e. Bazel, or in Xcode XCPretty or XcodeCompilationDatabase.

YCMD Integration:

• The insertion_text is a basic string containing text characters the user would type. (variables, method names and parameter names )
• Completion triggers should provide a user experience for what a Swift user would expect.

Build

• It does a source build of SwiftySwiftVim on the users machine. The build process uses CMake to do build the project and should have few system dependencies.
• The backend links the system sourcekitd dlyib by default and optionally user may override this.

Detailed Design

See the backend implementation design over at SwiftySwiftVim.

---

This change is 

[puremourning]

Thanks for sending a PR! This is very cool.

I haven't looked at the code yet, but thought best to ping @oblitum who I think also made a swift prototype. So would very much value his input :)

[oblitum]

Some days ago I released what I've done until now, and it's on top of my fork, you may check its capabilities here: http://nosubstance.me/post/swift-code-completion-for-vim-on-linux/.

It's not relying on sourcekit because it has no support on Linux yet, since I wanted to do this for Linux too, I've produced my code based on swift-ide-test's code, it should also compile on OS X.

Anyway, my swift-ycm-core relies on Swift's build system, so... all-in-all, there's pros and cons. Mostly everybody is using sourcekit for this kind of job for a good reason, but it exists on OS X solely, I guess YouCompleteMe upstream leans more towards a sourcekit solution, despite losing some platforms, so, I dunno.

FYI, it's trying to embed what ycm-core/YouCompleteMe#1300 proposes.

[jerrymarino]

@puremourning thanks! I hope this PR can useful to understand the best way to getting swift support in Vim - I believe YCM is the best option.

@oblitum nice! I'm going to read through what you've got so far. For an upstream to YCMD I also thought using sourcekitd seemed like a good starting point and good fit. It might be interesting to implement a prototype against SourceKit API rather than sourcekitd, but I'm biased to using a stable API if possible.

I originally considered using the lower level API's, similar to swift-ide-test, but it seems like features are already exist at the SourceKit level, i.e. caching and fuzzy completion for now. Also, I think SourceKit (sourcekitd?) is the path of least resistance to keeping up with the latest swift releases as it is a higher level and theoretically more stable. i.e. I believe there are other consumers/stakeholders of this API today, like Xcode. From my understanding, it is similar tradeoff to using clang-c vs clang directly.

From my understanding many APIs of SourceKit (i.e. SourceKitCore) are cross platform, but I haven't tried it out on linux yet. Did you find something here that wouldn't work on linux? The implementation of sourcekitd is based on XPC, so this definitely isn't cross platform today. Maybe sourcekitd would eventually be supported on linux and I saw a lot of FIXMEs in the source for portability of sourcekitd.

Also, I decoupled this one from the swift build system in hope eventually people wouldn't have to build swift from source, similar to how clang support was implemented in YCMD. I can add automation if we think this is the right way to do it.

[puremourning]

I believe YCM is the best option

No arguments here!

[puremourning]

On APIs, using the stable API is the way to go IMO, otherwise we're just going to get a bazillion "YCM doesn't build" issue reports. I'd also like to somehow fix the version/checkout of the swift compiler/API to prevent upstream changes to the "stable" api breaking our code.

[oblitum]

I originally considered using the lower level API's, similar to swift-ide-test, but it seems like features are already exist at the SourceKit level, i.e. caching and fuzzy completion for now.

I'm a bit curious regarding the position about using or not these features in the pull, since caching and fuzzy is also provided by ycmd's completion system.

Also, I think SourceKit (sourcekitd?) is the path of least resistance to keeping up with the latest swift releases as it is a higher level and theoretically more stable. i.e. I believe there are other consumers/stakeholders of this API today, like Xcode. From my understanding, it is similar tradeoff to using clang-c vs clang directly.

Not much sure about it keeping up with the latest more than what it relies on internally, but regarding theoretical stability I agree. I also agree it's a tradeoff like clang-c vs clang, but it's also somewhat similar to clang-c vs python-libclang, which lagged a bit because of the added a layer of abstraction. I even provided performance improvements at the time I used clang_complete, I'm glad YCM uses libclang, the C API version, and not the python one =). But anyways, the C API is also stable.

From my understanding many APIs of SourceKit (i.e. SourceKitCore) are cross platform, but I haven't tried it out on linux yet. Did you find something here that wouldn't work on linux?

Thinking back in time, I tried to compile it but definitely it was not supported outside OS X. I'm not sure about the current state, but I coudn't use SourceKit (dependencies on GCD and other non-existent stuff for Linux), nor sourcekitd.

The implementation of sourcekitd is based on XPC, so this definitely isn't cross platform today. Maybe sourcekitd would eventually be supported on linux and I saw a lot of FIXMEs in the source for portability of sourcekitd.

I hope so too. I'm not sure how things are going, but I was seeing SourceKit related stuff very much tied to Xcode/OSX enviroment, project structures, etc, if I recall correctly.

[puremourning]

GCD is cross-platform and open source now...

https://github.com/apple/swift-corelibs-libdispatch

[Valloric]

I'd love to see Swift semantic completion support in ycmd! :) Thanks for expressing interest in this.

We do have a general pattern for plugging in new semantic engines, and it starts with creating a new HTTP+JSON server. Here's more background on that.

Look at JediHTTP and racerd as examples. Both of those servers were built specifically to be used in ycmd.

This server-based approach is great for many reasons, not the least of which is that your server is disconnected from ycmd development as a separate repo. We then use it as a submodule with a thin client layer that sends HTTP requests.

We currently only have libclang running in-process for ycmd and that's likely to change in the future, so new in-process semantic completers are not something we want to do.

You're likely to have lots of questions while building this so we'd be happy to invite you to our ycm-dev mailing list (if you wish) where we'll be more than happy to provide any answers you need.

[oblitum]

@Valloric all links in the previous comment are broken at the moment (40min later).

[Valloric]

@oblitum Thanks, fixed.

[oblitum]

FYI. SourceKit now seems to build on Linux too: swiftlang/swift#3594 (comment).

[homu]

☔ The latest upstream changes (presumably #701) made this pull request unmergeable. Please resolve the merge conflicts.

[jerrymarino]

I wanted to post an update on this one.

Over the holiday weekend, I made some progress in the direction that we discussed above. I ported the original prototype to a design similar to other third party engines in the project: using a HTTP server based on JediHttp. It was pretty easy to get it working with JediHttp, thanks for the suggestion @Valloric 👍

Currently the server is up on GitHub https://github.com/jerrymarino/swiftyswiftvim. I've got it working end to end locally and can push my updated fork which integrates this into YCMD.

I expect to wrap this up within the next few weekends.

cc @oblitum @puremourning

[vheon]

I ported the original prototype to a design similar to other third party engines in the project: using a HTTP server based on JediHttp. It was pretty easy to get it working with JediHttp, thanks for the suggestion @Valloric 👍

Currently the server is up on GitHub https://github.com/jerrymarino/swiftyswiftvim. I've got it working end to end locally and can push my updated fork which integrates this into YCMD.

First off, kudos for the project 👍
I was looking at the it and saw that you wrapped it with a Python server which is the same we use for ycmd and JediHTTP wrapping your completer with boost/Python. I understand that making the HTTP layer in Python helped with the implementation of the handlers because you could look what JediHTTP implemented and replicate it for your swift completer. The only thing that came to mind is: did you consider NOT to wrap it with Python but with a C++ HTTP server library? I'm saying this because this way you're coupling your project to 2 tecnologies and I can assure you that making a compiled binary wrapped with Python is a nightmare in terms of compatibility. Just look at the issue tracker of YouCompleteMe for some examples. Plus right not might not be a "problem" because ycmd already uses Python and we would just use the same version as ycmd but in the future we might move away from Python.

[jerrymarino]

I was looking at the it and saw that you wrapped it with a Python server which is the same we use for ycmd and JediHTTP wrapping your completer with boost/Python.

Awesome, thank you for taking the time to check it out, and thank you for the awesome code! I hope we can get some level of Swift support into YCMD.

I understand that making the HTTP layer in Python helped with the implementation of the handlers because you could look what JediHTTP implemented and replicate it for your swift completer. The only thing that came to mind is: did you consider NOT to wrap it with Python but with a C++ HTTP server library? I'm saying this because this way you're coupling your project to 2 tecnologies and I can assure you that making a compiled binary wrapped with Python is a nightmare in terms of compatibility. Just look at the issue tracker of YouCompleteMe for some examples.

There were a few tradeoffs I considered when selecting a server framework for SwiftySwiftVim. Minimal development effort and maintainability are 2 key requirements, where effort is king. Due to my time constraints, I needed to get something working within in a few days.

Initially, I was leaning towards using a lightweight C++ server framework. I am heavily biased towards C++ here for the same reasons you mention and more.

After looking at sever options I and came to the following conclusions:

• YCMD is heavily reliant on python including the Completer, the test suite, and Completer subclasses. Given the current state, it seems like I need to manage the complexity multiple technologies even if the Swift backend server is totally in C++.

• JediHTTP already implements a known working server and client in YCMD. On the surface, the code seems reuseable for Swift or the Nth language. As proof, I was up and running within a few hours using the client/server stack 👍

• YCMD uses python and boost python already: I might be able to reuse some the YCMD tooling to handle the Python and Boost dependencies.

If there had been a JediHTTP equivalent in C or C++, I would have used it hands down. I didn't find an appealing Bottle/JediHTTP alternative and I didn't want to spend time implementing a client and server.

Plus right not might not be a "problem" because ycmd already uses Python and we would just use the same version as ycmd but in the future we might move away from Python.

Today, it doesn't seem so bad for SwiftySwiftVim to integrate with Python - but a full C++ YCMD could be great.

[puremourning]

I heard ycmd3k would be written in Rust. #justsayin :)

…

On 20 Apr 2017, at 06:38, Jerry Marino ***@***.***> wrote: I was looking at the it and saw that you wrapped it with a Python server which is the same we use for ycmd and JediHTTP wrapping your completer with boost/Python. Awesome, thank you for taking the time to check it out, and thank you for the awesome code! I hope we can get some level of Swift support into YCMD. I understand that making the HTTP layer in Python helped with the implementation of the handlers because you could look what JediHTTP implemented and replicate it for your swift completer. The only thing that came to mind is: did you consider NOT to wrap it with Python but with a C++ HTTP server library? I'm saying this because this way you're coupling your project to 2 tecnologies and I can assure you that making a compiled binary wrapped with Python is a nightmare in terms of compatibility. Just look at the issue tracker of YouCompleteMe for some examples. There were a few tradeoffs I considered when selecting a server framework for SwiftySwiftVim. Minimal development effort and maintainability are 2 key requirements, where effort is king. Due to my time constraints, I needed to get something working within in a few days. Initially, I was leaning towards using a lightweight C++ server framework. I am heavily biased towards C++ here for the same reasons you mention and more. After looking at sever options I and came to the following conclusions: YCMD is heavily reliant on python including the Completer, the test suite, and Completer subclasses. Given the current state, it seems like I need to manage the complexity multiple technologies even if the Swift backend server is totally in C++. JediHTTP already implements a known working server and client in YCMD. On the surface, the code seems reuseable for Swift or the Nth language. As proof, I was up and running within a few hours using the client/server stack 👍 YCMD uses python and boost python already: I might be able to reuse some the YCMD tooling to handle the Python and Boost dependencies. If there had been a JediHTTP equivalent in C or C++, I would have used it hands down. I didn't find an appealing Bottle/JediHTTP alternative and I didn't want to spend time implementing a client and server. Plus right not might not be a "problem" because ycmd already uses Python and we would just use the same version as ycmd but in the future we might move away from Python. Today, it doesn't seem so bad for SwiftySwiftVim to integrate with Python - but a full C++ YCMD could be great. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[Valloric]

@jerrymarino Thanks for doing this work! Your life might be a bit easier if you went with Rust instead; take a look at racerd which is to Rust what JediHTTP is to Python (a ycmd semantic engine server).

Like @puremourning mentioned, Python is not the technology will be using for the long term. There will be a ycmd2 at some point, and it will be in Rust. We have unending issues with Python deployment.

[jerrymarino]

That is super exciting and I'm looking forward to Rust YCMD - Rust looks awesome :)

I haven't had an opportunity to try it yet, so it'd be a learning opportunity. The downside is this is lot effort for me to ramp up and probably out of scope for me to add Swift support to YCMD.

For now, going full C++ on the Swift backend could be an option. I was looking at cross platform C++ HTTP server libs again and it seems like Boost.ASIO could be a good fit long term. A compiled server binary depending on Boost would be easy to distribute. Additionally, from a development and tooling standpoint, it is widely used and easy to setup.

Another thing that comes to mind is the Swift compiler APIs are in C++ ( Lib IDE ) and C ( sourcekitd ), so having 1 language for the entire project is an attractive architecture.

Does that seem resonable? Is this something we can add a client for in YCMD?

[Valloric]

@jerrymarino Sure, a C++ semantic completion server for Swift sounds reasonable.

[jerrymarino]

@Valloric ok great. I'll write up plans for this into a design doc and get started soon 👍

[jerrymarino]

I ended up having a dash of time to work on this. I rewrote the server in C++ and I'm a lot happier with the new implementation, so thank you all for convincing me to move away from python 👍 .

The commit I pushed has a summary of the integration. Basically, it should work end to end for completions - just run build.py and go.

I initially setup completions with YCMD ( see ycmd/completers/swift ), which should be a good starting point for people using swift. I've got diagnostics implemented in the server and can integrate this and more features as follow ups if we think it is a good fit.

You can find notes about the new design over at SwiftySwiftVim - if anyone is interested or has feedback for me, I'd love to hear! ( https://github.com/jerrymarino/swiftyswiftvim )

[Valloric]

Looking nice! I love the GIF in your readme. :)

I've looked over the code and I can see that it's all very much in an RFC stage (for instance, no tests) so it was a light review; in general, I love where this is going. Definitely mergeable with the issues/details worked out.

Thanks for working on this! Really well done!

---

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

ycmd/completers/swift/swift_completer.py, line 47 at r1 (raw file):

                'third_party', 'swiftyswiftvim', 'build', 'http_server' ) )

class SwiftCompleter( Completer ):

You also need tests for all of this.

---

ycmd/completers/swift/swift_completer.py, line 62 at r1 (raw file):

    self._keep_logfiles = user_options[ 'server_keep_logfiles' ]
    self._hmac_secret = ''
    self._message = 'SSVIM'

Not sure what this is for? It doesn't appear to be used anywhere and SSVIM appears to be a magic value of some sort.

---

ycmd/completers/swift/swift_completer.py, line 79 at r1 (raw file):

      self._logger.debug( 'JediHTTP not running.' )
      return False
      try:

This seems incorrect given that return False is right above it and thus this will never be executed.

---

ycmd/completers/swift/swift_completer.py, line 133 at r1 (raw file):

    self._http_port = utils.GetUnusedLocalhostPort()
    self._http_host = ToBytes( 'http://0.0.0.0:{0}'.format(
    self._http_port ) )

Indentation looks wrong.

---

ycmd/completers/swift/swift_completer.py, line 188 at r1 (raw file):

    extra_headers = []

    self._logger.error( 'Making SSVIM request: %s %s %s %s', 'POST', url,

Why is this being logged to stderr?

---

ycmd/completers/swift/swift_completer.py, line 199 at r1 (raw file):

    response.raise_for_status()
    value = response.json()
    self._logger.error( 'Got SSVIM response: %s %s %s %s', 'POST', url,

Same here, why error? Maybe you meant debug log?

---

ycmd/completers/swift/swift_completer.py, line 231 at r1 (raw file):

    flags.append( '-sdk' )
    flags.append(
        '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk' )

We'll need a different approach than this before merging. :)

---

ycmd/completers/swift/swift_completer.py, line 244 at r1 (raw file):

  def _GetExtraData( self, completion ):
      location = {}

2 space indent, not 4

---

ycmd/completers/swift/swift_completer.py, line 345 at r1 (raw file):

    'key.modulename': 'ObjectiveC.NSObject'
  }
  '''

Not sure why you have json in the docstring here...

---

Comments from Reviewable

[jerrymarino]

Thanks for the kind words and glad you enjoyed the GIF! I wanted to get feedback around the overall integration and design before investing more time - so there some rough edges in the current revision, which I can smooth out. I still need to come up with a way to get tests and user options/flags working, so if you have any suggestions around either of these, it'd be incredibly useful. Great to hear that this is on the right track :).

---

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

ycmd/completers/swift/swift_completer.py, line 47 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

You also need tests for all of this.

I looked through some of the tests, and noticed most are integration style, where the completes are depending on, and making requests to semantic engines end to end. If I end up going this route, is it possible to only allow my tests to run on a supported env on the CI? i.e. the backend requires Xcode. I looked through some of the CI scripts, but I'm not very familiar with travis or appveyor, so if you have any suggestions for how I can achieve this it'd be super useful. I definitely want to write some integration tests for this.

---

ycmd/completers/swift/swift_completer.py, line 62 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Not sure what this is for? It doesn't appear to be used anywhere and SSVIM appears to be a magic value of some sort.

It's totally useless, I'll rip it out on the next revision.

---

ycmd/completers/swift/swift_completer.py, line 79 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

This seems incorrect given that return False is right above it and thus this will never be executed.

Thanks for the comment, Yeah this is definitely not good! I think this got committed after some late night coding and left in here.

---

ycmd/completers/swift/swift_completer.py, line 133 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Indentation looks wrong.

Done.

---

ycmd/completers/swift/swift_completer.py, line 188 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Why is this being logged to stderr?

Leftover prototyping cruft - I don't think it should be.

---

ycmd/completers/swift/swift_completer.py, line 199 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Same here, why error? Maybe you meant debug log?

Totally agree, I don't think this should be logged as an error - I'll move it over to stdout.

---

ycmd/completers/swift/swift_completer.py, line 231 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

We'll need a different approach than this before merging. :)

I filed this issue to get it working: jerrymarino/swiftyswiftvim#2. It seems like adopting the ycmd_extra_conf system could be an option to fit it in to the front end for swift. Another thing I thought of was reading in a compilation database directly in the SwiftCompleter, which may simplify things. Given the talk in the PR around moving off of python, maybe its better to not have swift users adopting the python format. Another potential complication with ycmd_extra_conf support in swift is dealing with existing clang based .ycmd_extra_confsin hybrid swift/c codebases. Do you have any suggestions around this?

---

ycmd/completers/swift/swift_completer.py, line 244 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

2 space indent, not 4

Done.

---

ycmd/completers/swift/swift_completer.py, line 345 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Not sure why you have json in the docstring here...

Definitely shouldn't be here!

---

Comments from Reviewable

[Valloric]

@jerrymarino For tests, I recommend looking at how tests are set up for other semantic engines that are servers themselves, like JediHTTP and racerd.

---

Review status: 0 of 8 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

ycmd/completers/swift/swift_completer.py, line 47 at r1 (raw file):

the backend requires Xcode.

Is Xcode a strict requirement? I thought it was only SourceKit which from my cursory Googling is possible to get running on Linux. If so, we'd really love to get it working on Linux too. Maybe not as a v1 of this integration, but in the future.

WRT platform-specific tests, we do have those. Search for @MacOnly in the codebase. Tests with that annotation will only run on a Mac platform.

---

ycmd/completers/swift/swift_completer.py, line 231 at r1 (raw file):

Previously, jerrymarino (Jerry Marino) wrote…

I filed this issue to get it working: jerrymarino/swiftyswiftvim#2. It seems like adopting the ycmd_extra_conf system could be an option to fit it in to the front end for swift. Another thing I thought of was reading in a compilation database directly in the SwiftCompleter, which may simplify things. Given the talk in the PR around moving off of python, maybe its better to not have swift users adopting the python format. Another potential complication with ycmd_extra_conf support in swift is dealing with existing clang based .ycmd_extra_confsin hybrid swift/c codebases. Do you have any suggestions around this?

Avoid our extra conf system; you don't want to depend on Python. Loading clang's compilation database (if doable) is likely the best approach. It's also what we recommend these days instead of using a ycm_extra_conf.py file (we load comp db's by default).

---

ycmd/completers/swift/swift_completer.py, line 244 at r1 (raw file):

Previously, jerrymarino (Jerry Marino) wrote…

Done.

Either I'm crazy, or this function's body is still indented by 4 spaces. :)

Note that you can make use of our flake8 setup that will report these style issues automatically. Use our run_tests.py script (it will run flake8 for you).

---

Comments from Reviewable

[jerrymarino]

Thanks for the comments, it's super helpful! I'll go off of the patterns JediHTTP and racerd are using for testing then.

---

Review status: 0 of 8 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

ycmd/completers/swift/swift_completer.py, line 47 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

the backend requires Xcode.

Is Xcode a strict requirement? I thought it was only SourceKit which from my cursory Googling is possible to get running on Linux. If so, we'd really love to get it working on Linux too. Maybe not as a v1 of this integration, but in the future.

WRT platform-specific tests, we do have those. Search for @MacOnly in the codebase. Tests with that annotation will only run on a Mac platform.

For now, Xcode is a requirement since the build links against the user's default SourceKit dylib in Xcode. This is optimized for speed and simplicity, and I think a lot of swift users are required to have Xcode installed ( like iOS developers using Apple SDKs ).

Building from source is big overhead. A source build of takes hours and requires downloading and compiling the Swift compiler and it's deps like llvm, clang and more. The disk space that code and build output consumes isn't small.

I think it'd be incredibly valuable to get it running for the folks on linux too! It looks like the swift team has done some work to get it to build on Ubuntu. It could be a matter of tweaking the build system ( CMake ) to build sourcekit for linux, or even better, use a prebuilt bin. Perhaps we could find a contributor running linux to help out with this as a follow up to v1?

Cool, I'll take a look for the @MacOnly tests.

---

ycmd/completers/swift/swift_completer.py, line 231 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Avoid our extra conf system; you don't want to depend on Python. Loading clang's compilation database (if doable) is likely the best approach. It's also what we recommend these days instead of using a ycm_extra_conf.py file (we load comp db's by default).

Thanks for the comment, I didn't realize YCM was autoloading clang compilation dbs automatically now, that is super awesome!

I'm not sure if they're using the compilation dbs swift or anything similar yet. If there is a standardized way to spec compilations in swift I'll use that, otherwise, I'll look into adopting the clang format here, and will stay away from python.

---

ycmd/completers/swift/swift_completer.py, line 244 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Either I'm crazy, or this function's body is still indented by 4 spaces. :)

Note that you can make use of our flake8 setup that will report these style issues automatically. Use our run_tests.py script (it will run flake8 for you).

Yep, it is still at 4 spaces :) Thanks for the tip. I've got all the style issues sorted out locally. I'll push it this up along with tests when I have some time to write them.

---

Comments from Reviewable

[Valloric]

Review status: 0 of 8 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

ycmd/completers/swift/swift_completer.py, line 47 at r1 (raw file):

Previously, jerrymarino (Jerry Marino) wrote…

For now, Xcode is a requirement since the build links against the user's default SourceKit dylib in Xcode. This is optimized for speed and simplicity, and I think a lot of swift users are required to have Xcode installed ( like iOS developers using Apple SDKs ).

Building from source is big overhead. A source build of takes hours and requires downloading and compiling the Swift compiler and it's deps like llvm, clang and more. The disk space that code and build output consumes isn't small.

I think it'd be incredibly valuable to get it running for the folks on linux too! It looks like the swift team has done some work to get it to build on Ubuntu. It could be a matter of tweaking the build system ( CMake ) to build sourcekit for linux, or even better, use a prebuilt bin. Perhaps we could find a contributor running linux to help out with this as a follow up to v1?

Cool, I'll take a look for the @MacOnly tests.

That's ok. It wouldn't be the first semantic engine that requires something third-party installed (TypeScript completer requires tsserver). We should probably also make the path to the compiled library configurable (with a sane default) so that people can plug in a custom-compiled lib.

You'll need to make sure to document all this well though in YCM's readme (that will need a separate PR after this one is merged, as usual).

---

Comments from Reviewable

[jerrymarino]

@Valloric Thanks for the comments, Sir. I added some tests and refactored SwiftySwiftVim's build system to remove OSX and Xcode hardcoded defaults.

The plan for now is:

• move forward with a flags API based on the compilation database spec this weekend. It looks like swift hasn't use this yet ( since there is no LibTooling or similar ) but the format will be a nice API for this.
• run swift tests only on the OSX CI.

I wrote a few more notes in my last commit msg and will try to wrap this up soon.

[codecov-io]

Codecov Report

Merging #487 into master will increase coverage by 0.54%.
The diff coverage is 95.78%.

@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
+ Coverage   93.89%   94.43%   +0.54%     
==========================================
  Files          79       81       +2     
  Lines        5276     5359      +83     
  Branches      158      145      -13     
==========================================
+ Hits         4954     5061     +107     
+ Misses        279      256      -23     
+ Partials       43       42       -1

[jerrymarino]

Good evening! I've had some time to test and tinker on this. I think I've addressed most comments in the last few commits and made even more improvements. I think it's mostly ready for a v0.1 🎉 ( minus a exception of a compiler warning in the backend build - I can put up a fix for that this evening )

@puremourning @Valloric @bstaletic @vheon would you kindly give it another look and or test when you have a chance?

[jerrymarino]

I noticed some intermittent failures on the CI and managed to fix it.

Summary:

• the completer wasn't waiting for the server to finish it's startup process, now it does. 🚀 💥
• now that it correctly waits for it to startup, the shutdown_test needed an increase in timeout.

@puremourning I was able to repro completion timeouts; 382b92f seems to have fixed it for me locally and on the CI.

[vheon]

@jerrymarino I saw just now that you marked this PR as READY, I'm not sure if Github sends notification when you just change the title, so next time write something in it to be sure 👍

Thanks a lot for this PR I'm not sure how much will take to review it, is massive! 😝

[bstaletic]

I gave this another look. Everything seems good, but I'll stay away from marking this LGTM, as I don't know anything about swift and didn't run this code myself.

Still, I left a minor comment.

Even though I don't use swift this is a great PR. Thanks for your time and effort @jerrymarino.

---

Reviewed 1 of 2 files at r6, 7 of 9 files at r7, 3 of 3 files at r8, 3 of 5 files at r9, 3 of 3 files at r10.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

ycmd/completers/swift/swift_completer.py, line 238 at r10 (raw file):

      self._logger.debug( 'Got SSVIM response: %s %s %s %s',
                          'POST', url, value, value.keys() )
      return value

Since value is an empty dictionary here, shouldn't it be better to use a different debug line and have it obvious that something bad happened? That would also allow for return {} and dropping the value in the except: block.

---

Comments from Reviewable

[jerrymarino]

@vheon, cool please let me know if you've got any questions!

If you're planning to manually test, the examples in ycmd/tests/swift/testdata/ could be useful; the iOS tests write out a hardcoded compile_commands.json.

Also, I posted up a repo with test iOS app/OSX app Xcode projects, which are setup to generate compile_commands.json as part of their builds. This should make easy to get up and running on Swift|Clang YCM 🚀

@bstaletic thanks for the kind words and the review!

I'll post an update to the JSON parsing error message.

---

Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

ycmd/completers/swift/swift_completer.py, line 238 at r10 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Since value is an empty dictionary here, shouldn't it be better to use a different debug line and have it obvious that something bad happened? That would also allow for return {} and dropping the value in the except: block.

Cool, I'll make it more explicit and call out the fact that JSON parsing failed

---

Comments from Reviewable

[jerrymarino]

One of the last CI builds failed when brew tried to install Mono - I think it's unrelated but I can't re trigger it:

==> Downloading https://download.mono-project.com/sources/mono/mono-5.0.0.100.ta
==> ./configure --prefix=/usr/local/Cellar/mono/5.0.0.100 --disable-silent-rules
==> make
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

[Valloric]

@micbou Could you take a look at this from a code perspective? You probably have the clearest idea about how all the pieces tie together in ycmd at this point.

@puremourning Thanks for giving this a spin! Since you're the only one who has given this some hands-on testing, I'm declaring your LGTM a necessity for this PR. :)

---

Review status: 22 of 23 files reviewed at latest revision, 19 unresolved discussions.

---

ycmd/completers/swift/swift_completer.py, line 254 at r6 (raw file):

Previously, jerrymarino (Jerry Marino) wrote…

ShouldUseNowInner seems to be wrapped behind the cache: it should be hitting the cache and cache invalidation logic is still governed by the super class. It's a bit tricky to follow with the inheritance. In the case that there is a cache miss on the client, there is caching the entire way down on the backend so it should be fast and intelligently cache as well.

We tend to avoid triggering semantic completion on space, and only do it on semantic triggers (stuff like . etc). The semantic engine (no matter which language) is never as fast as the identifier completer and when typing identifiers, the user already knows which name they are typing and thus only needs help to reduce keystrokes. The name itself is then almost always in the same file.

For method completion like on . etc, the method being called may not be fully clear in the user's mind (they might have a vague recollection of the name) so the "API exploration" use case comes into play here.

This pattern is what we've used in YCM for the past 5 years and it has served us pretty well. The user can always force semantic completion with Ctrl+Space.

Given that, it might be best to go with the standard YCM model; it's what YCM users expect and what the docs talk about. It would be confusing if completion for one language behaved differently.

---

Comments from Reviewable

[jerrymarino]

Thanks again for the comment and review @Valloric!

I'll defer your opinions on the [a-zA-Z] trigger in the latest revision, and remove it if it's best. Please let me know if there is anything else I can improve 🚀 👍

---

Review status: 22 of 23 files reviewed at latest revision, 19 unresolved discussions.

---

ycmd/completers/swift/swift_completer.py, line 254 at r6 (raw file):

We tend to avoid triggering semantic completion on space, and only do it on semantic triggers (stuff like . etc).

After I spent some time testing this out, I can totally see why!

In the latest revisions, I replaced the ' ' trigger with an a-z character set trigger:

  'swift' : [ '.', 're![_a-zA-Z]' ],

My rational here is that this is was aligned with the ObjC bracket calls:

  'objc' : [
    '->',
    '.',
    r're!\[[_a-zA-Z]+\w*\s',    # bracketed calls
    r're!^\s*[^\W\d]\w*\s',     # bracketless calls

For example, the following method will complete in ObjC

   [self viewWillTransitionToSize: someSize, with:]

In Swift, the code to call this method would be written

     viewWillTransition(to: someSize, with:)

In the patches current state, it will use semantic completion in this case ( like Xcode does )

A key difference here between the grammars is that in Swift is that, there's no [] or self to indicate the start of a callsite on self.

A side effect is that this regex will trigger semantic completion for other identifiers as well. In my testing, the perf for this level of completion seemed OK.

That being said, I'll defer to you all of weather or not [a-zA-Z] should be a trigger - It should be easy to get rid of if it's not a good fit: to remove it now is a matter of deleting that trigger in completer_utils.py.

Given that, it might be best to go with the standard YCM model; it's what YCM users expect and what the docs talk about. It would be confusing if completion for one language behaved differently.

Totally agree here!

PS: outside of UIKit and AppKit subclass land, people seem to be getting away from deep OOP inheritance. Due to the massive API surface of Apple's framework base classes, that OOP style does benefit from "exploration" style completions for selfcalls.

---

Comments from Reviewable

[jerrymarino]

@puremourning @micbou friendly ping 🙏

I think there's one more improvement I'd like to make: moving all of the flag logic to the server. I started this last night:
jerrymarino/swiftyswiftvim@eda08c0 and I can update the PR for it ( which moves a ton of complexity out of YCMD ). Please let me know your thoughts when you've got a min. I'd like to wrap this up 🍾

[puremourning]

Sorry for the delay. This is a big PR and requires quite a lot of time to review and test out, so it's taking me longer than I would like.

I'm testing this again now, and looking at the code.

[puremourning]

Reviewed 2 of 8 files at r2, 6 of 13 files at r4, 1 of 3 files at r5, 1 of 2 files at r6, 5 of 9 files at r7, 1 of 3 files at r8, 2 of 5 files at r9, 2 of 3 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 45 unresolved discussions.

---

a discussion (no related file):

Previously, puremourning (Ben Jackson) wrote…

So i've been trying most of the evening to get solid completions working, but I guess I'm either not swift enough, or there's some problem.

I have a basic macOS project created by Xcode:

Bens-MBP:SwiftTest ben$ find SwiftTest
SwiftTest
SwiftTest/AppDelegate.swift
SwiftTest/Assets.xcassets
SwiftTest/Assets.xcassets/AppIcon.appiconset
SwiftTest/Assets.xcassets/AppIcon.appiconset/Contents.json
SwiftTest/Base.lproj
SwiftTest/Base.lproj/Main.storyboard
SwiftTest/Info.plist
SwiftTest/ViewController.swift

I added a button to the Main storyboard and ctrl-dragged it to create an IBOutlet in the ViewControler. I then tried to use the outlet. That leaves ViewController.swift like this:

import Cocoa

class ViewController: NSViewController {

    override func viewDidLoad() {
        super.viewDidLoad()

        button_.
    }

    @IBOutlet weak var button_: NSButton!
}

I built this with xcodebuild and copy/pasted the compile command into compile_commands.json. This lead to every completion request timing out, so I pruned a lot of the flags down to just what looked like includes:

[
  {
    "directory": "/Users/ben/Development/Swift/Test/SwiftTest/SwiftTest",
    "command": "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swiftc -module-name SwiftTest -O -sdk /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -target x86_64-apple-macosx10.12 -I /Users/ben/Development/Swift/Test/SwiftTest/build/Release -F /Users/ben/Development/Swift/Test/SwiftTest/build/Release /Users/ben/Development/Swift/Test/SwiftTest/SwiftTest/ViewController.swift -Xcc -I/Users/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/swift-overrides.hmap -Xcc -iquote -Xcc /Users/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/SwiftTest-generated-files.hmap -Xcc -I/Users/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/SwiftTest-own-target-headers.hmap -Xcc -I/Users/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/SwiftTest-all-target-headers.hmap -Xcc -iquote -Xcc /Users/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/SwiftTest-project-headers.hmap -Xcc -I/Users/ben/Development/Swift/Test/SwiftTest/build/Release/include -Xcc -I/Users/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/DerivedSources/x86_64 -Xcc -I/Users/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/DerivedSources",
    "file": "/Users/ben/Development/Swift/Test/SwiftTest/SwiftTest/ViewController.swift"
  }
]

So the problem. When typing button., in Xcode, I get suggestions, but in Vim/ycmd it doesn't even recognise that button_ is a real thing (no suggestions at all, and self.button_ isn't suggested either).

I added some debugging and it certainly read the compile commands:

--   Flags: ['-module-name', 'SwiftTest', '-O', '-sdk', '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk', '-target', 'x86_64-apple-macosx10.12', '-I', '/Users/ben/Development/Swift/Test/SwiftTest/build/Release', '-F', '
/Users/ben/Development/Swift/Test/SwiftTest/build/Release', '/Users/ben/Development/Swift/Test/SwiftTest/SwiftTest/ViewController.swift']

So I think I ran out of talent at this point. Can't tell if it is a bug, missing feature, or pebkac :)

With the latest code, and the latest Xcode on a totally different mac, i still can't get SDK completions to work properly without constant timeouts.

This on both a MBP (admittedly quite old, but by no means sluggish), and a reasonably new iMac:

After running the tests, i did cd $PATH_TO_SWIFT_TESTS/testdata/iOS/Basic and opened Basic/ViewController.swift.

I am able to get completions in the viewDidLoad method, but not after it, in the didReceiveMemoryWarning method.

I'm worried that this is endemic - there's nothing special about my macs, and that it will be a source of many issues on the tracker. I would at least like to understand why before we declare this "officially supported". I can't see swift completion being super useful without the SDK.

---

a discussion (no related file):
The behaviour of method completion is still IMO not in keeping with YCM's general model. You type something, then complete a method, then you have to leave insert mode to type the parameters. That's just super awkward, and i don't think it is mergeable in that state, sorry.

I think we have to only complete up to the next point the user would start typing. So that the user's typing flow is not interrupted, like it wouldn't be with the identifier completer.

---

build.py, line 439 at r11 (raw file):

def BuildSwiftySwiftVim():
  os.chdir( p.join( DIR_OF_THIS_SCRIPT, 'third_party', 'swiftyswiftvim' ) )
  CheckCall( [ 'bash', 'bootstrap' ] )

I think we should:

• only try to do this on mac, and report an error "Swift support is only available on macOS with Xcode v8.3 or later" if --swift-completer attempted on other OSes
• Validate the Xcode version. We know that it doesn't work well on Xcode 8.0 as that's what I tried before.

I'm not super worried if we do that in ycmd's build.py or in ssvim. Probably better in ssvim actually, though that makes --all fiddly here.

---

ycmd/completers/swift/hook.py, line 25 at r11 (raw file):

from ycmd.completers.swift.swift_completer import SwiftCompleter
from ycmd.completers.swift.swift_completer import ShouldEnableSwiftCompleter

These could be one line

from ycmd.completers.swift_completer import SwiftCompleter, ShouldEnableSwiftCompleter

If that wraps 80 chars:

from ycmd.completers.swift_completer import ( SwiftCompleter, 
                                              ShouldEnableSwiftCompleter )

Or something. Not a big deal.

---

ycmd/completers/swift/swift_completer.py, line 108 at r6 (raw file):

Previously, jerrymarino (Jerry Marino) wrote…

Oh intersting, so I only will need a lock on the starting and stopping then?

anything that changes the internal state of this object needs to be thread safe (e.g. messing around with log files, starting stopping the server, etc.)

---

ycmd/completers/swift/swift_completer.py, line 254 at r6 (raw file):

people seem to be getting away from deep OOP inheritance

OMG. Woo \o/. Maybe the world isn't doomed after all!

---

ycmd/completers/swift/swift_completer.py, line 347 at r6 (raw file):

Previously, jerrymarino (Jerry Marino) wrote…

The idea here was to move all of the raw JSON extraction into a type so it's not mixing the protocol handling work with completion logic. Also, I think its nicer to work with a structured type than a dictionary and will be easier to refactor the API. That's my crufty opinion from doing enterprise stuff for too long or bad python skills :) I'll try stripping it down to something more elemental. I think it'd be nicer if there wasn't so many small methods too.

Meh it's fine. I was just reeling against too much OO in the world

---

ycmd/completers/swift/swift_completer.py, line 88 at r11 (raw file):

    Check if the server is alive AND ready to serve requests.
    '''
    self._logger.info( 'Got SSVIM Healthy Request' )

Not sure we need this logging at info level. Seems like debug to me.

---

ycmd/completers/swift/swift_completer.py, line 92 at r11 (raw file):

      self._logger.info( 'SSVIM not running.' )
      try:
        return bool( self._GetResponse( '/status', timeout = 0.2 ) )

under what circumstances would this ever succeed? I mean this is within a branch if not self._ServerIsRunning(). I think that means we know the server isn't running, but we're sending it a status request anyway?

Is that logic backwards?

It seems that we return True if:

• the server is running, or
• the server is not running, but magically responds to a /status request.

I feel it should be, we return true if:

• the sever is running, AND
• the server responded to a /ready request.

Or am I going senile ?

---

ycmd/completers/swift/swift_completer.py, line 94 at r11 (raw file):

        return bool( self._GetResponse( '/status', timeout = 0.2 ) )
      except requests.exceptions.ConnectionError as e:
        self._logger.error( 'Failed Ready' )

is it really an error? The server isn't running, but that's not itself an error. There's an error that the server failed to start or crashed, I suppose, but I would be surprised if the exception (printed below) would be useful other than to say "unable to connect" which is fairly banal. Shrug, again not a big deal, but a lot of our users discover the log files and get scared about exceptions that are printed there habitually. I feel like we should try and minimise where we log non-errors as errors :)

---

ycmd/completers/swift/swift_completer.py, line 108 at r11 (raw file):

      status = ( bool( self._http_port ) and
                 ProcessIsRunning( self._http_phandle ) )
      self._logger.debug( 'Healthy Status ' + str( status ) )

Strange to log "Healthy" status in "Running" check, particularly given the comment.

---

ycmd/completers/swift/swift_completer.py, line 114 at r11 (raw file):

self._StopServer()
self._StartServer()

Best to protect both calls, to prevent these calls getting interleaved with another execution of this method. The lock is a rlock so it is safe to acquire it multiple times in one context. Which is kinda crazy.

with self._server_lock:
  self._StopServer()
  self._StartServer()

---

ycmd/completers/swift/swift_completer.py, line 157 at r11 (raw file):

        json.dump( { 'hmac_secret': ToUnicode(
                      b64encode( self._hmac_secret ) ) },
                       hmac_file )

align hmac_file with { as it is an argument to dump

---

ycmd/completers/swift/swift_completer.py, line 216 at r11 (raw file):

    url = urljoin( self._http_host, handler )
    parameters = self._PrepareRequestBody( request_data )
    body = ToBytes( json.dumps( parameters ) ) if parameters else bytes()

my recollection of python 2/3 support is that the portable way to get an empty bytes string is bytes( b'' ) but i can't entirely remember.

Might be safer to put this like: ToBytes( json.dumps( parameters ) if parameters else '' ) ?

---

ycmd/completers/swift/swift_completer.py, line 237 at r11 (raw file):

      self._logger.debug( 'Failed to parse SSVIM response: %s %s',
                          'POST', url )
      return {}

I think we should do a self._logger.exception here - not being able to decode a response is a real problem and will help debug any issues.

I also think we should let the exception bubble, rather than eating it.

---

ycmd/completers/swift/swift_completer.py, line 258 at r11 (raw file):

    source = request_data[ 'file_data' ][ path ][ 'contents' ]
    line = request_data[ 'line_num' ]
    col = request_data[ 'start_codepoint' ]

just checking: ssvim definitely expects code points, not bytes? and it definitely uses 1-based indices?

Did you add a test that uses unicode characters? We often have problems with these and, which is why i'm bringing it up :)

---

ycmd/completers/swift/swift_completer.py, line 273 at r11 (raw file):

  def ComputeCandidatesInner( self, request_data ):
    logging.info( 'Request SSVIM Completions' )

we really shouldn't be writing to the log file for every completion request. I/O is really really slow, and this message is not actually very useful. Maybe debug? but TBH the logging of the request being sent is probably enough clue (at a guess).

---

ycmd/completers/swift/swift_completer.py, line 302 at r11 (raw file):

      logfiles = [ self._logfile_stdout, self._logfile_stderr ] )

    return responses.BuildDebugInfoResponse(

Include the flags that are used for the current file. This is frequently very valuable for debugging issues.

---

ycmd/completers/swift/swift_completer.py, line 319 at r11 (raw file):

    self.name = json_value.get( 'key.name' )
    self.description = json_value.get( 'key.description' )
    self.type = 'swift'

what's the purpose of this self.type ?

---

ycmd/completers/swift/swift_completer.py, line 350 at r11 (raw file):

    for json_completion in self.json[ 'key.results' ]:
      completion = SwiftCompletion( json_completion, self.request_data )
      completions.append( completion )

Unless I'm missing something, this whole method could be written more pythonically:

  return [ SwitfCompletion( c, self.request_data ) for c in self.json[ 'key.results' ] ) ]

Because, well, Python.

---

ycmd/completers/swift/swift_flags.py, line 35 at r11 (raw file):

    with open( db_file_name ) as json_db:
      self._raw_value = json.load( json_db )
      logging.debug("Loaded JSON Raw Value: " + str(self._raw_value))

spaces inside parentheses

---

ycmd/completers/swift/swift_flags.py, line 39 at r11 (raw file):

  def _RawCommandForFile( self, compilable_file ):
    logging.debug("Raw Command For File: " + compilable_file)

spaces inside parentheses

---

ycmd/completers/swift/swift_flags.py, line 99 at r11 (raw file):

        self._dbs_by_folder[ folder ] = db
        return db
    logging.debug("No DB For File: " + path)

spaces inside parentheses

---

ycmd/completers/swift/swift_flags.py, line 101 at r11 (raw file):

    logging.debug("No DB For File: " + path)
    return None

2 blank lines

---

ycmd/completers/swift/swift_flags.py, line 108 at r11 (raw file):

    with self._db_lock:
      return db._RawCommandForFile( compilable_file )

2 blank lines

---

ycmd/completers/swift/swift_flags.py, line 117 at r11 (raw file):

    # in another file, the previous flags are invalided. Unlike clang
    # directory includes, we need to put all dependent *files* as part of the
    # 'primary-file'.

I didn't really understand this TODO and particularly I don't have enough Swift fu to understand the impact. Should we be worried ?

---

ycmd/completers/swift/swift_flags.py, line 122 at r11 (raw file):

      cached_flags = self._flags_for_file.get( compilable_file )
      if cached_flags:
        logging.debug("Cached Flags: " + str(cached_flags))

spaces inside parentheses

---

ycmd/completers/swift/swift_flags.py, line 125 at r11 (raw file):

        return cached_flags
    command = self._RawCommandForFile( compilable_file )
    logging.debug("Raw Command: " + str(command))

spaces inside parentheses

---

ycmd/completers/swift/swift_flags.py, line 131 at r11 (raw file):

    split = shlex.split( command )
    filtered = FlagsForCommandList( split )
    final_flags = filtered

not sure what final_flags is adding here?

---

ycmd/completers/swift/swift_flags.py, line 179 at r11 (raw file):

  # Skip the first flag.
  # This is the compiler being used.
  i = 1

I personally prefer even loop variables to have real names, such as flag_index

---

ycmd/completers/swift/swift_flags.py, line 180 at r11 (raw file):

  # This is the compiler being used.
  i = 1
  flags_len = len( split_cmd )

num_flags ? meh.

---

ycmd/tests/server_utils_test.py, line 51 at r11 (raw file):

  os.path.join( DIR_OF_THIRD_PARTY, 'tern_runtime' ),
  os.path.join( DIR_OF_THIRD_PARTY, 'waitress' ),
  os.path.join( DIR_OF_THIRD_PARTY, 'swiftyswiftvim' )

These were previously sorted. I'm going to assume there's a reason for that, and that we should maintain the sort.

---

ycmd/tests/test_utils.py, line 212 at r11 (raw file):

def WaitUntilCompleterServerReady( app, filetype, timeout = 40 ):

Bit sad that we had to increase this :(

---

ycmd/tests/swift/init.py, line 76 at r11 (raw file):

  @functools.wraps( test )
  def Wrapper( *args, **kwargs ):
    WriteBasicExampleCompilationDatabase()

I think you want this to be a context manager and for it to remove the generated compile_commands.json after it has run.

leaving it behind leaves the tree in a different and arbitrary state if the test fails, or in any case differently after running the test.

In order to have solid, repeatable tests, each individual test should start and run independently and clean up after itself.

I recall that I wrote something somewhere called TemporaryClangProject() or something which did exactly that when testing our c++ compile_commands.json support.

---

ycmd/tests/swift/get_completions_test.py, line 71 at r11 (raw file):

@OSXOnlySharedYcmd
def GetCompletions_SDK_RecursiveImport_test( app ):

Make sure to add a test which includes unicode characters in the line before and after the completion request.

There are plenty of examples for the other completers (trust me :)). The point of the tests is ensuring that we send the correct offsets when the byte offset in the line differs from the "character" (codepoint) offset

---

ycmd/tests/swift/swift_flags_test.py, line 22 at r11 (raw file):

from ycmd.tests.swift import PathToTestFile
from ycmd.tests.swift import BasicTest

Put on one line?

---

ycmd/tests/swift/swift_flags_test.py, line 51 at r11 (raw file):

@BasicTest
def PrimaryFile_dependent_flags_caching_test():

I would like to see more module tests for the flags parsing. There is quite a log of code in there and it is quite fiddly (lots of i = i + 1 and all that.

Using the mocking framework it should be possible to write some concise tests that exercise the finer points of the logic independently.

---

ycmd/tests/swift/testdata/.gitignore, line 4 at r11 (raw file):

# the files: there is a template compile_commands.json.template, which is
# written to this file during test runs.
ios/Basic/compile_commands.json

Like i said, I don't like this. I would rather that we did any such generations transitively and/or generated them to a temporary file, like we do for clang completer.

---

ycmd/tests/swift/testdata/iOS/Basic/compile_commands.json.template, line 4 at r11 (raw file):

{
  "directory": "__SRCROOT__",
  "command": "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swift -frontend -c -primary-file __SRCROOT__/Basic/ViewController.swift __SRCROOT__/Basic/AppDelegate.swift -enable-objc-interop -sdk /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk -enable-testing -g -serialize-debugging-options -Xcc -I/Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/swift-overrides.hmap -Xcc -iquote -Xcc /Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/Basic-generated-files.hmap -Xcc -I/Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/Basic-own-target-headers.hmap -Xcc -I/Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/Basic-all-target-headers.hmap -Xcc -iquote -Xcc /Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/Basic-project-headers.hmap -Xcc -I/Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Products/Debug-iphonesimulator/include -Xcc -I/Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/DerivedSources/x86_64 -Xcc -I/Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/DerivedSources -Xcc -DDEBUG=1 -Xcc -working-directory__SRCROOT__ -Onone -module-name Basic -o /Users/fakeuser/Library/Developer/Xcode/DerivedData/Basic-dcvyhoqxsulylretnajwiqnkngaj/Build/Intermediates/Basic.build/Debug-iphonesimulator/Basic.build/Objects-normal/x86_64/ViewController.o",

What's all the fakeuser stuff about?

---

Comments from Reviewable

[puremourning]

OK been through it. I know I've made a lot of (somewhat pedantic) comments, but don't be disheartened! Thanks so much for the effort, really excited to get this merged!

[jerrymarino]

Review status: all files reviewed at latest revision, 45 unresolved discussions.

---

a discussion (no related file):
@puremourning thank for the review - this is super valuable. This video is helpful and connects the dots for me, thanks for sharing it ( cool theme btw )!

I tried the exact same workflow that you ran through in the video, and can repro these issues. Thankfully, it's not your mac having issues! Those timeouts are happening because the test examples causes a crash, which should definitely be addressed. Python's error messages here are certainly confusing.

I'll try to dig into testdata example to find out why it isn't working. None the less, I think the testdata either needs to work for manual testing or be removed altogether.

I can't see swift completion being super useful without the SDK.

I don't understand this comment - all of the methods in the first few seconds of your video are populated from the SDK includes ( the symbols for UIViewController aren't in that file ).

Completions seem to be returned reasonably in your video until a crash was triggered - it's got that going for it.

This may have got lost in the comments, but I did post a sample test project specifically to test this ( I'm doing most of all dev on this project ): https://github.com/jerrymarino/SwiftVimTestHost I don't run into these issues in that example or real projects. I think the delta here could be the compile_commands.json, but I'll dig into it more!

---

a discussion (no related file):

The behaviour of method completion is still IMO not in keeping with YCM's general model.

This is a really good point - under YCMD convention, it would return completions for the first word of the method ( not the actual method ).

For YCMD style, should method signatures be split at the (?

That seems like it be good for methods with short signatures ( where there is 1 unnamed param ) but when you have super long UIKit methods, it is not very desirable:

Under this logic, the method: likeSome(uberCanonical:factoryManager:runnerDelegate:factoryEmpireBuilder:immutableMangerBuilder:withOptions:inContext:successBlock:progressBlock:failureBlock:)

Would complete to:
likeSome

I think that behavior is good for C++, C, python, and other languages without named parameters - but it would need some adjusting to be idiomatic for Swift. It doesn't work with a large subset of code out there - ( i.e. 5+ word long method signatures )

You type something, then complete a method, then you have to leave insert mode to type the parameters.

I totally agree with this - this totally sucks. Having the entire method name completed isn't ideal without the placeholders. I originally wrote it with the assumption that it'd be using the corresponding Vim script that allows it to jump between placeholders ( like Xcode does ).

Making Swift completion conform to the YCMD convention breaks a few key behaviors that Xcode got right:

• trigger semantic completions on typing words
• trigger semantic completions without typing self. ( self. in this context is not idiomatic swift code )
• navigating placeholders in poorly written code with 100 parameter long methods

Here is a video of what swift completion looks like under Xcode:

For now, I'll focus on implementing the ideal behavior for Swift under Vim. Thanks for helping review this! 👍

---

Comments from Reviewable