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-3725] Preventing a crash in Flow due to unattributed ASTs (cleared nerrors prevents post attr to run in some cases). #7153

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Mar 13, 2024

javac processes code in several phases, and one of the phase is "analyze" which looks into method bodies, attaches attributes to the AST nodes and looks for various errors in the method bodies. It consists of two sub-phases, Attr and Flow.

It sometimes may happen that Attr leaves some fields in the AST unfilled when the code is erroneous. In the normal way javac works, this is not problematic, as if there's a compile-time error reported during Attr, Flow does not run. But, when running inside NetBeans, we push javac to Flow regardless of any errors anywhere in the compilation pipeline. To facilitate that, javac will fill various error-like types/symbols on the AST after Attr, if it continues with Flow, and if error was reported.

Up to this point, everything works reasonably.

But, the current code in NetBeans is clearing the number of reported errors, and then javac cannot detect that an error was reported, and fill not fill the AST with the error-like attributes. And this then may lead to a crash in Flow.

I believe the clearing of nerrors was needed to force javac to continue processing even after an error was detected, before the introduction of the current -Dshould-stop.at=FLOW approach. But, I don't think it is needed (on most places) currently, and is actually harmful, as described above.

So, this patch removes the nerrors = 0 on all places I could find except in VanillaCompileWorker. The situation in the VanillaCompileWorker is different - we only clear nerrors after we fix (or at least try to) the AST to not contain errors.

…cleared nerrors prevents post attr to run in some cases).
@lahodaj lahodaj added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) VSCode Extension [ci] enable VSCode Extension tests labels Mar 13, 2024
@lahodaj lahodaj added this to the NB22 milestone Mar 13, 2024
@lahodaj lahodaj requested a review from dbalek March 13, 2024 10:56
@mbien mbien linked an issue Mar 13, 2024 that may be closed by this pull request
@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Mar 13, 2024
@apache apache locked and limited conversation to collaborators Mar 13, 2024
@apache apache unlocked this conversation Mar 13, 2024
@mbien mbien linked an issue Mar 13, 2024 that may be closed by this pull request
@apache apache locked and limited conversation to collaborators Apr 10, 2024
@apache apache unlocked this conversation Apr 10, 2024
@cstamas
Copy link
Member

cstamas commented Apr 10, 2024

👍 tested locally: 21 got me NPEs, this PR made it work (re Find usages and Call Hierarchy)

@mbien
Copy link
Member

mbien commented Apr 10, 2024

i think we should merge this soon since it looks like it is fixing at least part of the problem (see #3725 (comment)).

This would close many "NPE in javac" issues and hopefully make it easier to track down the remaining problem.

@ebarboni ebarboni merged commit 46c7f63 into apache:master Apr 11, 2024
122 checks passed
@ebarboni
Copy link
Contributor

merged as tested + approved

@BlueGoliath
Copy link

This PR has not fixed class or method renaming, despite claims made on the mailing list. Now Netbeans 22 RC 3 just fails silently, leaving projects in a broken state. There is no difference in the pass/fail before this PR and after as far as I can tell.

@mbien mbien linked an issue May 25, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment