[RFC] Add extra conf support to Clangd completer

[micbou]

This PR adds support for .ycm_extra_conf.py files to the Clangd completer by sending the flags to Clangd through the didChangeConfiguration notification using the compilationDatabaseChanges setting on the FileReadyToParse event.

The value of the language field used in extra conf files for the Clangd completer is changed to the same value as the libclang completer (cfamily) to allow the use of extra confs that were written for the libclang completer. For instance, ycmd's own .ycm_extra_conf.py file is supported with no additional changes.

---

This change is 

[codecov]

Codecov Report

Merging #1250 into master will increase coverage by 0.03%.
The diff coverage is 98.38%.

@@            Coverage Diff             @@
##           master    #1250      +/-   ##
==========================================
+ Coverage   96.99%   97.02%   +0.03%     
==========================================
  Files          95       95              
  Lines        7497     7542      +45     
  Branches      153      153              
==========================================
+ Hits         7272     7318      +46     
+ Misses        184      183       -1     
  Partials       41       41

[bstaletic]

Reviewed 13 of 14 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @micbou)

---

ycmd/completers/cpp/clangd_completer.py, line 215 at r3 (raw file):

      flags = flags[ index: ]
      break
  return [ 'clangd' ] + flags

Why is this using [ 'clangd' ] as the compiler flag?

---

ycmd/completers/cpp/clangd_completer.py, line 437 at r3 (raw file):

    return super( ClangdCompleter, self ).OnFileReadyToParse(
      request_data,
      handlers = [ lambda self: self._SendFlagsFromExtraConf( request_data ) ] )

Just a thought. Why don't we set handlers in __init__ and avoid overriding another member function?

---

ycmd/completers/cpp/flags.py, line 458 at r3 (raw file):

    # remove that.
    if _SkipStrayFilenameFlag( current_flag,
                              previous_flag,

Misaligned.

---

ycmd/completers/cpp/flags.py, line 468 at r3 (raw file):

def _SkipStrayFilenameFlag( current_flag,
                           previous_flag,

Misaligned.

---

ycmd/completers/language_server/simple_language_server_completer.py, line 87 at r3 (raw file):

  def Language( self ):

Why did this get removed? Shouldn't it just be overridden in the ClangdCompleter?

---

ycmd/completers/python/python_completer.py, line 80 at r3 (raw file):

      if hasattr( module, 'Settings' ):
        settings = module.Settings( language = 'python',
                                    filename = filepath,

Is there any reason why a user would care about filename for a python project?

[kadircet]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @micbou)

---

README.md, line 221 at r3 (raw file):

basis or globally. Currently, it is required by the libclang-based completer and
optional for other completers. The following arguments can be retrieved from
the `kwargs` dictionary and are common to all completers:

Irrelevant to this patch but could you also clarify whether the following arguments would always exist or optional.
As you might've noticed filename was not being passed before in all cases for example.

---

README.md, line 264 at r3 (raw file):

- `include_paths_relative_to_dir`: (optional) the directory to which the include
  paths in the list of flags are relative. Defaults to ycmd working directory
  for the libclang completer and `.ycm_extra_conf.py`'s directory for the

why are the defaults for these two are different?

---

ycmd/completers/cpp/clangd_completer.py, line 205 at r3 (raw file):

def AddCompilerToFlags( flags, enable_windows_style_flags ):

Maybe call it Prepend ?

---

ycmd/completers/cpp/clangd_completer.py, line 215 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why is this using [ 'clangd' ] as the compiler flag?

It doesn't matter from clangd's perspective, but as a general convention I would suggest clang-tool

---

ycmd/completers/cpp/clangd_completer.py, line 218 at r3 (raw file):

def BuildCompilationCommand( flags, filepath ):

why not use ycmd.flags as clang_completer does ?

---

ycmd/completers/cpp/clangd_completer.py, line 250 at r3 (raw file):

  def GetCompleterName( self ):
    return 'C-family'

Maybe keep clangd in here to be able to distinguish libclang and clangd

---

ycmd/completers/cpp/clangd_completer.py, line 404 at r3 (raw file):

  def _SendFlagsFromExtraConf( self, request_data ):

again some parts in here could be avoided by using ycmd.cpp.flags

---

ycmd/completers/cpp/clangd_completer.py, line 411 at r3 (raw file):

    with self._server_info_mutex:
      module = extra_conf_store.ModuleForSourceFile( filepath )

Do we want to send out flags coming from a global extra conf, even if there is a compilation_database.json for the project?

---

ycmd/completers/language_server/language_server_completer.py, line 1090 at r3 (raw file):

  def GetSettings( self, module, request_data ):

Again, this function doesn't need to be exposed if ycmd.cpp.flags is used

---

ycmd/completers/language_server/simple_language_server_completer.py, line 37 at r3 (raw file):

class SimpleLSPCompleter( lsc.LanguageServerCompleter ):
  @abc.abstractmethod
  def GetCompleterName( self ):

How is this different than GetServerName ?

---

ycmd/completers/language_server/simple_language_server_completer.py, line 93 at r3 (raw file):

                                          extras = extras )

    return responses.BuildDebugInfoResponse( name = self.GetCompleterName(),

maybe use GetServerName ?

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @kadircet and @micbou)

---

README.md, line 264 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

why are the defaults for these two are different?

The libclang behaviour is backwards compatible, while clangd behaviour depends on what got reported to the server as the rootUri in initialize request.

---

ycmd/completers/cpp/clangd_completer.py, line 215 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

It doesn't matter from clangd's perspective, but as a general convention I would suggest clang-tool

Then let's use clang-tool.

---

ycmd/completers/cpp/clangd_completer.py, line 250 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

Maybe keep clangd in here to be able to distinguish libclang and clangd

We shouldn't distinguish clangd and libclang here, so that the current extra confs (that don't use override_filename) work seamlessly.

Ideally, when we make clangd the default, users will not have to do anything.

---

ycmd/completers/cpp/clangd_completer.py, line 411 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

Do we want to send out flags coming from a global extra conf, even if there is a compilation_database.json for the project?

We used to do that for the libclangd completer. Then we had reports that users don't want that, so we changed to preferring the local database over the global extra conf.

Naturally, after that we got complaints from users who prefer the original behaviour, but they can still have it if they place the global extra conf in $HOME.

So, for consistency, let's prioritize local database over the global extra conf.

[kadircet]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic, @kadircet, and @micbou)

---

README.md, line 264 at r3 (raw file):

clangd behaviour depends on what got reported to the s

But they both have the same semantics. Every include dir in the compile commands are assumed to be relative to this directory, if they are not absolute.
I totally agree that it makes sense to use ycm_extra_conf's directory, since people are most likely to write their commands relative to that.

I just don't understand how it come to be the working directory of ycmd for libclang. Was there a specific use-case that we should be aware of ?

---

ycmd/completers/cpp/clangd_completer.py, line 250 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

We shouldn't distinguish clangd and libclang here, so that the current extra confs (that don't use override_filename) work seamlessly.

Ideally, when we make clangd the default, users will not have to do anything.

I totally agree, but we already have Language field for that. This one is returning completer name

[micbou]

Reviewed 14 of 14 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 4 of 4 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @kadircet)

---

README.md, line 221 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

Irrelevant to this patch but could you also clarify whether the following arguments would always exist or optional.
As you might've noticed filename was not being passed before in all cases for example.

Yes, that's why filename was not in the list of arguments prior to this PR. I don't see a need to clarify this.

---

README.md, line 264 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

clangd behaviour depends on what got reported to the s

But they both have the same semantics. Every include dir in the compile commands are assumed to be relative to this directory, if they are not absolute.
I totally agree that it makes sense to use ycm_extra_conf's directory, since people are most likely to write their commands relative to that.

I just don't understand how it come to be the working directory of ycmd for libclang. Was there a specific use-case that we should be aware of ?

Because this was the case before adding the include_paths_relative_to_dir option and we didn't want to break backward compatibility. See PR #795.

---

ycmd/completers/cpp/clangd_completer.py, line 205 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

Maybe call it Prepend ?

Done.

---

ycmd/completers/cpp/clangd_completer.py, line 215 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Then let's use clang-tool.

Done.

---

ycmd/completers/cpp/clangd_completer.py, line 218 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

why not use ycmd.flags as clang_completer does ?

We can't use the Flags class because parts about the compilation database depend on libclang which is not available if support is not compiled in.

---

ycmd/completers/cpp/clangd_completer.py, line 250 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

I totally agree, but we already have Language field for that. This one is returning completer name

This is currently only used in the debug info response:

-- C-family completer debug information:
--   clangd running
--   clangd process ID: 2404
--   clangd executable: [...]
--   clangd logfiles:
--     .../clangd_stderry0u97mz1.log
--   clangd Server State: Initialized
--   clangd Project Directory: ...
--   clangd Settings: {}
--   clangd Extra Configuration Flags: ['-Wall', '-Werror']

As you can see, the output can't be confused with libclang.

---

ycmd/completers/cpp/clangd_completer.py, line 404 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

again some parts in here could be avoided by using ycmd.cpp.flags

See answer above.

---

ycmd/completers/cpp/clangd_completer.py, line 411 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

We used to do that for the libclangd completer. Then we had reports that users don't want that, so we changed to preferring the local database over the global extra conf.

Naturally, after that we got complaints from users who prefer the original behaviour, but they can still have it if they place the global extra conf in $HOME.

So, for consistency, let's prioritize local database over the global extra conf.

I think PR #1035 was a mistake and I don't want to repeat it. For users that want the compilation database of a project to be used instead of the global extra conf, there is now the solution to not return the flags field for that project.

---

ycmd/completers/cpp/clangd_completer.py, line 437 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Just a thought. Why don't we set handlers in __init__ and avoid overriding another member function?

Great idea. It's now possible to register handlers on the FileReadyToParse event.

---

ycmd/completers/cpp/flags.py, line 458 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Misaligned.

Done.

---

ycmd/completers/cpp/flags.py, line 468 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Misaligned.

Done.

---

ycmd/completers/language_server/language_server_completer.py, line 1090 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

Again, this function doesn't need to be exposed if ycmd.cpp.flags is used

See answer above.

---

ycmd/completers/language_server/simple_language_server_completer.py, line 37 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

How is this different than GetServerName ?

A completer may have a different name than the server e.g. in Java, the completer is named Java but the server is jdt.ls.

---

ycmd/completers/language_server/simple_language_server_completer.py, line 87 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why did this get removed? Shouldn't it just be overridden in the ClangdCompleter?

Because it's not covered and we don't need to implement an abstract method in a class that is never instanced. Also, the language is rarely going to be the same as the server's name.

---

ycmd/completers/language_server/simple_language_server_completer.py, line 93 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

maybe use GetServerName ?

See answer above.

---

ycmd/completers/python/python_completer.py, line 80 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is there any reason why a user would care about filename for a python project?

Not particularly but it's for consistency with other completers.

[bstaletic]

Reviewed 4 of 4 files at r4.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @kadircet and @micbou)

---

ycmd/completers/language_server/language_server_completer.py, line 1163 at r4 (raw file):

      return

    for handler in reversed( self._on_file_ready_to_parse_handlers ):

Any reason for reversed()?

[micbou]

Reviewed 2 of 2 files at r5.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic, @kadircet, and @micbou)

---

ycmd/completers/language_server/language_server_completer.py, line 1163 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Any reason for reversed()?

We want to send the flags to Clangd before updating the file contents so _UpdateServerWithFileContents must be run last.

[bstaletic]

Reviewed 2 of 2 files at r5, 2 of 2 files at r6.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @micbou)

---

ycmd/completers/language_server/language_server_completer.py, line 1163 at r4 (raw file):

Previously, micbou wrote…

We want to send the flags to Clangd before updating the file contents so _UpdateServerWithFileContents must be run last.

That makes a lot of sense, but it isn't obvious at first sight. Could you add a comment about it?

[kadircet]

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @micbou)

---

ycmd/completers/cpp/clangd_completer.py, line 218 at r3 (raw file):

Previously, micbou wrote…

We can't use the Flags class because parts about the compilation database depend on libclang which is not available if support is not compiled in.

OK, thanks for explaining.

---

ycmd/completers/cpp/clangd_completer.py, line 411 at r3 (raw file):

Previously, micbou wrote…

I think PR #1035 was a mistake and I don't want to repeat it. For users that want the compilation database of a project to be used instead of the global extra conf, there is now the solution to not return the flags field for that project.

Correct me if I am wrong, but I thought the point of global extra conf was to provide users with a "fallback mode". So that people can generate some "project-independent flags" in the case of everything else has failed. Adding special logic to exclude some paths(which will change over time) doesn't sound sensible to me and likely to put a negative effect on clangd's usability.

Currently it is enough for people to generate a compilation database, after that patch it would also require people to modify their global extra conf to not return any flags for a given directory. I am not saying that this would break those users, since clangd completer is experimental it is OK to have such breakages, but I believe this decision rather results in bad user experience.

---

ycmd/completers/language_server/language_server_completer.py, line 1099 at r6 (raw file):

  def GetSettings( self, module, request_data ):
    if hasattr( module, 'Settings' ):
      settings = module.Settings(

Why not cover this call with a try/except?

---

ycmd/completers/language_server/simple_language_server_completer.py, line 37 at r3 (raw file):

Previously, micbou wrote…

A completer may have a different name than the server e.g. in Java, the completer is named Java but the server is jdt.ls.

I don't follow, Java sounds like the language that semantic completer provides support for. Whereas jdt.ls sounds like the completer itself, which in my opinion doesn't have any difference with server.

It just seems like duplication to me, either language and completer(according to your point of view) or completer and server(my view) provides the same info.

I don't wanna dwell so much on this one, in the end you guys are the maintainers, but still would like to hear @bstaletic's point as well.
I am OK with it anyways.

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @micbou)

---

ycmd/completers/language_server/language_server_completer.py, line 1099 at r6 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

Why not cover this call with a try/except?

Hm... A try/except would avoid two lookups of Settings in object module.

---

ycmd/completers/language_server/simple_language_server_completer.py, line 37 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

I don't follow, Java sounds like the language that semantic completer provides support for. Whereas jdt.ls sounds like the completer itself, which in my opinion doesn't have any difference with server.

It just seems like duplication to me, either language and completer(according to your point of view) or completer and server(my view) provides the same info.

I don't wanna dwell so much on this one, in the end you guys are the maintainers, but still would like to hear @bstaletic's point as well.
I am OK with it anyways.

I'm with @kadircet here. "language", "server name" and "completer name" do overlap and maybe ycmd could do with two of the three. On the other hand, it's not super important and I wouldn't block this PR on it. If there is an agreement to change this, it can be changed consistently in another PR.

[puremourning]

Looks good with a few outstanding questions to bottom out.

Reviewed 10 of 14 files at r1, 1 of 1 files at r3, 2 of 4 files at r4, 2 of 2 files at r6.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @micbou)

---

ycmd/completers/cpp/clangd_completer.py, line 411 at r3 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

Correct me if I am wrong, but I thought the point of global extra conf was to provide users with a "fallback mode". So that people can generate some "project-independent flags" in the case of everything else has failed. Adding special logic to exclude some paths(which will change over time) doesn't sound sensible to me and likely to put a negative effect on clangd's usability.

Currently it is enough for people to generate a compilation database, after that patch it would also require people to modify their global extra conf to not return any flags for a given directory. I am not saying that this would break those users, since clangd completer is experimental it is OK to have such breakages, but I believe this decision rather results in bad user experience.

I"m not totally following this discussion but I think the resolution of extra conf/global extra conf and compilation db should be consistent between the lib clang and clang completer because that's what users would expect if/when they migrate.

Then again, I always had my reservations about the current model from the linked pr.

so I guess I'm ambivalent, so long as we can clearly document it without sounding like we're documenting emergent behaviour.

---

ycmd/completers/cpp/clangd_completer.py, line 207 at r6 (raw file):

def PrependCompilerToFlags( flags, enable_windows_style_flags ):
  """Removes everything before the first flag and returns the remaining flags
  prepended with clangd."""

clang-tool

---

ycmd/completers/cpp/clangd_completer.py, line 447 at r6 (raw file):

    return [ responses.DebugInfoItem(
      'Extra Configuration Flags',
      self._flags_for_file.get( request_data[ 'filepath' ], False ) ) ]

do we really want 'false' as the default. perhaps an empty list or None ?

[kadircet]

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic, @kadircet, and @micbou)

---

ycmd/completers/language_server/language_server_completer.py, line 1099 at r6 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Hm... A try/except would avoid two lookups of Settings in object module.

In addition to that I was also worried about user's Settings function throwing an exception, which unfortunately kills the ycmd(silently), at least this is what I experienced during testing.

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @micbou)

---

ycmd/completers/language_server/language_server_completer.py, line 1099 at r6 (raw file):

Previously, kadircet (kadir çetinkaya) wrote…

In addition to that I was also worried about user's Settings function throwing an exception, which unfortunately kills the ycmd(silently), at least this is what I experienced during testing.

The silent part is surprising. There should be something in :messages if using vim. In other words, the exception should be propagated to the client.

[kadircet]

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @micbou)

---

ycmd/completers/cpp/clangd_completer.py, line 218 at r3 (raw file):

arts about the compilation database depend on libclang which is not available if support is not compiled in.

Sorry I misread this one, we actually don't want to use compilation database loading mechanism in cpp.flags right? I believe it would be better to refactor that part so that it can be skipped when called through clangd. WDYT?

[puremourning]

I just tried this out on a project. I had to convert my .ycm_extra_conf.py to use the Settings method rather than FlagsForFile. Is that intentional?

Either way; it worked perfectly with no changes to the extra conf file (other than that one), which is very neat.

So based on that!

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @micbou)

[puremourning]

Rebased at #1267

[puremourning]

Superseded by #1267. Thanks to @micbou for the PR!