clang-format-18 false negative when using function-try block

[Alexolut]

I have code like this, formatted locally with clang-format. I use function-try-block here:

void TestFunc(const std::string& data) try {
    do_something();
    do_something_more();
} catch (const std::exception& ex) {
    log_error(ex);
}

cpp-linter has complaints on this code, pointing to the line with catch (99):

Notice: File filename.cpp does not conform to Custom style guidelines. (lines 99)

At the same time I don't see any diff suggestions in PR despite I have format-review: true and if I intentionally add wrongly-formatted changes on other lines - I will see only diff for those lines (but line with catch still won't be in diff for some reason).

Full list of options that I use:

        uses: cpp-linter/[email protected]
        id: linter
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        with:
          style: file
          tidy-checks: '-*'
          version: '18'
          step-summary: true
          lines-changed-only: true
          format-review: true
          passive-reviews: true
          thread-comments: update

Looks like everything is OK with formatting, but cpp-linter mark that code as not-formatted.

[2bndy5]

What does the review summary comment say? The format-review feature should attach a full diff of desired changes in the review's summary comment. It would be collapsed under the text "Click here for the full clang-format patch". For example: https://cpp-linter.github.io/cpp-linter-action/images/format-review.png

I have to ask, what triggers are used by your CI workflow? I think I asked you this before but got no answer. For example the triggers are under the yaml field on:

on:
  pull_request:
    branches: [main, master, develop]
    paths: ['**.c', '**.cpp', '**.h', '**.hpp', '**.cxx', '**.hxx', '**.cc', '**.hh', '**CMakeLists.txt', 'meson.build', '**.cmake']
  push:
    branches: [main, master, develop]
    paths: ['**.c', '**.cpp', '**.h', '**.hpp', '**.cxx', '**.hxx', '**.cc', '**.hh', '**CMakeLists.txt', 'meson.build', '**.cmake']

Also, which lines are changed in the following code?

void TestFunc(const std::string& data) try {
    do_something();
    do_something_more();
} catch (const std::exception& ex) {
    log_error(ex);
}

[Alexolut]

In general I don't have problems with other code. That's why I think the triggers not relevant here.

Only the line with catch is problematic from cpp-linter point of view. All lines of code that I mentioned are new. So the entire function is checked.

[2bndy5]

If you don't answer all my questions, then I'm unable to help/diagnose the problem. I'm guessing this has occurred on a private repo.

[2bndy5]

formatted locally with clang-format

Are you also using clang-format v18 locally?

what triggers are used by your CI workflow?

This is important because the review may not be updated based on what event triggers the workflow.

What does the review summary comment say?

I'm curious if the "full clang-format patch" includes the line about catch.

[Alexolut]

The repo is private. Excerpt from related yaml file:

on:
  workflow_call:

I formatted entire file locally with clang-format 18.1.7 (on CI I see 18.1.8 but I had faced similar problem before while CI had 18.1.7 as well) before push to PR.

When I changed code into this (added extra spaces inside of parentheses):

void TestFunc(const std::string& data) try {
    do_something();
    do_something_more(      );
} catch (const std::exception& ex) {
    log_error(   ex    );
}

I got a message with diff in PR review:

I.e. no problems on line with catch are detected in CI side! That's why I think triggers and difference in minor versions of LLVM are not relevant in my case.

When I apply the diff (i.e. format file again locally) I will see:

• no diff on review page
• complaints about line with catch

The only way I found to work around the problem (get no complains from linter side) - don't use function-try-block and use regular try-block like this:

void TestFunc(const std::string& data) {
    try {
        do_something();
        do_something_more();
    } catch (const std::exception& ex) {
        log_error(ex);
    }
}

[2bndy5]

on:
  workflow_call:

So it is a reusable workflow. What are the triggers for the actual workflow that invokes the reusable workflow? I ask because reusable workflows inherit the event type from the calling workflow.

I formatted entire file locally with clang-format 18.1.7 (on CI I see 18.1.8 but I had faced similar problem before while CI had 18.1.7 as well) before push to PR.

I was more concerned about a difference between major versions. I doubt this problem is occurring from a difference between patch versions

When I apply the diff (i.e. format file again locally) I will see:

• no diff on review page
• complaints about line with catch

It sounds like format-review is working as expected, but you keep seeing a message from the file-annotations feature. Since this isn't the first time you have had complaints about the file-annotations feature (and you're more focused on diff suggestions), I would recommend you disable it:

file-annotations: false

Note

We have no way of deleting an outdated file annotation because they are only relevant to the commit that created the annotation. This usually isn't a problem because refreshing the list of file changes in a PR will only show annotations created by the latest commit to the head branch.

It could be that the line number in the annotation is incorrect, but this is highly unlikely.

[Alexolut]

I disabled annotation as you suggested file-annotations: false. Below the output from action run:

Performing checkup on filename.cpp
  INFO:CPP Linter:Running "/usr/bin/clang-format-18 -style=file --output-replacements-xml --lines=96:103 filename.cpp"
  INFO:CPP Linter:Getting fixes with "/usr/bin/clang-format-18 -style=file --lines=96:103 filename.cpp"
  
Posting comment(s)
  INFO:CPP Linter:1 clang-format-checks-failed
  INFO:CPP Linter:0 clang-tidy-checks-failed
  INFO:CPP Linter:1 checks-failed
  ERROR:CPP Linter:response returned 403 from PUT https://api.github.com/repos/*/reviews/*/dismissals with message: {"message":"Branch protections do not permit dismissing this review","documentation_url":"https://docs.github.com/rest/pulls/reviews#dismiss-a-review-for-a-pull-request","status":"403"}
  ERROR:CPP Linter:response returned 403 from PUT https://api.github.com/repos/*/reviews/*/dismissals with message: {"message":"Branch protections do not permit dismissing this review","documentation_url":"https://docs.github.com/rest/pulls/reviews#dismiss-a-review-for-a-pull-request","status":"403"}

As far as I understand the line INFO:CPP Linter:1 clang-format-checks-failed means that the problem persist. Do have an ability (public sandbox?) to run your cpp-linter with my code snippet?

[2bndy5]

Do have an ability (public sandbox?) to run your cpp-linter with my code snippet?

No. We would need more than just the snippet:

• the .clang-format config file you are using
• any compilation database generated would have to be significantly altered (clang-tidy only)

You can enable cpp-linter's debug output:

verbosity: debug # default is 'info'

If I recall correctly, this should show the output from

/usr/bin/clang-format-18 -style=file --lines=96:103 filename.cpp

[Alexolut]

Look like the output is empty line? But same output here was before changing verbosity to debug.

the .clang-format config file you are using

I hope that default based on Google-style will be enough to reproduce

[2bndy5]

I forgot, clang-format output is basically the modified source. With format-review enabled, the modified source is captured into a buffer that is used to generate a diff (which corresponds to the code suggestions).

Could this just be a difference in LF vs CRLF? git typically normalizes line endings according to .gitattributes and git config settings

[Alexolut]

Could this just be a difference in LF vs CRLF?

I don't think so. All lines have the same ending. But complaining only for the one with catch.

I would like to repeat that the problem not raising with regular try/catch block, only with function-try-block. Looks like clang-format (or other wrapper) returns non-zero exit code to cpp-linter, but doesn't suggest any changes.

[2bndy5]

If clang-format was returning a non-zero exit code, then it would have been reported with debug output enabled.

    logger.info('Running "%s"', " ".join(cmds))
    results = subprocess.run(cmds, capture_output=True)
    if results.returncode:
        logger.debug(
            "%s raised the following error(s):\n%s", cmds[0], results.stderr.decode()
        )

(from src here)

[2bndy5]

What is the output when you run the following command locally?

clang-format --output-replacements-xml -style=file --lines=96:103 filename.cpp

(assuming clang-format-18 is synonymous with a clang-format call)

I'm curious to see what the XML output is. It might be that v18 uses a format that is different from previous versions' XML output (we've seen that problem before).

[2bndy5]

I hope that default based on Google-style will be enough to reproduce

Its not. You clearly use 4-space indents, Google guidelines mandate 2-space indents. I wouldn't be surprised if you deviate from their guidelines further.

[Alexolut]

output-replacement-xml returns me the following:

<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='2789' length='0'>&#13;&#10;    </replacement>
</replacements>

Offset 2789 points right after the curly brace:

} catch (const std::exception& ex) {
                                    ^

I'm not sure what does it mean. How to read this xml-output.

You clearly use 4-space indents

I mean that for the sake of reproducibility it (I hope) should not depend on .clang-format style. Just try to format my code snippet with your settings and look into response from cpp-linter, if it's not so hard for you.

[2bndy5]

Using the xml feedback that you provided, I saved this locally to a file named "fmt.xml". Then I opened a python interpretter to parse the xml similarly to how cpp-linter does:

>>> from pathlib import Path
>>> f = Path("fmt.xml").read_text(encoding="utf-8")
>>> f
"<?xml version='1.0'?>\n<replacements xml:space='preserve' incomplete_format='false'>\n<replacement offset='2789' length='0'>&#13;&#10;    </replacement>\n</replacements>\n"
>>> import xml.etree.ElementTree as ET
>>> tree = ET.fromstring(f)
>>> len(tree)
1
>>> tree[0]
<Element 'replacement' at 0x7f0ce8f45170>
>>> tree[0].text
'\r\n    '
>>> int(tree[0].attrib["offset"]) # the position where replacement begins
2789
>>> int(tree[0].attrib["length"]) # the number of characters to remove
0

Given that you said offset 2789 corresponds to the end of the catch( ... ) line, the suggested fix is appending \r\n (4 spaces after a CRLF) after the {.

How to fix this?

I'm not sure what is happening in cpp-linter. I have to investigate the code to make sure that this case is covered. It certainly should be.

[2bndy5]

Does you project use mixed line endings? Or only LF? Or only CRLF?

[Alexolut]

According your explanation (if I understand it correctly) looks like clang-format expects the following:

After that linter suggests me to remove entire line 100!

Mind blowing.

I work on Windows as well. So \r\n only.

[2bndy5]

It looks like you did understand my analysis. This feels like it might be a bug in clang-format v18. I'm going to research how pygit2 (python binding we use for libgit2) deals with creating a patch from 2 buffers. The resulting patch is what cpp-linter uses to create code suggestions.

[2bndy5]

Can you double check the offset is translated correctly into line & column? It is a 1-based count.

You can do this with a python prompt:

from pathlib import Path

offset = 2789
filename = "filename.cpp" # replace this with the actual filename
ba = Path(filename).read_bytes()[:offset]
lines = ba.count(b"\n") + 1  # lines are a 1-based count
cols = offset - ba.rfind(b"\n")
print("line:", lines, "col:", cols)

This is an expanded version of the code that cpp-linter uses

[Alexolut]

You can do this with a python prompt

line: 99 col: 37

Exactly the same position that I mentioned before

} catch (const std::exception& ex) {
                                    ^

[Alexolut]

It seems that we can post an issue (bug) on LLVM GH only based on the assumption that we must get "empty" output-replacements-xml, e.g.:

<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
</replacements>

if we did clang-format with fix before on the same .clang-format settings. Am I right?

[2bndy5]

I would like to repeat that the problem not raising with regular try/catch block, only with function-try-block

I would focus on this fact.

The function-try-block is basically syntactic sugar for encapsulating the entire function body in a try-catch block. However, it is ideally used for a class' constructor that derives from a base class. Other use cases (like what you're doing) are really just a clever workaround for projects with restrictions on line length.

---

Am I right?

That is what an empty XML should look like, yes. If you do raise this upstream, then please leave a reference to this thread.

[2bndy5]

Hmm, I'm looking at the diff flags that cpp-linter can use when creating a patch that gets translated into code suggestions in a PR review. Most diff flags don't really apply to our use case, which creates a patch from 2 arbitrary buffers, not anything in the index or working tree. However, I did find a flag that might be applicable to our specific usage:

• GIT_DIFF_INDENT_HEURISTIC: Use a heuristic that takes indentation and whitespace into account which generally can produce better diffs when dealing with ambiguous diff hunks.

Although, I'm not sure if this flag would help avoid this problem (as reported in OP). I doubt it would hurt anything if I added it to cpp-linter.

[2bndy5]

Conclusion

This is definitely a bug in clang-format.

In cpp-linter, we use the XML output to count the suggested format fixes and represent it to the user as the clang-format-checks-failed output. To workaround this, cpp-linter would have to compare the formatted source with the un-formatted source for each source file. Such a workaround would prove very costly in terms of memory consumption/runtime on large files and/or large repositories, and that's not even considering the extra post-processing needed to satisfy our lines-changed-only and files-changed-only options.

To work around this problem, I would suggest you either

1. take advantage of clang-format's toggle comments
2. revert to a version that is not affected by this bug (if any exists)
3. switch to a normal try-catch block that encapsulates the function's entire body