log() method incorrectly calculates the column number when Tabs are used

[Zopsss]

I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

Detected at: #14643 (comment)

I was working on the JavadocLeadingAsteriskAlignCheck. This new module checks if the leading asterisk is aligned properly in the javadoc comment or not. I had written a test case in which leading asterisks were preceded with Tabs. The Check is working fine, I used the getTabWidth() to get correct the tab size and calculate the column number. The Check is passing the correct column number of the leading asterisk to the log() method, so I expected all the test cases to pass. But the test which uses tabs was showing incorrect violations in its result.

This is my input file: InputJavadocLeadingAsteriskAlignTabs.java.
This is my test method: testTabs().

Below I have provided the Snapshot of the test case with debugger:

As you can see in the right side of the Snapshot, on line 59 there is a violation because the leading asterisk is incorrectly aligned on column number 5, it should be on column number 6 to make it aligned correctly. On the left side, it is visible that the log() method is receiving the asterisk's current column number which is 5 ( asteriskColumn ). Below is the test result:

Violations for C:\Open Source\checkstyle\src\test\resources\com\puppycrawl\tools\checkstyle\checks\javadoc\javadocleadingasteriskalign\InputJavadocLeadingAsteriskAlignTabs.java differ.
missing (5)
#1      : 49:5: Leading asterisk has incorrect indentation, should be 4.
#2      : 59:5: Leading asterisk has incorrect indentation, should be 6.
#3      : 60:7: Leading asterisk has incorrect indentation, should be 6.
#4      : 69:7: Leading asterisk has incorrect indentation, should be 6.
#5      : 70:5: Leading asterisk has incorrect indentation, should be 6.

unexpected (5)
#1      : 49:7: Leading asterisk has incorrect indentation, should be 4.
#2      : 59:8: Leading asterisk has incorrect indentation, should be 6.
#3      : 60:10: Leading asterisk has incorrect indentation, should be 6.
#4      : 69:11: Leading asterisk has incorrect indentation, should be 6.
#5      : 70:8: Leading asterisk has incorrect indentation, should be 6.

In the unexpected section of the test result, for line 59 it is expecting the asterisk's column number to be 8 but the actual column number passed to the log() method was 5. The test case always shows incorrect column number in the unexpected section when Tabs are used even though the log() method is receiving the correct column numbers. After debugging the log() method, I found that the lengthExpandedTabs() method is calculating the column incorrectly that's why the test result shows incorrect column numbers. This is the same reason for other violations too.

---

Describe what you expect in detail.
The test result is showing the incorrect column number in the unexpected section, the leading asterisk is not located at the column number that it is showing. So the lengthExpandedTabs() should be able to calculate the Tabs properly.

[rnveach]

#14643 (comment)

let’s reproduce it with an existing check

In addition, see https://checkstyle.org/report_issue.html#How_to_report_a_bug.3F .
Please do not post with pictures, it makes it hard to read and copy for reproduce testing.

[rnveach]

When a case is provided, it will have to be examined if this issue is related to #14780 as both are javadocs.

[MANISH-K-07]

When a case is provided, it will have to be examined if this issue is related to #14780 as both are javadocs.

@rnveach , your reference is accurate.
This issue reported here is an after effect of what we found of column number mismatch at #14780
It is getting reflected probably as we log violations by AST parsing. It makes me wonder where else we have been missing 😕

[rnveach]

If this is a duplicate of #14780 then we need to close this issue.

I can't argue against closing as too few details are given in this issue. #14780 isn't truly about tabs, so I don't see how we are calculating tabs incorrectly. All our AbstractCheck log methods use tab conversion code so you could show a case with nearly all our checks if one existed.

If the method is expecting 1-base and we are giving it 0-base then that is the issue and that is how it connects. However, looking at that method's code it does appear to be 0-based as well. So I am not seeing the connection yet.

[rnveach]

This may be the issue, which is unrelated to #14780 .

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/utils/CommonUtil.java

Line 209 in 05981e0

len = (len / tabWidth + 1) * tabWidth;

Shouldn't this be len += tabWidth? I am not understanding where this division is coming from.

It looks like it is saying tabs go up to a certain position and then stop. Meaning 2 spaces plus a tab stop at the same position as a tab alone. That does make sense as I have seen some tools do this.

Javadoc:

Each tab is counted as the number of characters is takes to jump to the next tab stop.

@Zopsss Please try to create a case.

[MANISH-K-07]

#14780 isn't truly about tabs, so I don't see how we are calculating tabs incorrectly

@rnveach , yep sorry, my bad.
The snapshots were hard to read and I misread '8' as '0', hence got bit confused :)

[Zopsss]

Hey @rnveach Thanks for reviewing the issue. I have fixed the failing test case mentioned in this issue. I'll explain in detail why I opened this issue and what problem I was facing. I'll reply to your questions soon as it is already mid night here. Thanks again for showing your interest in this issue.

[Zopsss]

I have updated the issue description explaining the problem I was facing in a more precise way

---

I was passing the column number of leading asterisk with expanded tabs to the log() method. The log() method internally calls lengthExpandedTabs() which calculates the column number at a specific position of the provided string by expanding the tabs.

So I was already passing the asterisk's column number with the expanded tabs to the log() method as mentioned here:

log() method is receiving the asterisk's current column number

Here by current column number I meant the column number of the leading asterisk with expanded tabs.

At that time I wasn't sure what is the use of the lengthExpandedTabs() because I didn't knew the term expanding tabs in string. The log() was receiving the column number of the leading asterisk after expanding the tabs, this column number was being passed to the lengthExpandedTabs(), so it was receiving the wrong column number because I was passing the column number with tabs already expanded while the lengthExpandedTabs() method expects to receive the original column number ( without expanded tabs ), that's why it was calculating the column number incorrectly.

So to fix this, I modified the Check implementation to pass the column number of the leading asterisk without tabs expanded to the log() method. Now that the log() method is receiving the column number without tabs expanded, it gets passed to the lengthExpandedTabs() method and it calculates the column number by expanding tabs and everything works fine.

The lengthExpandedTabs() method is working fine, I was just passing the wrong column number to it that's why it was calculating the incorrect column number. All the problems I was facing has been resolved, I will close this issue now. And sorry for the delay I was busy with college stuff so it took a little longer to reply.