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.

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

Java Grammar: Hard to understand message #4632

Closed
denizarsan opened this issue Jul 6, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@denizarsan
Copy link

commented Jul 6, 2017

$ cat C.java # the code does not compile

class C extends D<${package}.E> {}

$ cat config.xml

<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="LineLength"/> <!-- can be any check that requires parsing -->
  </module>
</module>

$ java -jar checkstyle-8.0-all.jar -c config.xml C.java

Starting audit...
 areLtsAndGtsBalanced(currentLtLevel)
Audit done.

The source file does not compile, so giving a warning/error message is fine, but the message is hard to understand.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

I can reproduce the message and it is not very informative. It doesn't even look like an error, but is most likely a parsing error as file is uncompilable.
We should display some type of error about failing to parse like javadoc, or a stack trace like #4633 .
@romani You agree?

@romani

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

We can not detect non compiled sources at all. We could even parse without problems some of them, we could parse them but Checks could fail.
There are a lot of uncertainty could happen if source is not compilable.
That is why it is our limitation to work only on compiled sources.

I am closing issue , but discussion could continue

@romani romani closed this Jul 14, 2017

@rnveach

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

@romani The main issue is that CS isn't throwing this as an error or violation. Except for that one display, TreeWalker assumes the file was parsed correctly even though the AST being returned is null. We have places with null protection on the tree being returned to not throw an NPE. When is null every valid?
If cache was enabled, you would only see the output once as it would add the file to the cache since there is no violation.

This is the exception being produced in master:

 areLtsAndGtsBalanced(currentLtLevel)
	at com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer.typeArguments(GeneratedJavaRecognizer.java:1086)
	at com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer.classOrInterfaceType(GeneratedJavaRecognizer.java:958)
	at com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer.superClassClause(GeneratedJavaRecognizer.java:2425)
	at com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer.classDefinition(GeneratedJavaRecognizer.java:630)
	at com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer.typeDefinitionInternal(GeneratedJavaRecognizer.java:556)
	at com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer.typeDefinition(GeneratedJavaRecognizer.java:389)
	at com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer.compilationUnit(GeneratedJavaRecognizer.java:202)
	at com.puppycrawl.tools.checkstyle.TreeWalker.parse(TreeWalker.java:445)

However, typeDefinition catches this exception and only does a simple print to System.err via reportError(ex) and than continues on by returning null.

We should look into not hiding the failure.

@romani romani reopened this Jul 14, 2017

@romani

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

yes, lets review this case, but expected behavior is any wide exception or message, it still could be unclear to user, but we should not hide details.

@romani romani added the approved label Jul 14, 2017

@rnveach rnveach changed the title Hard to understand message Java Grammar: Hard to understand message Mar 18, 2018

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 10, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 10, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 16, 2019

rnveach added a commit that referenced this issue Mar 16, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Fix was merged

@rnveach rnveach closed this Mar 16, 2019

@rnveach rnveach added this to the 8.19 milestone Mar 16, 2019

Vantuz added a commit to Vantuz/checkstyle that referenced this issue Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.