Error appending replacements in multi-line comments

[florian-kuebler]

With #38, suggestions including multiple replacements where tried to be tackled. However, right now, replacements text is only concatenated and the length is being summed, which is a bit too simple, resulting in potentially unusable comments.

Consider the following example:

Diagnostics:
- BuildDirectory: /home/runner/work/orbitprofiler/orbitprofiler/build/src/DataViews
  DiagnosticMessage:
    FileOffset: 1269
    FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
    Message: invalid case style for local variable 'resultingColumns_'
    Replacements:
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1269
      ReplacementText: resulting_columns
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1292
      ReplacementText: resulting_columns
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1335
      ReplacementText: resulting_columns
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1420
      ReplacementText: resulting_columns
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1503
      ReplacementText: resulting_columns
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1581
      ReplacementText: resulting_columns
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1663
      ReplacementText: resulting_columns
    - FilePath: /home/runner/work/orbitprofiler/orbitprofiler/src/DataViews/CallstackDataView.cpp
      Length: 17
      Offset: 1762
      ReplacementText: resulting_columns
  DiagnosticName: readability-identifier-naming
  Level: Warning
MainSourceFile: ''

[platisd]

😱 Crap, I'll take a look. Thanks for reporting it. Please feel free to use an earlier version until a fix is found. 🙏

[florian-kuebler]

Thinking about this a bit more, I am not even sure if multiline atomic suggestion comments are possible. Clang-tidy's replacements may span over multiple files (in particular in the example of renaming). Maybe #15 would needed to be reconsidered to support atomic suggestions.

[platisd]

I haven't had time to fix this yet, but I was thinking of trying out the following logic.
Let's take this as an example of a good use-case for merging comments:

    Replacements:
    - FilePath: /home/me/projects/cpp-command-parser/include/CommandParser.h
      Length: 5
      Offset: 8851
      ReplacementText: ''
    - FilePath: /home/me/projects/cpp-command-parser/include/CommandParser.h
      Length: 47
      Offset: 8856
      ReplacementText: "            return sizeof...(Args);\n       "

The main difference compared to the one you posted above is that they are "consecutive", i.e. if you sum the length and the offset, you end up in the next offset (or the next offset -1 in another case).

So I am hoping that we can distinguish between the two cases and support both? 🤔

What do you mean by "atomic" suggestion comments btw?

[florian-kuebler]

Thanks for the example.

Right, in this case it totally makes sense to have a single suggestion. So these consecutive changes should be easy to find, as Offset0+Lenght0 == Offset1. So if you ensure the replacements are sorted by offset, you should be able to merge them with one iteration.

By atomic suggestion, I meant a suggestion comment that contains multiple changes, but the changes only make sense together. E.g. in the renaming case, ideally you want to "apply the suggestion" for declaration, definition and all usages at the same time.
But also in your example, it only makes sense to have "one" comment.

[platisd]

ideally you want to "apply the suggestion" for declaration, definition and all usages at the same time

Yeah, I've been looking for that feature... 😅
If this is what the Checks API allows then I might look into it again. 😁

[florian-kuebler]

I am actually not very familiar with with Checks API, but as you can trigger webhooks with it, it should be possible to archive what we were hoping for.

But anyways. I think for a first step, it would be fine to only merge the "consecutive" replacements together.

[jellespijker]

Hi @platisd we're also experiencing this problem, I will revert back to the commit before #38 for now. Let me know if we can help out with testing or something else

[platisd]

@jellespijker Can you please post your yaml report here as well?

[platisd]

@florian-kuebler and @jellespijker could you please try out this fix branch: platisd/clang-tidy-pr-comments@fix_non_consecutive_bug

Thanks! 🙏

[platisd]

I am confident #43 is an improvement over master so I went ahead and merged it.

[florian-kuebler]

I currently have #43 running with my example, but am also confident that it will be fixed by the change.

Still, #43 would not catch the case where there are consecutive and non-consecutive changes in a row. But, I am also not sure if this can actually happen.

[florian-kuebler]

So yes, #43 fixed the case mentioned above. I am going to close the issue. As mentioned above, I think there is still room for improvement, but that would be a different issue, I guess. Let's see how current master works for us.

Thanks for fixing this.

[platisd]

Still, #43 would not catch the case where there are consecutive and non-consecutive changes in a row. But, I am also not sure if this can actually happen.

No idea either. 😅
The documentation on the format is very scarce, basically the best source is the related parts of clang-tidy's source code that I found somewhere. 😅

I am planning to add some tests for this Action so hopefully we will avoid regressions in the future. 🤞