Skip to content

added @SafeVarargs to ensure warnings error enabled on main#20945

Open
ThyTran1402 wants to merge 1 commit intoopensearch-project:mainfrom
ThyTran1402:fix/ensure_warnings_enabled_for_CI_on_main
Open

added @SafeVarargs to ensure warnings error enabled on main#20945
ThyTran1402 wants to merge 1 commit intoopensearch-project:mainfrom
ThyTran1402:fix/ensure_warnings_enabled_for_CI_on_main

Conversation

@ThyTran1402
Copy link
Contributor

@ThyTran1402 ThyTran1402 commented Mar 20, 2026

Description

Replace @SuppressWarnings("unchecked") with @SafeVarargs on all generic varargs methods across libs/core and libs/common.

Root cause: Methods with generic varargs parameters (e.g., Iterator<? extends T>..., E...) produce two distinct compiler warnings:
A declaration-site warning (-Xlint:varargs): "Possible heap pollution from parameterized vararg type"
A call-site warning (-Xlint:unchecked): "unchecked generic array creation for varargs parameter"

With -Werror enabled globally, any call site outside that scope remained exposed and could fail compilation on a cold build which is exactly what caused the Maven snapshot publication to break after #9120 was merged as mentioned in #9149.

Fix: Annotate each safe generic varargs method with @SafeVarargs at the declaration. This suppresses both warnings for all callers automatically and is semantically correct because none of these methods write typed elements back into their varargs arrays with no heap pollution.

Related Issues

Resolves #9150

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
@ThyTran1402 ThyTran1402 requested a review from a team as a code owner March 20, 2026 16:43
@github-actions github-actions bot added bug Something isn't working Build Build Tasks/Gradle Plugin, groovy scripts, build tools, Javadoc enforcement. Build Libraries & Interfaces CI CI related good first issue Good for newcomers labels Mar 20, 2026
@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add @SafeVarargs to libs/common generic varargs methods

Relevant files:

  • libs/common/src/main/java/org/opensearch/common/collect/Iterators.java
  • libs/common/src/test/java/org/opensearch/common/collect/IteratorsTests.java
  • libs/common/src/main/java/org/opensearch/common/util/set/Sets.java

Sub-PR theme: Add @SafeVarargs to libs/core generic varargs methods

Relevant files:

  • libs/core/src/main/java/org/opensearch/ExceptionsHelper.java
  • libs/core/src/main/java/org/opensearch/core/common/util/CollectionUtils.java

⚡ Recommended focus areas for review

SafeVarargs on Non-final

@SafeVarargs on the package-private ConcatenatedIterator constructor is valid only if the constructor is not overridden. Since it is a constructor (not a method), this is fine. However, verify that ConcatenatedIterator is not subclassed anywhere, as @SafeVarargs on non-final, non-static, non-private methods would be a compile error in Java, but constructors are safe. Confirm no heap pollution risk exists in the constructor body.

@SafeVarargs
ConcatenatedIterator(Iterator<? extends T>... iterators) {
SafeVarargs Correctness

@SafeVarargs on unwrap is appropriate only if the clazzes varargs array is never written to or exposed in a heap-polluting way. The method iterates over clazzes using isInstance, which is safe. However, confirm that no subclass or override of this method writes to the varargs array, as @SafeVarargs does not propagate to overrides.

@SafeVarargs
public static Throwable unwrap(Throwable t, Class<?>... clazzes) {

@github-actions
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Retain suppressed unchecked warning on method

The @SuppressWarnings("unchecked") annotation was removed from the convert method,
but the method still uses an unchecked cast via Iterators.concat with a varargs call
involving wildcard types. This may reintroduce an unchecked cast compiler warning
that was previously suppressed. Verify that removing this annotation does not cause
new compiler warnings or errors, and if so, retain the annotation.

libs/core/src/main/java/org/opensearch/core/common/util/CollectionUtils.java [164-167]

+@SuppressWarnings("unchecked")
 private static Iterable<?> convert(Object value) {
     return switch (value) {
         case Map<?, ?> map -> () -> Iterators.concat(map.keySet().iterator(), map.values().iterator());
         case Iterable<?> iterable when value instanceof Path == false -> iterable;
Suggestion importance[1-10]: 5

__

Why: The PR's intent is to replace @SuppressWarnings("unchecked") with @SafeVarargs where appropriate. The convert method itself doesn't use varargs, so @SafeVarargs doesn't apply here. However, since Iterators.concat now has @SafeVarargs, the unchecked warning at the call site may be suppressed by the callee's annotation, making the @SuppressWarnings on convert unnecessary. The suggestion to verify is valid but the concern may already be addressed by the @SafeVarargs on concat.

Low

@github-actions
Copy link
Contributor

❌ Gradle check result for 98f884e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Build Libraries & Interfaces Build Build Tasks/Gradle Plugin, groovy scripts, build tools, Javadoc enforcement. CI CI related good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure that warnings as errors are enabled for CI checks on main

1 participant