TODO: Fix NPE in BlazeInfoRunner #8058
Conversation
We found that providing fallback configuration to backend leads to recursive watch roots being set up for include paths mentioned in the fallback config. We know this has very negative performance implications because of greediness in VfsWarmQueue. This extension point allows us to disable fallback configurations for Bazel projects. However, since the extension point was introduced in 2025.2.5, it is not save to simply instantiate the implementation class and we need to do some manual registration magic.
Summary of ChangesHello @LeFrosch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the integration with CLion's Radler feature for C++ projects, specifically by introducing a mechanism to disable fallback configurations for Bazel projects and ensuring compatibility across various CLion versions. It also includes utility additions for managing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix a NullPointerException in BlazeInfoRunner, but currently only adds a TODO comment identifying the issue. I've provided a suggestion to actually fix the NPE. Additionally, the PR introduces functionality to disable fallback configurations for Blaze projects. I've made a few suggestions to improve the robustness and code style of this new functionality. Specifically, I recommend adding exception handling for reflective calls and adhering to idiomatic Kotlin style in a new compatibility file.
| try (InputStream blazeInfoStream = invoker.invokeInfo(builder, context)) { | ||
| // TODO: NPE blazeInfoStream is null when there is an error in the invocation | ||
| return blazeInfoStream.readAllBytes(); | ||
| } |
There was a problem hiding this comment.
The added TODO correctly identifies a potential NullPointerException but doesn't fix it. To prevent the NPE, you should check if blazeInfoStream is null before using it. The try-with-resources statement itself will throw an NPE if the stream is null. A better approach is to get the stream, check for null, and then use a try-with-resources block on the non-null stream. Returning an empty byte array in case of a null stream is a safe way to handle this, as it will result in an empty BlazeInfo object downstream, which is better than crashing.
InputStream blazeInfoStream = invoker.invokeInfo(builder, context);
if (blazeInfoStream == null) {
return new byte[0];
}
try (blazeInfoStream) {
return blazeInfoStream.readAllBytes();
}| val implementation = Class.forName(IMPLEMENTATION_CLASS).getConstructor().newInstance() | ||
|
|
||
| extensionPoint.registerExtension(implementation, pluginApplicationDisposable()) |
There was a problem hiding this comment.
The reflective instantiation and registration can throw several exceptions (e.g., ClassNotFoundException, NoSuchMethodException). While unlikely in a normal scenario, it's safer to wrap this logic in a try-catch block to prevent unhandled exceptions from propagating and potentially causing issues during application startup. You should log any exceptions that occur.
try {
val implementation = Class.forName(IMPLEMENTATION_CLASS).getConstructor().newInstance()
extensionPoint.registerExtension(implementation, pluginApplicationDisposable())
} catch (e: Exception) {
com.intellij.openapi.diagnostic.Logger.getInstance(RadBazelProjectFallbackConfigRegister::class.java).error("Failed to register RadBazelProjectFallbackConfigDisabler", e)
}| package com.google.idea.sdkcompat.radler; | ||
|
|
||
| import com.intellij.openapi.project.Project | ||
|
|
||
| /** | ||
| * This is a do nothing stub. #api251 | ||
| */ | ||
| interface RadProjectFallbackConfigDisabler { | ||
|
|
||
| suspend fun disableFallbackConfigForProject(project: Project): Boolean; | ||
| } |
There was a problem hiding this comment.
This Kotlin file uses Java-style semicolons at the end of the package declaration and the function signature. While technically valid, this is not idiomatic Kotlin. Please remove them to maintain a consistent Kotlin style across the codebase.
| package com.google.idea.sdkcompat.radler; | |
| import com.intellij.openapi.project.Project | |
| /** | |
| * This is a do nothing stub. #api251 | |
| */ | |
| interface RadProjectFallbackConfigDisabler { | |
| suspend fun disableFallbackConfigForProject(project: Project): Boolean; | |
| } | |
| package com.google.idea.sdkcompat.radler | |
| import com.intellij.openapi.project.Project | |
| /** | |
| * This is a do nothing stub. #api251 | |
| */ | |
| interface RadProjectFallbackConfigDisabler { | |
| suspend fun disableFallbackConfigForProject(project: Project): Boolean | |
| } |
1 participant
TODO