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

[NETBEANS-7981] Handling Diagnostics with position -1 while writing error/warning index. #7983

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Nov 23, 2024

When code like:

package javaapplication2;

public class JavaApplication2 {

    public static void main(String[] args) {
        new G() {
            
        };
    }
    
}

class G {
    @Deprecated
    G() {}
}

is indexed with -Xlint:deprecation, writing error/warning index may fail due to an exception like:

java.lang.ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 605
	at com.sun.tools.javac.util.Position$LineTabMapImpl.getColumnNumber(Position.java:266)
	at com.sun.tools.javac.util.Position$LineMapImpl.getColumnNumber(Position.java:236)
	at com.sun.tools.javac.util.Position$LineTabMapImpl.getColumnNumber(Position.java:253)
	at org.netbeans.modules.java.source.indexing.JavaCustomIndexer$ErrorConvertorImpl.getRange(JavaCustomIndexer.java:1329)

This is because the Diagnostics has end position -1. This patch simply proposes to treat this case as a missing end position.

Separately, it should be investigated if javac should produce errors or warnings like this.

@lahodaj lahodaj added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests labels Nov 23, 2024
@lahodaj lahodaj requested review from dbalek and mbien November 23, 2024 16:49
@mbien mbien added this to the NB24 milestone Nov 23, 2024
@mbien mbien linked an issue Nov 23, 2024 that may be closed by this pull request
@matthiasblaesing
Copy link
Contributor

The warnings that cause this for the sample case @mbien described are:

"NodesAnnotationProcessor.java:119: Warnung: [deprecation] SimpleAnnotationValueVisitor6() in javax.lang.model.util.SimpleAnnotationValueVisitor6 ist veraltet
                    String clsName = type.accept(new SimpleAnnotationValueVisitor6<String, Object>() {
                                                                                                     ^"
                                                                                                     
"NodesAnnotationProcessor.java:119: Warnung: [deprecation] SimpleAnnotationValueVisitor6() in javax.lang.model.util.SimpleAnnotationValueVisitor6 ist veraltet
                    String clsName = type.accept(new SimpleAnnotationValueVisitor6<String, Object>() {
                                                                                                     ^"

"NBClassReader.java:123: Warnung: [unchecked] Nicht geprüfter Aufruf von ForwardingJavaFileObject(F) als Mitglied des Raw-Typs javax.tools.ForwardingJavaFileObject
                            c.classfile = new ForwardingJavaFileObject(origFile) {
                                                                                 ^"

"JavaHintsAnnotationProcessor.java:142: Warnung: [deprecation] ElementScanner6() in javax.lang.model.util.ElementScanner6 ist veraltet
            new ElementScanner6<Void, Void>() {
                                              ^"

"TestBase.java:409: Warnung: [deprecation] MIMEResolver() in org.openide.filesystems.MIMEResolver ist veraltet
    static class MIMEResolverImpl extends MIMEResolver {
           ^"

For these cases the new code path is hit, so from my POV this fixes the issue. Thank you.

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the file location to the message might help in future to track problems down:

diff --git a/ide/parsing.indexing/src/org/netbeans/modules/parsing/impl/indexing/errors/TaskCache.java b/ide/parsing.indexing/src/org/netbeans/modules/parsing/impl/indexing/errors/TaskCache.java
index 08a415f..95d128a 100644
--- a/ide/parsing.indexing/src/org/netbeans/modules/parsing/impl/indexing/errors/TaskCache.java
+++ b/ide/parsing.indexing/src/org/netbeans/modules/parsing/impl/indexing/errors/TaskCache.java
@@ -229,7 +229,7 @@
                 }
             });
         } catch (IOException ex) {
-            Exceptions.printStackTrace(ex);
+            LOG.log(Level.SEVERE, "can't dump errors for: " + String.valueOf(i), ex);
         }
     }

It might be better to wrap + rethrow as runtime exception and let the indexer handle it - but this would go beyond a hotfix.

Copy link
Contributor

@dbalek dbalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@ebarboni
Copy link
Contributor

I merge as it is ? and @mbien patch is for NB25 ?

@mbien
Copy link
Member

mbien commented Nov 25, 2024

I merge as it is ? and @mbien patch is for NB25 ?

@ebarboni I think it would be good to have better logging there. Since the exception does not tell where the problem is. In my case: i had probably about 100 projects open and had to narrow it down first. A good log line would point to the right place right away.

I suppose I could a) open a second PR or b) add a commit to this PR? cc @lahodaj @matthiasblaesing

@mbien mbien added the Regression This used to work! label Nov 25, 2024
@matthiasblaesing
Copy link
Contributor

I think this is good to go as is. The exception backtrace is a 100% match into the right file. While I agree, that knowing the problematic file, this is IMHO not really needed for this hotfix.

@mbien
Copy link
Member

mbien commented Nov 25, 2024

I think this is good to go as is. The exception backtrace is a 100% match into the right file. While I agree, that knowing the problematic file, this is IMHO not really needed for this hotfix.

the exception trace won't contain the file path which is causing the exception. To clarify the diff: the i is the Indexable method parameter. i.toString() would provide the path - this is no index or anything like that.

@matthiasblaesing
Copy link
Contributor

My focus is to get a new VC fast. We can discuss this longer, but from my perspective we gain little. I don't care either way. The fix is here and is mergeable, everything else takes more time.

@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 25, 2024

I've tweaked the code to attach the message to the exception.

@ebarboni ebarboni merged commit 0511bca into apache:delivery Nov 26, 2024
37 checks passed
@ebarboni
Copy link
Contributor

ebarboni commented Nov 26, 2024

rc4 online

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests Regression This used to work! VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArrayIndexOutOfBoundsException in JavaCustomIndexer$ErrorConvertorImpl
5 participants