Skip to content

Null check for org.eclipse.help.IContext #1863

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SougandhS
Copy link
Contributor

Check whether context is null or not before invoking getText()

Fixes : #1689

Copy link
Contributor

github-actions bot commented May 8, 2025

Test Results

 1 764 files  ±0   1 764 suites  ±0   1h 24m 55s ⏱️ +31s
 4 381 tests ±0   4 357 ✅ ±0   24 💤 ±0  0 ❌ ±0 
13 143 runs  ±0  12 976 ✅ ±0  167 💤 ±0  0 ❌ ±0 

Results for commit 7ff57f3. ± Comparison against base commit c3dfa7b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the right thing to do? Cancelling an execution somewhere deep inside a method because a value is null for undocumented reasons sounds a bit dangerous. And the affected code is actually related to a bug documented here:

The referenced issue was introduced by exactly the code that now may lead to a null value. So the proposed change rather seems to work around an issue with the change made in https://bugs.eclipse.org/bugs/show_bug.cgi?id=533828 than adding an actual missing null pointer check.

@SougandhS
Copy link
Contributor Author

The referenced issue was introduced by exactly the code that now may lead to a null value.

Thank you for the detailed review and context. I agree that introducing a null check here may mask the underlying issue rather than addressing its root cause, especially considering the regression mentioned in bug 533828.

Given that, I'll go ahead and close this PR. It makes more sense to investigate and resolve the underlying cause of the null value instead of working around.

@SougandhS SougandhS closed this Jun 2, 2025
@laeubi
Copy link
Contributor

laeubi commented Jun 2, 2025

If the value must not be null at this place, a null check can still be sufficient, e.g using Objects.requireNonNull(context, "describe why it must not be null here");
This is much better than having the code run into a generic NPE...

@SougandhS
Copy link
Contributor Author

If the value must not be null at this place, a null check can still be sufficient, e.g using Objects.requireNonNull(context, "describe why it must not be null here"); This is much better than having the code run into a generic NPE...

thanks I'll update with this

Check whether context is null or not before invoking getText()

Fixes : eclipse-platform#1689
@eclipse-platform-bot
Copy link
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

runtime/features/org.eclipse.core.runtime.feature/feature.xml
ua/org.eclipse.help.ui/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From 4ea6c3968449872910dfef8f75dd3101aba0f644 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Fri, 6 Jun 2025 00:32:15 +0000
Subject: [PATCH] Version bump(s) for 4.37 stream


diff --git a/runtime/features/org.eclipse.core.runtime.feature/feature.xml b/runtime/features/org.eclipse.core.runtime.feature/feature.xml
index be6874fea0..5e0968005c 100644
--- a/runtime/features/org.eclipse.core.runtime.feature/feature.xml
+++ b/runtime/features/org.eclipse.core.runtime.feature/feature.xml
@@ -2,7 +2,7 @@
 <feature
       id="org.eclipse.core.runtime.feature"
       label="%featureName"
-      version="1.4.800.qualifier"
+      version="1.4.900.qualifier"
       provider-name="%providerName"
       license-feature="org.eclipse.license"
       license-feature-version="0.0.0">
diff --git a/ua/org.eclipse.help.ui/META-INF/MANIFEST.MF b/ua/org.eclipse.help.ui/META-INF/MANIFEST.MF
index 1218bc2fd5..7593a41764 100644
--- a/ua/org.eclipse.help.ui/META-INF/MANIFEST.MF
+++ b/ua/org.eclipse.help.ui/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %help_system_plugin_name
 Bundle-SymbolicName: org.eclipse.help.ui; singleton:=true
-Bundle-Version: 4.8.100.qualifier
+Bundle-Version: 4.8.200.qualifier
 Bundle-Activator: org.eclipse.help.ui.internal.HelpUIPlugin
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.49.0

Further information are available in Common Build Issues - Missing version increments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullPointerException in DefaultHelpUi.displayContext
4 participants