Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

.h files interpretted as C #12

Closed
HaberR opened this issue Dec 20, 2021 · 17 comments
Closed

.h files interpretted as C #12

HaberR opened this issue Dec 20, 2021 · 17 comments

Comments

@HaberR
Copy link

HaberR commented Dec 20, 2021

I'm working on a project in which we use .h files for cpp headers. For some reason, clangd treats these like C files after I've run the hedron commands extractor, and then VSCode complains a lot because we're including C++ libraries in what it perceives to be C code. Is there some extra piece of config I can provide to either this tool, or to clangd directly that will instruct it to treat these files like cpp?

@cpsauer
Copy link
Contributor

cpsauer commented Dec 21, 2021

Hey, @HaberR! Thanks for giving the tool a try. Sorry things aren't working as expected.

As background:
This is caused by a clangd issue that generally plagues tooling, but a good chunk of the code of this tool (unlike others) is there to work around it.

We also have files like this internally. That behavior is super annoying, so we spent time trying to make this case work for everyone. Sorry that isn't working for you; there must be a subcase we hadn't considered. Let's figure out where things are going wrong!

How the fix is intended to work:
We extract the headers used (transitively) by a specific source file and then additionally emit compile_commands.json entries for the headers, marking them as being compiled the same way as their source files.

It's be great to figure out where in the process things are going wrong. Could I ask for your help? (I'm assuming there isn't public code I can play around with?)

For a given .h file that's giving us trouble:

  1. Is there indeed an entry for that header in compile_commands.json?
    [Confirms that the fix is trying to apply itself to the header.]
  2. Is there a language specifier in the command associated to the entry?
    For example: -std=gnu++17
    [Hypothesis: There won't be, and this is the problem. The project doesn't specify an explicit language standard, so that isn't applied to the header. We missed this case, because we, and the other test projects, explicitly set the language standard.]
  3. If there isn't a language specifier, does adding one solve the issue?
    Quick test: Add one manually and save. Recent clangd versions should automatically pick up the change on save and refocusing the file--and tell you so in their debug output.
    Interim fix if this works--but we'll get to it quickly: You can temporarily add, e.g., build --cxxopt=-std=gnu++17 to your .bazelrc, to specify the language standard, and then refresh compile_commands.json.
  4. If there is already a language specifier or 3 doesn't work, it's be great to make sure clangd is actually picking up the compile_commands.json.
    We want to look at the clangd output logs. In VSCode, you can get to them through [Terminal Tab]>Output>[Dropdown]>clangd.
    Then open up the header in question. Hopefully the log has an entry something like "ASTWorker building file with command <...>"

Anyway, lmk! I'll also run some tests over here shortly.

@HaberR
Copy link
Author

HaberR commented Dec 21, 2021

Thanks for getting back to me, I've confirmed that:

  1. There is an entry for the header in compile_commands
  2. Each line has the option --std=c++17 specified

Here's the clangd output when I open the file:
I[02:58:07.727] <-- textDocument/codeAction(225)
I[02:58:07.727] --> reply:textDocument/codeAction(225) 0 ms
I[02:58:07.727] --> textDocument/clangd.fileStatus
I[02:58:07.729] <-- textDocument/documentLink(226)
I[02:58:07.764] --> reply:textDocument/documentLink(226) 34 ms
I[02:58:07.766] --> textDocument/clangd.fileStatus

Not sure if it's any help, but the specific errors VSCode references are:
Unknown argument: '-fno-canonical-system-headers'clang(drv_unknown_argument)
Invalid argument '--std=c++17' not allowed with 'C'clang(drv_argument_not_allowed_with)

I can get the -fno-canonical-system-headers error to go away by deleting that from compile_commands, but deleting --std=c++17 seems to just push the issue further along (I get cstddef not found elsewhere).

cpsauer added a commit that referenced this issue Dec 21, 2021
clangd 13 otherwise wrongly assumes them all them are C, regardless of the source file being compiled in the command.

#12
@cpsauer
Copy link
Contributor

cpsauer commented Dec 21, 2021

Yeah, of course. Thanks for digging in. That that really narrowed down the problem.

I can reproduce the issue! Or at least the Invalid argument '--std=c++17' not allowed with 'C' clang(drv_argument_not_allowed_with) part. (On my machine, C++ completion does seems to work nicely on my machine for those files, though.)

Further, I can solve it by manually specifying the language, inferring it from the extension from the source file that included it! I've just pushed up a commit that does just that.

(In the background: Clangd 13 seems to detect the language from the file name, assume all .h files are C, and then ignore any evidence in the command to the contrary.)

Woo. Okay! So could you let me know if that fixes things for you?

@cpsauer
Copy link
Contributor

cpsauer commented Dec 21, 2021

[I don't anticipate that commit fixing the -no-canonical-system-headers error message, since that's pretty different. From a quick Google, it looks like that's a GCC-specific flag that clang doesn't support. I think clangd would gracefully recover, but if it breaks things or gets annoying, lmk. We could automatically strip it out of compile_commands.json via refresh.template.py::_all_platform_patch]

@HaberR
Copy link
Author

HaberR commented Dec 21, 2021

Amazing, it works!!! This is awesome, nobody should have to develop without autocomplete. I do still see the squiggle for -no-canonical-system-headers but that's small potatoes. Thanks for sharing this project, and for the incredible support!

@HaberR HaberR closed this as completed Dec 21, 2021
@cpsauer
Copy link
Contributor

cpsauer commented Dec 22, 2021

Huzza! Delighted to hear it, @HaberR. Good autocomplete makes me smile, too :)

Thank you for bringing this up and for working with me to narrow it down and fix it. It'll help out a lot of others, too.

Please let me know if there are other rough edges you see--or if you have ideas on how to make things even better.

-Chris

@cpsauer
Copy link
Contributor

cpsauer commented Dec 23, 2021

Hey so one other update: The (awesome) clangd folks are going to fix this case, after some discussion. See https://reviews.llvm.org/D116167 for more.

When it's released, I'll test and then revert my workaround to keep things clean and simple.

cpsauer added a commit that referenced this issue Dec 29, 2021
…4fc5e for #12.

Makes it harder to forget to revert and simplify. (Also have an automated personal reminder)
@cpsauer
Copy link
Contributor

cpsauer commented Apr 6, 2022

@HaberR, could I ask for your help?

In a sec, I'm going to revert the workaround, since clangd 14 was just released, which should contain their fix for the underlying problem.

Could I ask you to quickly update (both this tool and clangd) and make sure that the issue doesn't come back? I've tested quickly myself, but I'd love it if you'd double check. Plus, there are a lot of good fixes in the latest :)

-Chris

cpsauer added a commit that referenced this issue Apr 6, 2022
@HaberR
Copy link
Author

HaberR commented May 26, 2022

Hi, sorry for the delay, just seeing this, thanks for checking in!

I am actually bumping into this issue again.. I'm wondering if maybe it's because I didn't execute:

code --install-extension llvm-vs-code-extensions.vscode-clangd
# We also need make sure that Microsoft's C++ extension is not involved and interfering.
code --uninstall-extension ms-vscode.cpptools

I'm running my code server on a remote machine and only connecting to it locally--I don't think those instructions would work for my case?

At the top of my .h files I'm getting the following error:

Invalid argument '--std=c++17' not allowed with 'C'clang(drv_argument_not_allowed_with)

Running clangd v0.1.17, and compile-commands-extractor af9af15f7bc16fc3e407e2231abfcb62907d258f

@cpsauer
Copy link
Contributor

cpsauer commented May 26, 2022

Hey, @HaberR! Thanks for reaching back out.

Trying to scope: Sounds like the error is back for you now that I reverted the workaround--clangd having released a fix to the underlying issue. I'm assuming that this reemerged when you updated this tool to include the commit that undid the workaround. Is that right?

Could I first ask you to confirm that you're using the latest clangd (>=14)? (That is, at least version 14 of clangd the binary--not just the latest vscode extension.) You can double check this by looking at the top of clangd's output in VSCode (View>Output, then select clangd from the dropdown). And if clangd is too old, you can update by opening the command palette and typing "clangd check for update".

Sorry for the hassle; just want to make sure that the solution isn't as simple as upgrading clangd.

[I'm afraid I haven't used all this with a remote dev environment, so I'm not sure how clangd works with all that. But if it worked before, then it seems more likely that the problem is the above.]

@HaberR
Copy link
Author

HaberR commented May 26, 2022

gah, yup wrong clangd binary, upgrading that fixed it. Thank you, and sorry for the trouble!

@cpsauer
Copy link
Contributor

cpsauer commented May 26, 2022

Woo! Great. Good you wrote in--and glad it's working.

While we're chatting, anything special users should know if they're trying to set things up for remote dev? Or does it just work out of the box?

I'm also curious how you're approaching updating this tool in your WORKSPACE--just so I can keep tabs on what folks are doing.

[And if there's anything else feedback wise that I should know, really, please say! I'd love to hear it.]

@HaberR
Copy link
Author

HaberR commented May 26, 2022

Don't remember needing to do anything special--I think it just worked right out of the box!

Re our usage: In order to make the setup a little easier for the rest of my team, I've added the clangd config to our settings.json and similarly configured our extensions.json. Things also tend to work a bit better when I use this in conjunction with the 'attach to running container' functionality to connect to our dev environment. A big part of that comes from the fact that when I run from outside of that container, <nlohmann/json.hp> gives me

In included file: 'filesystem' file not foundclang(pp_file_not_found)

and that doesn't happen from within the container. Unfortunately that single issue can throw a good chunk of the type-checking off (also unfortunate is the fact that we include that library in lots of places).

I appreciated some of the new features (automatic symlinks, more readable compile_commands.json, automatic .gitignore update). My only complaint is that it can be a touch slow for the intellisense to kick in when I open a new cc file (3 seconds?) but that's a small price to pay!

I know the recommendation is to use Renovate for the purpose of updating our dependencies, but I don't think we have any such tool in place... At the moment I think our upgrade strategy is to do it when someone notices a new feature that they want (or when they're forced to) 😬

@cpsauer
Copy link
Contributor

cpsauer commented May 28, 2022

Sweet. Thank you so much @HaberR for your unfiltered feedback there. It really helps improve my mental model of how you (and others!) use this.

Sounds like with the attach you manage to get around that issue? (Please lmk if wrong--not sure I'm totally sure what's going on, but I'm guessing it has to do with referencing STL headers at locations only found inside the container.)

On the speed: Pretty sure that's clangd's indexing speed, sadly. (We're just generating the compile_commands.json, which I'd like to make faster, too!) CCLS is a bit faster, apparently, but I think clangd is probably the right call long term.

And I appreciate your honestly about Renovate! While I'd recommend it, I really appreciate your telling me that you don't use it. No need to feel bad. In fact, incredibly valuable to help me get a sense of how people are actually approaching updates.

Thanks again!
Chris

@HaberR
Copy link
Author

HaberR commented Jun 7, 2022

Hi! So I recently demoed this tool to some of my teammates and I've gotten a lot of love for it, and wanted to forward that your way! Daniel wrote me
"I've been writing C++ today and OMG it's so much better with the tool you showed off--actually checking BEFORE I compile"
So yeah, we're into it!

A few follow ups

  • yes attaching to the container does tend to fix that issue, and I think that's exactly right about some of the stuff only being visible from within the container.
  • I've noticed (and I might be wrong on this) that when a headerfile sets up a type-alias like so: using Vec = vector<T, Alloc<T>>; the alias tends not to be included properly in the consuming cc file. Can provide the exact error when I get in tomorrow.
  • While we're not using Renovate yet, when I asked teammates about it there was definitely an appetite for it. The response seemed to be that we just needed some of our testing infrastructure to be a bit stronger before we felt ready for it.

@cpsauer
Copy link
Contributor

cpsauer commented Jun 7, 2022

Hey, I'm so glad. Puts a big smile on my face that this is helping you guys. Thanks for forwarding :) I'm curious what you all are building together with it!

@HaberR
Copy link
Author

HaberR commented Jun 7, 2022

I work for Third Wave Automation, we're building autonomous forklifts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants