-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix compilation database not listing every Swift source file #14264
base: master
Are you sure you want to change the base?
Conversation
58dee7a
to
46bbd88
Compare
8a89ecc
to
26d874b
Compare
d = {'directory': builddir, 'arguments': arguments, 'file': file} | ||
jsondb_data += [d] | ||
with open(os.path.join(builddir, 'compile_commands.json'), 'w', encoding='utf-8') as f: | ||
json.dump(jsondb_data, f, indent=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not a fan of hotpatching the compilation database in this manner. For example in C you can write #include<otherfile.c>
and it is not reflected in the compilation db at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about the compilation entrypoints though, C LSPs should be smart enough to follow includes, but they can't do that if none of the files are known to it. This is the latter case.
For example, a Swift target with two source files A.swift and B.swift will only have A.swift in the compilation database since both are compiled with the same rule (Swift doesn't support per-file compiles in its stable CLI interface and neither does the LSP), and this gets worse the more source files you have, making LSP work for almost no files without this patch.
Adding entries to the compilation database is the only obvious option I've seen, since Ninja doesn't have enough information to generate this itself (which input corresponds to which output file?) and also explicitly states here that only the first file will be used:
given a list of rules, each of which is expected to be a C family language compiler rule whose first input is the name of the source file, prints on standard output a compilation database in the JSON format expected by the Clang tooling interface. Available since Ninja 1.2.
(Also see ninja-build/ninja#1590)
What do you suggest instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you suggest instead?
What is the exact thing you need this information for? Or in other words, is getting the same information via Meson introspection not sufficient?
Th file in question is the compilation command database after all. It lists compilation commands. That's what it is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SourceKit-LSP (such as with the Swift VSCode plugin) uses this file to provide syntax highlighting/errors for source code. For it to work, it needs an entry in the compilation database for each source file.
I don't think they'll want to add explicit Meson support...
26d874b
to
9088d2a
Compare
This makes SourceKit-LSP work correctly with multi-source-file targets.
9088d2a
to
dcf62f3
Compare
For SourceKit-LSP to highlight Swift sources correctly, the compile_commands.json has to contain an entry for each file with the corresponding "file" key. However, Ninja generates only one entry per build element, with the first input file as the input file for the compilation database entry, which is a problem for Swift targets consisting of multiple files since they are compiled with one command invocation per target instead of one per file. This adds entries for the build element for each input file to make LSP work for the rest of the source files to the compilation database.