[jackpot] Attempt to make JackpotTrees more robust.

[mbien]

• JackpotTrees.createInstance() will match constructors directly - no filling up with null objects anymore
• extracted typesafe factory methods to keep the fragile code in one place and make it hopefully easier to update it in future

[mbien]

@jlahoda this should help with the issue in #3206 hopefully.

[matthiasblaesing]

I gave this a spin together with #3206 and #3286 and it seems to work. We have to expect more breakage to creep up, but I think this is a step into the right direction.

Before this is merged the if and for statements need to add braces. Otherwise the implementation looked sane to me.

[mbien]

Before this is merged the if and for statements need to add braces. Otherwise the implementation looked sane to me.

i am surprised about this requirement given the code styles i have seen so far. The lambda in question is fairly concise and should be easy to read. The indentation makes it difficult to misinterpret what is going there.

[mbien]

@jlahoda should I remove the logic which fills unknown params with null values? Current impl won't fill primitives but will still fill objects.

I was hesitant in doing this because I thought there might be some JDK version compatibility issue this was supposed to solve - so i didn't want to cause a regression.

[matthiasblaesing]

Before this is merged the if and for statements need to add braces. Otherwise the implementation looked sane to me.

i am surprised about this requirement given the code styles i have seen so far. The lambda in question is fairly concise and should be easy to read. The indentation makes it difficult to misinterpret what is going there.

My take on this: If I want indention based syntax, I go to python.

I had to look at it multiple times to see the right indentions and blocks. I looked through other classes in spi.java.hints and found only cases where the if is very trivial and none where nested.

This is what I had in mind:

matthiasblaesing@876c94f

IMHO lambdas get messy quickly if people put to much code in them and even more if braces are missing.

[mbien]

Before this is merged the if and for statements need to add braces. Otherwise the implementation looked sane to me.

i am surprised about this requirement given the code styles i have seen so far. The lambda in question is fairly concise and should be easy to read. The indentation makes it difficult to misinterpret what is going there.

My take on this: If I want indention based syntax, I go to python.

I didn't want to change the language just to fix this method :)

I had to look at it multiple times to see the right indentions and blocks. I looked through other classes in spi.java.hints and found only cases where the if is very trivial and none where nested.

I have seen files which looked like they tried intentionally to avoid any kinds of newlines. To achive that they re-assigning method params, inside other assignments or declarations. While loops without bodies, and i just replaced a for loop which used continue labels with this patch.

I don't actually mind having braces in that lambda i was just surprised to get that requirement, given the context.

IMHO lambdas get messy quickly if people put to much code in them and even more if braces are missing.

I can see that

[mbien]

but to bring this back to the more interesting questions:

• does this issue also occur without the auto-javac patch
  • if yes this should land in 12.6 if somehow possible otherwise NB is broken on the ubuntu JDK (maybe others too)
• should the method only allow direct constructor matches and remove the null filling entirely
  • could cause a regression if the reason for it was to solve another compatibility issue already

@jlahoda ping

[matthiasblaesing]

Just to make this formal - please add braces for if and for statements.

[matthiasblaesing]

For the question: Could it happen with NB 12.6? Yes it could - the conflicting constructors are there (disassembled from the referenced binary):

        protected JCVariableDecl(JCModifiers mods, Name name, JCExpression vartype, JCExpression init, Symbol.VarSymbol sym) {
            this(mods, name, vartype, init, sym, false);
        }

        protected JCVariableDecl(JCModifiers mods, Name name, JCExpression vartype, JCExpression init, Symbol.VarSymbol sym, boolean declaredUsingVar) {
            this.mods = mods;
            this.name = name;
            this.vartype = vartype;
            this.init = init;
            this.sym = sym;
            this.declaredUsingVar = declaredUsingVar;
        }

I don't see it though, but we don't know what influences the reported order of the constructors.

[mbien]

i did some archeological digging to check usages and call site compatibility to get a better picture if a direct constructor match would work. Two instances are created using JackpotTrees: JCCase and JCVariableDecl

// JDK 8+11
JCCase(JCExpression pat, List<JCStatement> stats) 
// JDK 12-18
JCCase(CaseKind caseKind, List<JCCaseLabel> labels, List<JCStatement> stats, JCTree body)

// JDK 8+11
JCVariableDecl(JCModifiers mods, Name name, JCExpression vartype, JCExpression init, VarSymbol sym)
JCVariableDecl(JCModifiers mods, JCExpression nameexpr,  JCExpression vartype)

// JDK ?-18
JCVariableDecl(JCModifiers mods,  Name name, JCExpression vartype, JCExpression init, VarSymbol sym)
JCVariableDecl(JCModifiers mods,  Name name, JCExpression vartype, JCExpression init, VarSymbol sym, boolean declaredUsingVar)
JCVariableDecl(JCModifiers mods, JCExpression nameexpr, JCExpression vartype)

Given the new knowledge i don't think the filling-params-with-null logic was of any help so far. There is also a bytebuddy code path selectively calling the JCCase(JCExpression pat, List<JCStatement> stats)constructor which doesn't exist anymore in modern JDKs, CaseKind is also deprecated for removal, so there will be another change in future.

I did the following:

• direct constructor matching, no variable guessing anymore
• added typesafe utility methods to JackpotTrees to keep the fragile code in one place, hopefully making it easier to update

lets see if the tests are ok with it.

[jlahoda]

Looks reasonable to me, thanks! (Seems the braces have been added.) Overall, I hope at some point we will only need to support ~1 version of the APIs/internal APIs, at which point we should be able to avoid all this reflection and class construction.

[matthiasblaesing]

@mbien thanks for working through this and @jlahoda thank you for the review. As the analysis showed, that the problem could happen with the nbjavac version currently targetted for 12.6, I think we should retarget this to delivery.

As delivery and master did not diverge that much, I suggest this approach (if you don't need the instructions, just ignore them):

• create a new branch 'happytrees2' from delivery git checkout -b github/happytrees2 (replace github with the name of your remote of the apache/netbeans repository)
• cherry pick both changes into it git cherry-pick 69b73908b47d7e844d743dd69d7a14bc5a3d16f7 6e1289dc5c7f65cc2a141e4b30f27ee8380905d0
• squash the two changes git rebase -i HEAD~2 (mark the second line as squash) save and create a new commit message for the final commit
• remove the old branch git branch -D happytrees
• rename the new branch git branch -m happytrees
• force push the new branch into the old PR git push -u -f origin happytrees
• update this PR with the right base branch (delivery)

After this you should only have a single commit in it.