Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Solving parsing errors for missing description in block tags #13061

Closed
0xbakry opened this issue May 13, 2023 · 6 comments
Closed

Solving parsing errors for missing description in block tags #13061

0xbakry opened this issue May 13, 2023 · 6 comments
Labels
approved bug
Milestone
10.20.2

Comments

@0xbakry
Copy link
Contributor

0xbakry commented May 13, 2023

Base on discussion at #13043

We need to update grammar for @exception, @throws, @param so their arguments can be empty.

current grammar for exception and param tags:
https://github.com/checkstyle/checkstyle/blob/bd3c64badd4f1f7d58261af8dae9c40f9bf672b5/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/javadoc/JavadocParser.g4#LL1144C1-L1148C56

EXCEPTION_LITERAL (WS | NEWLINE | {!isNextJavadocTag()}? LEADING_ASTERISK)+ CLASS_NAME (WS | NEWLINE)* ((WS | NEWLINE) description)?

PARAM_LITERAL (WS | NEWLINE | {!isNextJavadocTag()}? LEADING_ASTERISK)+ PARAMETER_NAME (WS | NEWLINE)* ((WS | NEWLINE) description)?

current for throw tag:
https://github.com/checkstyle/checkstyle/blob/bd3c64badd4f1f7d58261af8dae9c40f9bf672b5/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/javadoc/JavadocParser.g4#LL1165C1-L1167C1

THROWS_LITERAL (WS | NEWLINE | {!isNextJavadocTag()}? LEADING_ASTERISK)+ CLASS_NAME (WS | NEWLINE)* ((WS | NEWLINE) description)?

CLI

$ cat config1.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name = "Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
       <module name="NonEmptyAtclauseDescription"/>
    </module>

</module>

$ cat Test1.java
public class Test1 {

  /**
   * @param
   */
  public void method(String a)
  {

  }
}

$ java -jar checkstyle-10.9.3-all.jar -c config1.xml Test1.java
Starting audit...
[ERROR] E:\Parse\Test1.java:6: Javadoc comment at column 3 has parse error. Details: no viable alternative at input '<EOF>' while parsing JAVADOC_TAG [NonEmptyAtclauseDescription]
Audit done.
Checkstyle ends with 1 errors.

Excepted:

Parser should be able to finish parsing without parser error.
@romani
Copy link
Member

romani commented May 13, 2023

I am ok to ease parser as, no much people knows java doc syntax and we have to work after javac is passed but javadoc tool not executed yet.
But javadoc tool suppose to put error on this java doc.

@nrmancuso
Copy link
Member

Few things here:

  1. As we do javac for issue reporting for java code checks, we should show:
    a. javadoc tool output
    b. image/link to generated Javadoc
  2. We need to be careful "easing the parser", as this will result in further ambiguity. Performance/overhead in Javadoc parser/lexer grammar is not good, we need to keep an eye on this.
  3. If javadoc tool places error (not warning) on a Javadoc, we should not be expected to be able to parse it.

@romani
Copy link
Member

romani commented May 16, 2023

If javadoc tool places error (not warning) on a Javadoc, we should not be expected to be able to parse it.

in general, yes. But lets imagine real life example of checkstyle usage.
You have IDE, you did update to method signature to add parameter, save/compile, checkstyle complains on no parameter in javadoc, you add it without description. Save/compile. Checkstyle violate javadoc as not parse able! It will take a while for user to catch a problem.

problem is that javadoc tool is almost never executed in dev cycle, as it is mostly for "assemble artifact" phase of build. Usually highly ignorable violations.

if we ease a bit strictness of our parser, at least for cases what description might be absent. We at least not throw in user obscure error message.

If we want to stay strict, we need to make error message to be clear for user, that will be another challenge.

@nrmancuso
Copy link
Member

If we want to stay strict, we need to make error message to be clear for user, that will be another challenge.

Yes. Our error messages should be consistent. Our Javadoc parsing errors appear as check errors as opposed to normal checkstyle exception message when parsing Java code. If you agree we can open a new issue on this.

@romani
Copy link
Member

romani commented May 17, 2023

We can open issue on this, but I do not see easy way to make it, we don't have good parser error messages even in java parser.
Before we make good to read messages, we can relax parser a bit and restore strict parsing as we have good messages.

0xbakry added a commit to 0xbakry/checkstyle that referenced this issue Apr 16, 2024
@0xbakry
Issue checkstyle#13061: Update grammar for @exception, @throws, and @…
cb6f9ce 
…param tags
@0xbakry 0xbakry mentioned this issue Apr 16, 2024
Merged
0xbakry added a commit to 0xbakry/checkstyle that referenced this issue Apr 16, 2024
@0xbakry
Issue checkstyle#13061: Update grammar for exception, throws, and par…
814f652 
…am tags
0xbakry added a commit to 0xbakry/checkstyle that referenced this issue Apr 17, 2024
@0xbakry
Issue checkstyle#13061: Update grammar for exception, throws, and par…
113f78b 
…am tags
0xbakry added a commit to 0xbakry/checkstyle that referenced this issue Apr 17, 2024
@0xbakry
Issue checkstyle#13061: Update grammar for exception, throws, and par…
d052b98 
…am tags
0xbakry added a commit to 0xbakry/checkstyle that referenced this issue Apr 17, 2024
@0xbakry
Issue checkstyle#13061: Update grammar for exception, throws, and par…
c341c2d 
…am tags
0xbakry added a commit to 0xbakry/checkstyle that referenced this issue Apr 18, 2024
@0xbakry
Issue checkstyle#13061: Update grammar for exception, throws, and par…
582f327 
…am tags
0xbakry added a commit to 0xbakry/checkstyle that referenced this issue Apr 23, 2024
@0xbakry
Issue checkstyle#13061: Update grammar for exception, throws, and par…
3c823e6 
…am tags
romani pushed a commit to 0xbakry/checkstyle that referenced this issue Nov 12, 2024
@0xbakry @romani
Issue checkstyle#13061: Update grammar for exception, throws, and par…
3ee975d 
…am tags
romani pushed a commit to 0xbakry/checkstyle that referenced this issue Nov 13, 2024
@0xbakry @romani
Issue checkstyle#13061: Update grammar for exception, throws, and par…
f89c40e 
…am tags
romani pushed a commit to romani/checkstyle that referenced this issue Nov 14, 2024
@0xbakry @romani
Issue checkstyle#13061: Update grammar for exception, throws, and par…
184fd50 
…am tags
@romani romani mentioned this issue Nov 14, 2024
Closed
romani pushed a commit that referenced this issue Nov 14, 2024
@0xbakry @romani
Issue #13061: Update grammar for exception, throws, and param tags
479d3a9
@romani romani added the bug label Nov 14, 2024
@github-actions github-actions bot added this to the 10.20.2 milestone Nov 14, 2024
@romani
Copy link
Member

romani commented Nov 14, 2024

Fix is merged

@romani romani closed this as completed Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
approved bug
Projects
None yet
Milestone
10.20.2
Development

No branches or pull requests
3 participants
@romani @nrmancuso @0xbakry