apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.61k stars 836 forks source link

Declarative hint variable seems to capture too much #7078

Closed mbien closed 4 months ago

mbien commented 5 months ago

Apache NetBeans version

Apache NetBeans 21 (not a regression)

What happened

$mods$ $type $name = new $T();

does not capture this line correctly:

final List<Integer> copy2 = new ArrayList<>();

the $name variable will contain the whole line, all other variables seem to be correct, see reproducer.

Language / Project Type / NetBeans Component

java declarative hints / jackpot

How to reproduce

    public static void main(String[] args) {
        final List<Integer> copy2 = new ArrayList<>();
    }

hints debug file:

"mods test":
$mods$ $type $name = new $T();
=>
$mods$
;;

"type test":
$mods$ $type $name = new $T();
=>
$type
;;

"name test":
$mods$ $type $name = new $T();
=>
$name
;;

"constructor test":
$mods$ $type $name = new $T();
=>
$T
;;

all rules match, however, the "name test" rule will have the entire statement in it instead of just copy2

image

Did this work correctly in an earlier version?

No / Don't know

Operating System

linux

JDK

JDK 21

Apache NetBeans packaging

Own source build

Anything else

i couldn't find the cause, but at least I found two other issues I could fix while investigating this #7075 + #7077

Are you willing to submit a pull request?

Yes

mbien commented 5 months ago

@lahodaj this shouldn't be the case, right? I tried to find the cause but gave up deep in the bulk search code.

What I have learned though is that the rule is split into two parts at some point.

btw I noticed this while trying to implement:

$mods$ $type $name = new $T();
$name.addAll($other);
=>
$mods$ $type $name = new $T($other);
;;

this did only work when $mods$ was removed.

mbien commented 5 months ago

what if the TreePath is correct but the toString() of its leaf is not?

the tree of $name is a VariableTree, getName() would return the identifier. We could fix this via https://github.com/apache/netbeans/pull/7075 too. Going to add a commit there. This might be just a UI problem, jackpot itself is correct here?

lahodaj commented 5 months ago

When matching, the variable will be put into both variables and variables2Name, to keep other stuff (like conditions) working. I suspect we need to keep this, and this is more a problem on the "rewrite" side than the matching side.

A part of what you see is indeed just a UI problem. But not all. This:

"name test":
$mods$ $type $name = new $T();
=>
$name
;;

does not make much sense, as $name is an expression, but we need a statement, and this will crash.

Correcting this to:

"name test":
$mods$ $type $name = new $T();
=>
$name;
;;

leads to an infinite loop, which is pretty bad (but, frankly, does not make much sense either).

But in if one does e.g.:

"name test":
$mods$ $type $name = new $T();
=>
Object o = $name;
;;

then it works (and might make sense), its just the text that's wrong.

I need to look that's going on in the full case with $mods$ - that should work, but maybe there's some interaction between multi-statements and modifiers.

mbien commented 5 months ago

right I think it is two things we see here: 1) the UI code which builds the "rewrite to" string was wrong (it had multiple issues). I didn't know that at first and assumed that something was wrong in jackopt itself, so I tried to fix a problem which wasn't there and looked at the wrong code 2) there is still likely something wrong with $mods$ since some rules don't seem to match when they probably should

regarding 1): taking your example:

"name test":
$mods$ $type $name = new $T();
=>
Object o = $name;
;;

NB 21: image

https://github.com/apache/netbeans/pull/7075 applied: image

regarding 2): this turns out to be a separate issue and is beyond UI code, sample snippet: https://github.com/apache/netbeans/issues/7078#issuecomment-1950293375