One line changes don't detected as problematic in lines-changed-only=true mode

[Alexolut]

What events trigger your workflow?

on:
  workflow_call:

What OS does your workflow use?

runs-on: ubuntu-latest

How is cpp-linter-action configured?

- name: Format check
        uses: cpp-linter/cpp-linter-action@v2.17.0
        id: linter
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        with:
          style: file
          tidy-checks: '-*'
          version: '22'
          step-summary: true
          lines-changed-only: true
          format-review: true
          passive-reviews: true
          thread-comments: update
          verbosity: debug

What was the unexpected behavior?

In case I have changed only single line (or several lines but not adjacent). For example, lines 42 and 100 in .cpp file. Broken formatting will not be detected on such lines. Checks return zero statuses:

INFO:CPP Linter:0 clang-format-checks-failed
INFO:CPP Linter:0 clang-tidy-checks-failed
INFO:CPP Linter:0 checks-failed

If I do changes in 2 or more adjacent lines (e.g. 42 and 43) invalid formatting will be detected correctly.

Works fine when only single line is changed but lines-changed-only set to diff.

Seems the problem arises when we have same numbers on --lines option to clang-format. E.g.:

--lines=42:42 --lines=100:100

Checked with old clang-format 18.1.8 as well. Same issue

[2bndy5]

This is likely a regression from resolving #388. In that issue we had concluded that the line range was inclusive. Whereas adding 1 to the range-end had triggered format errors outside the scope of the lines changed (and once fixed would trigger more out-of-scope format errors...).

I suppose the case of a single line is considered an edge case where the range-end must be +1.

There's also a possibility that our suggestion filtering is omitting format suggestions when it involves more than just the single line.

[2bndy5]

@Alexolut What happens if you change it to this?

lines-changed-only: diff

Admittedly, this is not be ideal for most cases. I'm just trying to figure out if clang-format is reporting suggestions at all. If not, then we know it is indeed a problem with the specified line range and not our suggestion filtering.

[Alexolut]

With lines-changed-only: diff it works fine (as I already stated in initial message, probably you didn't notice that). With diff mode we just see extended line ranges, something like -3,+3 from actual changes. That will simply change the option to clang-format from --lines=42:42 into --lines=39:45 or such.

What I don't understand yet, that as far as I see there's still visible changes in output xml from clang-format even for --lines=42:42 case. So looks like clang-format works well here. And there's no reason to extend the range on cpp-linter side.

[2bndy5]

With lines-changed-only: diff it works fine (as I already stated in initial message, probably you didn't notice that).

Sorry, I did miss that.

What I don't understand yet, that as far as I see there's still visible changes in output xml from clang-format even for --lines=42:42 case. So looks like clang-format works well here. And there's no reason to extend the range on cpp-linter side.

That leads to my second question: If there is output from clang-format, does the suggested fix (per XML output) fit within the single changed line? If the suggested change about the single line does not within the single line, then it is likely that cpp-linter is filtering out the suggestion (see cpp-linter's parsing XML function).

As I was typing that out, I may have found the culprit in the XML parsing function.
Some proof using a Python REPL:

>>> True if 42 in range(42, 42) else False
False
>>> True if 42 in range(42, 43) else False
True

So, the range() function is not all inclusive; the range-end is exclusive. But the range of lines given to clang-format is (or at least expected to be) all inclusive.

[Alexolut]

If there is output from clang-format, does the suggested fix (per XML output) fit within the single changed line?

As far as I see, yes. --output-replacements-xml --lines=2:2 correctly detects formatting issues. E.g.:

<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='67' length='2'> </replacement>
</replacements>

I.e. replacements list is not empty. So I see no problems from clang-format side in such situations.

[Alexolut]

I see that @2bndy5 merged changes into cpp-linter main branch. What the normal procedure to receive recent fix in cpp-linter-action?

[shenxianpeng]

I’ve created #420 and am currently waiting for @2bndy5 to provide final approval and merge it. Once that’s done, we'll cut a new release.