jacobdufault / cquery

C/C++ language server supporting multi-million line code base, powered by libclang. Emacs, Vim, VSCode, and others with language server protocol support. Cross references, completion, diagnostics, semantic highlighting and more
MIT License
2.34k stars 163 forks source link

Some cquery initialization options should be supplied on a per-project configuration #563

Open edjubuh opened 6 years ago

edjubuh commented 6 years ago

I was playing around with blacklisting files from displaying diagnostics (clang really doesn't like some system headers from newlib). Blacklisting files in the Config struct is exactly what is supposed to solve this problem, but the only way to configure this is by configuring the LSP client to send the correct initialization options, which are typically hard-coded based on the capabilities of the client or configured in an editor-wide (not per-project) manner.

I'd like to propose modifying cquery to allow per-project client-independent initialization options. The client provides a projectRoot, so cquery could look for some file inside the projectRoot with additional configuration options. One option could be to look for a .cquery file directly inside projectRoot and parse the file as a configuration options after some special identifier, e.g.:

%clang
%c -std=gnu11
%cpp -std=gnu++14
-pthread

# Includes
-I/work/cquery/third_party
-I/work/cquery/another_third_party

# Initialization options follows
%%%%
{
    "diagnostics": { "blacklist": ["filea.h"] }
}

An alternative to extending the .cquery file would be to create a new file (.cquery.init maybe?). That doesn't sound too attractive since it's a whole new file that users have to have, but would be simpler on cquery's end (no modification to the code for parsing of .cquery files).

We wouldn't worry about doing any sort of recursive search for a .cquery file since that increases the complexity of resolving file paths and I'm sure we could get better logic for this behavior down the road, if we discover it's needed.

We would do a "deep merge" (e.g. join vector lists, join objects), where the project? configuration has priority. This would allow an editor to set the cache directory, but a per-project configuration to override it. This would also allow a project to override some not-so-immutable options like projectRoot. Not sure if we should just document "don't add to these configuration options in your project config" or do something a bit more serious. The search and parsing for this configuration file should probably occur in Project::Load so that it can respect workspace/DidChangeConfiguration and the initialize request.

I don't believe any of the editors provide a way to provide project-based initialization options, and if editors gain this ability, they could still do a merge of the initialization options on their own. This proposal gives every editor this per-project configuration ability without requiring any special support from clients.

If this sounds good from an architectural standpoint, I could probably get a PR in for it by the end of the week if it goes smoothly.

pelmers commented 6 years ago

Vscode has always had per-project workspace settings: https://code.visualstudio.com/docs/getstarted/settings And Atom recently added per-project configuration via https://github.com/atom/atom/pull/16845

edjubuh commented 6 years ago

@pelmers That's true, but I don't believe the cquery plugin for VS code allows users to configure the initialization configuration object beyond extraArgs. I was tempted to leverage per-project configuration for ide-cquery in Atom, but thought this might be a better solution that allows any editor to benefit.

jacobdufault commented 6 years ago

I had typed some stuff up here last night but it looks like I forgot to submit it - the right way to deal with this is as @pelmers stated, but I'm also okay with a .cquery.config.json (or similarly named file) that specifies init options for clients which lack this functionality.

Note that we also support --init which handles config merging. iirc it only does top-level merging; probably doing deeper merging is not worth the complexity (unless it proves to be very simple, but I don't expect it to be).

jacobdufault commented 6 years ago

but I don't believe the cquery plugin for VS code allows users to configure the initialization configuration object beyond extraArgs.

The vscode client exposes dedicated settings which map to the init config. Not everything is exposed though.

MaskRay commented 6 years ago

--init only does top-level merging. If a top-level field is specified in --init, it will override that from initialization options.

edjubuh commented 6 years ago

If it's preferred to do this at the client level, I don't have a problem with implementing it there. I'm gonna have to do the work anyway 🙂