This is a defect in the parser, not the check we just found it using a check execution, so we should add a test in javadoc ASTTest

@romani do you agree?

No, we covering all by testing of Checks and regular Inputs, no pure unit testing.
We add additional test that class if we think it helps.

@mohitsatr, please add one more test to that test class to visualize AST.

to visualize AST

I didn't quite get what you mean by that ?

@mohitsatr like I did in #16458

done

(WS | NEWLINE)* ((WS | NEWLINE) description)? seems repetative.
Any reason this isn't (WS | NEWLINE)* description?? The * should already handle multiple, so you don't need yo say an extra item of a multiple.

It is also a samilar pattern above.
It should be able to be reduced to (htmlElement | reference | STRING) (WS | NEWLINE)* description?

I see now, the (WS | NEWLINE) says one is required. While the other doesn't need a space between the html element. Still seems like there should be a way to reduce the repetative-ness.

(reference | STRING) ((WS | NEWLINE)+ description)? ?

(Assuming you meant to replace both lines and not the last one) Reference or STRING doesn't include htmlElement, so we can't drop that if the current code change (and original) is correct, and I don't doubt it.

We can't move (WS | NEWLINE) inside the parenthesis of the ?, because it will fail on things like STRING WS NEWLINE (assume complete match) when description is not present but everything else is.

description does include NEWLINE and WS (by virtue of text). You may be able to change ((WS | NEWLINE) description)? to just description? like the line above, but is best for regression to confirm.
Edit: but this could mean the description starts with a space/newline instead of the first non-whitespace text.

This grammar just seems excessively complex for the goal to be complex. The WS | NEWLINE (or similar) has gotten this grammar in trouble in the past since everything includes it, like I showed above with description.

Just another reason to put this grammar away and start over to see if it could be made leaner.

The WS | NEWLINE (or similar) has gotten this grammar in trouble in the past since everything includes it

I completely agree, without them the grammar will look a lot better.

Why are we interested in parsing WS or NEWLINE in the first place? If a check needs to enforce or use some whitespace rules, why not leave this responsibility to the check itself rather than the parser? This is how we handle Java code, whitespace is not explicitly parsed in the Java grammar, yet we have multiple checks that enforce whitespace rules.

Can't we just completely skip WS and NEWLINE?

If a check needs to enforce or use some whitespace rules, why not leave this responsibility to the check itself rather than the parser

My guess is the grammar was written to overcome Java's AST shortcomings in a way.

A check only sees what we provide it. For some reason we deemed java checks can only see the AST and not the source file anymore. This can cause some issues with whitespace (indentation check) and new lines as now we have to make complex logic on determining where newlines and spaces are because the AST does not include them. This made more complex by the fact our AST isn't in source order, so you have to do scans to determine what the source order next token even is before determining if there is a simple space/newline next. We also lose identification of what the whitespace (space, tab, etc...) and newlines (\r, \r\n, \n) actually is, going back to relying on the source code.

The fact that the Javadoc grammar is in source order may be a benefit to not needing to identify all WS/NEWLINE, but also if you remove all of them then things like descriptions may become a little weird to look at.

For example, this PR has TEXT -> . website of checkstyle and it isn't broken into 4 different TEXTs because of the whitespace. I think was the main reason the grammar keeps them in, so we could easily make chunks of text and we thought that would be easier to handle in checks.

It could be the Javadoc grammar is actually in a somewhat decent state (we still have some issues with excessive WS/NEWLINE IMO), it just looks more complex to those who are not fully familiar with it and is just not very new eyes friendly.

Unfortunately, due to how our Javadoc grammar is written, our lexer really only "scans" (which is typically only the first part of a lexer's job) in many cases. This means that we need to pass on this additional information (like newlines, whitespace) to the parser because it is also doing some of the work that the lexer normally does to recognize tokens.

it just looks more complex to those who are not fully familiar with it

couldn't agree more.

Honestly, there are a bunch of ways that we can achieve the goal of fixing the bug in the issue. While I think that @mahfouz72 's recommendation is reasonable, it is likely that the best performance can be achieved by reducing the original rule and avoiding extra alternatives:

@mahfouz72 can you help to analyze the difference in performance between your suggestion and this simplified version?

Looks like there is no much difference in performance. I tried several runs and my suggestion works slightly better.

Note: For this, I analyze performance on a single, small Javadoc so we may not capture many details.

Although your rule looks cleaner and has fewer alternatives, I’m unsure why it doesn’t improve performance.

The Javadoc used for testing

* Some summary.
*
* @param j some param
* @see Class  afdaf
* @see Classafdaf
* @see "ACADF" adfaf
* @see "adfaf"dafadf 
* @see <a href="www.checkstyle.org/">website</a>...
* @see <a href="www.checkstyle.org/">website</a> ...
* "some description"

@mahfouz72 I assume there was not any change in the tree itself?

All tests inside JavadocParseTreeTest are passing but we need regression on large projects also to be confident about this

Although your rule looks cleaner and has fewer alternatives, I’m unsure why it doesn’t improve performance.

Probably specificity :). It is ideal to write grammars to be as exact as possible in most cases. @mahfouz72 Thanks a lot for checking this out.