prevent NPE when clazz.type field is null

[homberghp]

closes #8699

---

^Add meaningful description above

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

• are all your own work, and you have the right to contribute them.
• are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

1. Was this PR correctly labeled, did the right tests run? When did they run?
2. Is this PR squashed?
3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

[neilcsmith-net]

Please don't open issues if you have a pull request ready, and please add the issue description to the pull request. The pull request is linked from release notes.

Minor nit on the formatting - should be a space between != and null.

[mbien]

as stated on a similar PR. We shouldn't patch NPEs like this without a reproducer. The same snippet is used across the file (type has 9 usages), if null is a valid value it will just fail somewhere else.

[matthiasblaesing]

I think VanillaCompileWorker needs more null checks. The problem with the "this needs a reproducer" is, that reproduction of these situations is difficult, as it might be a mix of "source in state of editing", incomplete indexing, $other reason.

If there are other patterns like the problematic one, then sure, lets also guard them, but also lets be real, that not all cases can be caught. I too often see exception from the javac infrastructure and that hast to stop, if every change is vetoed by "needs a reproducer", even when the reporter clearly says, that he can't, this will always be a problem.

[mbien]

i am looking into this right now (unless someone else wants) and check other usages.

if there is no way to get a reproducer, sure I would be ok with adding guards as best effort attempt. But given that the issue mentions under reproducer "open a module", my assumption was that this is somehow verifiable.

There is also the risk that things like this hide an issue which would surface in a less direct way later.

I do also maintain a small collection of reproducers here https://github.com/mbien/nb-reprorepo/tree/master/tinybugs/src/main/java/bugs for anyone interested.

[neilcsmith-net]

I too often see exception from the javac infrastructure and that hast to stop.

Agreed! There is also a question of where this is being reported from and why. I think we're possibly reporting exceptions too often in the UI where they should just be caught at a higher level and logged as parsing errors.

[mbien]

worth noting that the issue mentions NB 26 which runs on a different javac major version

I re-ran the indexer smoke test against NB 27rc2 and it did not encounter this exception

(it did find two javac bugs by now though JDK-8361445+JDK-8361570 and #8638 + #8702)

[homberghp]

Making an Issue AND a PR at almost the same time allows me to deal with the naming problem.
The problem description could have been clearer/completer.
However, the way the problem (the error bulb) presents itself means you either create an issue and then work on the solution, which appears to be hard, because the errors tend to disappear when you handled (opened, looked at the details).

If the purpose of the error bulb is to invite reporting or to prompt solving the issue, then that is what I did.
If not, then change the uncaught handler to suppress errors.

[lahodaj]

My opinion: it is better to make an effort to find reproducers, and look what exactly is happening, than just adding null checks without trying. If we can't, with a reasonable effort, find a reproducer, then it might be OK to just add a null check, but we should be aware it is not unlikely to surface somewhere else. There are (at least) two reasons:

• the null value often means that some part of the attribution didn't happen even though it should have. The null check will prevent the NPE, but will not make all the other features that rely on properly attributed AST to work. Sometimes, there's not much that can be improved, other times, things can be improved.
• if there's no test, there's a possibility that the problem will resurface. Not that likely for a null check, but may happen.