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

clang-format in the latest pre-release version has broken main include sorting #11914

Closed
benvanik opened this issue Feb 1, 2024 · 6 comments
Closed
Assignees
Labels
bug Feature: Code Formatting fixed Check the Milestone for the release in which the fix is or will be available. insiders Currently only repros with our latest Insiders release. Language Service regression A bug that didn't exist in a previous release
Milestone

Comments

@benvanik
Copy link

benvanik commented Feb 1, 2024

Environment

  • OS and Version: Windows 10 x64
  • VS Code Version: 1.86.0 (2024-01-31)
  • C/C++ Extension Version: v1.19.2 (pre-release) ---- works with release version v1.18.5

Bug Summary and Steps to Reproduce

Bug Summary:
The clang-format included with the extension incorrectly recognizes the main file for SortIncludes and moves it to the incorrect category. The current release version works correctly.

Steps to reproduce:

  1. Create a new .clang-format file with the given contents:
BasedOnStyle: Google
  1. Configure the C/C++ formatter settings to use clang-format and the file style:
    "C_Cpp.formatting": "clangFormat",
    "C_Cpp.clang_format_style": "file",
  1. Create a new file bugtest.c with the given contents:
#include "bugtest.h"

#include <stdio.h>

#include "something_else.h"
  1. Format the document with the "C/C++" formatter

Expected behavior (with current release version v1.18.5 and all prior):

#include "bugtest.h"

#include <stdio.h>

#include "something_else.h"

Actual behavior (with the pre-release version v1.19.2):

#include <stdio.h>

#include "bugtest.h"
#include "something_else.h"

Switching back to the release version and formatting again will resort back to the expected order with the bugtest.h file at the top.

Configuration and Logs

Workspace configuration:

    "C_Cpp.formatting": "clangFormat",
    "C_Cpp.clang_format_style": "file",

This reproduces with setting "C_Cpp.clang_format_style": "Google", and not using the .clang-format file either.

Other Extensions

No response

Additional context

No response

@Colengms
Copy link
Contributor

Colengms commented Feb 1, 2024

Hi @benvanik . I'm not able to repro this issue. I get the same results with 1.18.5 and 1.19.2. I'm seeing the later behavior from both versions.

#include <stdio.h>

#include "bugtest.h"
#include "something_else.h"

We haven't updated the binary we're shipping for clang-format between these versions.

C:\Users\coleng\.vscode\extensions\ms-vscode.cpptools-1.19.2-win32-x64\LLVM\bin>clang-format.exe --version
clang-format version 17.0.4 (https://github.com/llvm/llvm-project 309d55140c46384b6de7a7573206cbeba3f7077f)
C:\Users\coleng\.vscode\extensions\ms-vscode.cpptools-1.18.5-win32-x64\LLVM\bin>clang-format.exe --version
clang-format version 17.0.4 (https://github.com/llvm/llvm-project 309d55140c46384b6de7a7573206cbeba3f7077f)

Do you have other settings set, such as C_Cpp.clang_format_path?

If the difference were due to a change in the behavior of clang-format between versions, that's something that would need to be reported to LLVM folks, as we don't control clang-format itself.

If you're still seeing a difference, can you provide more information, such as the output of the C/C++ output channel, for both repros, having set "C_Cpp.loggingLevel": "Debug" ? It seems that, perhaps prior to 1.19.2, there may have been something preventing the format operation from being applied correctly?

@Colengms Colengms self-assigned this Feb 1, 2024
@Colengms Colengms added Language Service more info needed The issue report is not actionable in its current state labels Feb 1, 2024
@benvanik
Copy link
Author

benvanik commented Feb 1, 2024

Interesting! I confirmed what you say about the clang-format version being the same in both 1.18.5/1.19.2 on my machine, and invoking from the command line produces the correct results:

C:\Users\Ben>type C:\Users\Ben\bugtest.c
#include "bugtest.h"
#include <stdio.h>
#include "something_else.h"
C:\Users\Ben>C:\Users\Ben\.vscode\extensions\ms-vscode.cpptools-1.19.2-win32-x64\LLVM\bin\clang-format --style=Google C:\Users\Ben\bugtest.c
#include "bugtest.h"

#include <stdio.h>

#include "something_else.h"

I don't have C_Cpp.clang_format_path set as far as I can tell in user settings or workspace settings. I enabled debug output but it doesn't seem to tell me what clang-format it's using, just that it is formatting (identical output in both versions):

LSP: (received) cpptools/formatDocument: file:///c%3A/Users/Ben/bugtest.c (id: 140)
LSP: (invoked) cpptools/formatDocument: file:///c%3A/Users/Ben/bugtest.c (id: 140)
Formatting document: file:///c%3A/Users/Ben/bugtest.c
Formatting Engine: clangFormat
LSP: (received) textDocument/didChange: file:///c%3A/Users/Ben/bugtest.c
LSP: (invoked) textDocument/didChange: file:///c%3A/Users/Ben/bugtest.c

If I switch to the release version, reload the UI, and format the document it does the right thing. If I switch to the pre-release version, reload the UI, and format it does the incorrect thing. No other settings are changed between them, and it reproduces 100% of the time (I'm sitting and switching versions back and forth), and since the clang-format binary is the same that seems to indicate something else.

I don't think I'm taking crazy pills, so maybe this will at least show the difference being the version of the cpptools:

bugtest.mp4

Is there a way to see the command line being passed to clang-format? Maybe it differs?

@benvanik
Copy link
Author

benvanik commented Feb 2, 2024

Ahh I was wrong, there is a tiny difference:

Release version: note the capitalized drive specifier in the Formatting document line (D):

LSP: (received) cpptools/formatDocument: file:///d%3A/Dev/iree/bugtest.c (id: 61)
LSP: (invoked) cpptools/formatDocument: file:///d%3A/Dev/iree/bugtest.c (id: 61)
Formatting document: file:///D%3A/Dev/iree/bugtest.c
Formatting Engine: clangFormat
LSP: (received) textDocument/didChange: file:///d%3A/Dev/iree/bugtest.c
LSP: (invoked) textDocument/didChange: file:///d%3A/Dev/iree/bugtest.c

Pre-release version: note that the driver specifier is now lower-case (d):

LSP: (received) cpptools/formatDocument: file:///d%3A/Dev/iree/bugtest.c (id: 22)
LSP: (invoked) cpptools/formatDocument: file:///d%3A/Dev/iree/bugtest.c (id: 22)
Formatting document: file:///d%3A/Dev/iree/bugtest.c
Formatting Engine: clangFormat
LSP: Sending response (id: 22)
LSP: (received) textDocument/didChange: file:///d%3A/Dev/iree/bugtest.c
LSP: (invoked) textDocument/didChange: file:///d%3A/Dev/iree/bugtest.c

Perhaps a clue?

@Colengms
Copy link
Contributor

Colengms commented Feb 2, 2024

Aha. I failed to repro because I wasn't using a source name that matches the header's name. Now I can repro the difference. It does appear to be a difference in casing in the command line, in an -assume-filename= argument. I have a fix, which should be available in 1.19.3, once it's ready. I'll also add logging of the command line used to invoke clang-format, to make this type of issue easier to investigate in the future.

@Colengms Colengms added bug Feature: Code Formatting and removed more info needed The issue report is not actionable in its current state labels Feb 2, 2024
@Colengms Colengms added this to the 1.19.3 milestone Feb 2, 2024
@benvanik
Copy link
Author

benvanik commented Feb 2, 2024

Thank you for the speedy response!

@sean-mcmanus sean-mcmanus added regression A bug that didn't exist in a previous release insiders Currently only repros with our latest Insiders release. labels Feb 2, 2024
@Colengms Colengms added the fixed Check the Milestone for the release in which the fix is or will be available. label Feb 2, 2024
@sean-mcmanus
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Feature: Code Formatting fixed Check the Milestone for the release in which the fix is or will be available. insiders Currently only repros with our latest Insiders release. Language Service regression A bug that didn't exist in a previous release
Projects
None yet
Development

No branches or pull requests

3 participants