Skip to content

SONARJAVA-5718 Fix union type fully qualified names - #6239

Open
benzonico wants to merge 1 commit into
masterfrom
fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes
Open

benzonico wants to merge 1 commit into
masterfrom
fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes

Conversation

@benzonico

Copy link
Copy Markdown
Contributor

SONARJAVA-5718 Fix fullyQualifiedName() on Union types

https://sonarsource.atlassian.net/browse/SONARJAVA-5718

Union types in multi-catch clauses now have unique fully qualified names instead of the common exception superclass name.

The semantic Type API identifies union types and returns their alternatives. The parser retains resolved alternatives, and the type model produces a stable, sorted pipe-separated fully qualified name.

Compatibility: Type gains isUnionType() and getUnionTypes(). Non-union and unknown types retain a single-type fallback.

Acceptance criteria: multi-catch types now expose their alternatives and report the expected qualified name. Focused parser-semantic validation passes.

Validation: mvn -B -ntp -pl java-frontend -am -Dtest=JParserSemanticTest -Dsurefire.failIfNoSpecifiedTests=false test passed after rebase. The broader reactor test fails before java-frontend in TestClasspathUtilsTest.test_modules_classpath under Java 26; the identical failure reproduces at the merge base.

Vortex: context guidance informed the explicit API defaults, stable naming, and semantic regression coverage. Deep agentic analysis ran with enabled entitlement and project binding. It found no introduced CRITICAL or HIGH findings; the 14 reported findings are pre-existing outside changed lines.

No migration is required.

This pull request was opened as a draft by the autonomous factory. It may be marked ready only after its mechanical gates pass, and it still requires human approval and merge.

Generated by AI Factory

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

SONARJAVA-5718

Comment thread java-frontend/src/main/java/org/sonar/java/model/JavaTree.java
gitar-bot[bot]

This comment was marked as resolved.

@benzonico

Copy link
Copy Markdown
Contributor Author

Factory update: the Gitar finding is fixed locally in 8574e197a3 and focused parser-semantic validation plus deep Vortex analysis passed. The guarded git push --force-with-lease update was rejected twice as stale info even though the remote head was verified as the prior Factory commit. The amended work is preserved locally; a human decision is required before any further branch update.

Generated by AI Factory

@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch from fd8c292 to 8574e19 Compare September 25, 2026 16:02
Comment thread java-frontend/src/main/java/org/sonar/java/model/JavaTree.java
Comment thread java-frontend/src/main/java/org/sonar/java/model/JUnionType.java
@gitar-bot
gitar-bot Bot dismissed their stale review September 25, 2026 16:06

✅ Code review updated (blocking issues remain unresolved).

Configure merge blocking

@datadog-sonarsource

This comment has been minimized.

@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch from 8574e19 to 87c53b7 Compare September 25, 2026 16:13
@benzonico

Copy link
Copy Markdown
Contributor Author

Addressed the current Gitar findings in 87c53b7c.

  • Catch-variable symbols and identifier references now resolve through their declaration’s union type.
  • JType and JUnionType equality is symmetric, and union equality/hash values use the sorted qualified-name representation.
  • Added semantic assertions for the parameter symbol, identifier symbol, and union type tree.

Validation: focused parser-semantic Maven suite passed; changed-file secrets scan passed; deep Vortex analysis found no introduced CRITICAL or HIGH findings.

Generated by AI Factory

@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch from 87c53b7 to 1efac81 Compare September 25, 2026 16:27
@benzonico

Copy link
Copy Markdown
Contributor Author

Addressed the CI regression in 1efac815.

The recovered-binding test no longer asserts an ECJ-dependent qualified name. The normal union-type test continues to verify the stable public union semantics.

Validation: mvn -B -ntp -pl java-frontend -am -Dtest=JParserSemanticTest -Dsurefire.failIfNoSpecifiedTests=false test passed. Changed-file secrets scan passed. Deep Vortex analysis found only pre-existing findings at lines 609 and 1272, outside this change.

Generated by AI Factory

Comment thread java-frontend/src/main/java/org/sonar/java/model/JType.java
@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch from 1efac81 to 7f58c8b Compare September 25, 2026 16:39
@benzonico

Copy link
Copy Markdown
Contributor Author

Addressed the current Gitar findings in 7f58c8ba.

  • JUnionType.is(String) now preserves the underlying binding match in addition to the union qualified name. This keeps consumers that query the LUB-compatible name working.
  • name() and toString() now present sorted union alternatives.
  • Recovered-union coverage verifies the union alternatives and identifier LUB behavior.

symbol() and erasure() intentionally retain the least-upper-bound binding: a union has no declared type symbol, and its erasure is the LUB.

Validation: mvn -B -ntp -pl java-frontend -am -Dtest=JParserSemanticTest -Dsurefire.failIfNoSpecifiedTests=false test passed. Changed-file secrets scan passed. Deep Vortex analysis found only pre-existing findings at lines 609 and 1272, outside this change.

Generated by AI Factory

Comment thread java-frontend/src/main/java/org/sonar/java/model/JUnionType.java
Comment thread java-frontend/src/main/java/org/sonar/java/model/JUnionType.java
@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch from 7f58c8b to 48f3411 Compare September 25, 2026 16:50
@benzonico

Copy link
Copy Markdown
Contributor Author

Addressed the latest Gitar findings in 48f34113.

  • getUnionTypes() returns a defensive copy.
  • Union display names retain duplicate simple names while qualified names stay unique and sorted.
  • is(String) now also matches a type that every union alternative is a subtype of, which preserves exception-supertype checks for consumers such as symbolic execution.

Validation: focused parser-semantic Maven suite passed; changed-file secrets scan passed; deep Vortex analysis found only pre-existing findings at lines 609 and 1272, outside this change.

Generated by AI Factory

@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch 3 times, most recently from 23cfcb4 to 68334e3 Compare September 25, 2026 17:45
@benzonico

Copy link
Copy Markdown
Contributor Author

Addressed the SonarQube coverage gate in 68334e3d.

The semantic test now exercises recovery of a union alternative with a missing binding, covering the remaining JUnionType constructor branch.

Validation: mvn -B -ntp -pl java-frontend -am -Dtest=JParserSemanticTest -Dsurefire.failIfNoSpecifiedTests=false test passed. The changed-file secrets scan passed. Deep Vortex analysis found only pre-existing findings at JParserSemanticTest.java:610 (java:S5738) and JParserSemanticTest.java:1273 (java:S5961), outside this change.

Generated by AI Factory

@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch from 68334e3 to 9c5fa53 Compare September 25, 2026 17:56
@benzonico

Copy link
Copy Markdown
Contributor Author

Addressed the SonarQube quality-gate issue in 9c5fa53c.

unionFullyQualifiedName now uses a stream pipeline to map, deduplicate, sort, and join the alternative fully qualified names. This preserves the existing result while satisfying java:S9391.

Validation: mvn -B -ntp -pl java-frontend -am -Dtest=JParserSemanticTest -Dsurefire.failIfNoSpecifiedTests=false test passed. The changed-file secrets scan passed. Deep Vortex analysis was clean.

Generated by AI Factory

Comment thread java-frontend/src/main/java/org/sonar/java/model/JUnionType.java
Expose union type alternatives through the semantic type API and derive a stable qualified name for multi-catch types.
@benzonico
benzonico force-pushed the fix/nperu/SONARJAVA-5718-fixFullyQualifiedNameUnionTypes branch from 9c5fa53 to cf7ab00 Compare September 25, 2026 18:04
@benzonico

Copy link
Copy Markdown
Contributor Author

Addressed the latest Gitar review in cf7ab00b.

JUnionType.is(String) now preserves exact-type behavior: it matches the union FQN and its original LUB binding only. It no longer reports Throwable or Object as matches, avoiding false positives in rules that use Type.is.

Validation: mvn -B -ntp -pl java-frontend -am -Dtest=JParserSemanticTest -Dsurefire.failIfNoSpecifiedTests=false test passed. The changed-file secrets scan passed. Deep Vortex analysis reported only pre-existing untouched findings at JParserSemanticTest.java:610 (java:S5738) and JParserSemanticTest.java:1273 (java:S5961).

Generated by AI Factory

@gitar-bot

gitar-bot Bot commented Sep 25, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 9 closed / 9 findings

🔴 High risk · Public Type API adds abstract methods that may break external implementations

Fixes union type fully qualified names in multi-catch clauses so they report their alternatives instead of the common exception superclass name. Union types now have a stable, sorted pipe-separated FQN, the Type API gains isUnionType() and getUnionTypes() methods, and is() matching is restricted to the union FQN and original LUB binding. All findings have been addressed.

✅ 9 closed
✅ Bug: Union alternatives keyed on shared LUB binding change every same-type use

📄 java-frontend/src/main/java/org/sonar/java/model/JavaTree.java:530-536 📄 java-frontend/src/main/java/org/sonar/java/model/JParser.java:2667-2668 📄 java-frontend/src/main/java/org/sonar/java/model/JType.java:221-235 📄 java-frontend/src/main/java/org/sonar/java/model/JType.java:284-298 📄 java-frontend/src/main/java/org/sonar/java/model/JSema.java:53 📄 java-frontend/src/main/java/org/sonar/java/model/JSema.java:59-61 📄 java-frontend/src/main/java/org/sonar/java/model/JType.java:60-64 📄 java-frontend/src/main/java/org/sonar/java/model/JType.java:284-287
For a UnionType, ECJ's resolveBinding() returns the least-upper-bound class binding. The old test confirmed this: fullyQualifiedName() was java.lang.RuntimeException, and getIntersectionTypes() returned only that type. Inside one AST, ECJ returns that same DOM ITypeBinding object for every other RuntimeException reference. UnionTypeTreeImpl.symbolType() then does sema.unionTypeAlternatives.put(typeBinding, alternativeBindings), which makes the plain RuntimeException type (and RuntimeException[], because baseQualifiedName recurses into components) look like a union for the whole file. After that, isUnionType() returns true for it, getUnionTypes() returns the multi-catch alternatives, and, depending on when the JType is created, fullyQualifiedName() becomes java.lang.MatchException | java.lang.NumberFormatException. So type.is("java.lang.RuntimeException") fails for new RuntimeException(), throw expressions, and similar code. Two multi-catches with the same LUB but different alternatives also overwrite each other in the map. This is easy to hit: checks such as ServletMethodsExceptionsThrownCheck and AbstractOneExpectedExceptionRule call parameter().type().symbolType() on catch types. The alternatives need a key that belongs to the union itself, for example stored on the UnionTypeTreeImpl and returned as a dedicated union Type object, not a map keyed on the shared LUB binding.

✅ Bug: Catch variable e still has a non-union type despite the new API contract

📄 java-frontend/src/main/java/org/sonar/java/model/JavaTree.java:531-539 📄 java-frontend/src/main/java/org/sonar/plugins/java/api/semantic/Type.java:302-316
The union type is only built in UnionTypeTreeImpl.symbolType(). The catch parameter's symbol type (JSymbol.type() → sema.type(variableBinding.getType())) and every IdentifierTree that refers to e still go through JSema.type(), which returns a plain JType for the common-supertype binding. So for catch (IOException | SQLException e), e.symbol().type().isUnionType() is false and fullyQualifiedName() is still java.lang.Exception. The new Type.isUnionType() javadoc promises true for "the type of e in a multi-catch clause", and the PR says multi-catch types now report the unique name. That only holds when you read the type tree directly, so checks that use parameter().symbol().type() or the identifier's symbolType() (e.g. LoggedRethrownExceptionsCheck, CatchUsesExceptionWithContextCheck) see a different type from parameter().type().symbolType(). Either give the catch variable symbol and its references the same union type, or narrow the javadoc and description to the UnionTypeTree only.

✅ Bug: JUnionType.equals/hashCode break symmetry with JType and ignore order

📄 java-frontend/src/main/java/org/sonar/java/model/JUnionType.java:50-58 📄 java-frontend/src/main/java/org/sonar/java/model/JType.java:316-330
JType.equals accepts any JType subclass and only compares typeBinding. So runtimeExceptionJType.equals(unionOfMatchAndNfe) is true, because both hold the same LUB binding, while union.equals(runtimeExceptionJType) is false because JUnionType.equals requires another JUnionType. The two objects that compare equal also have different hash codes (fullyQualifiedName().hashCode() vs Arrays.hashCode(unionTypes)), which breaks the equals/hashCode contract in HashSet and HashMap. Separately, Arrays.equals compares alternatives in source order, but the FQN is sorted. A | B and B | A therefore get the same fullyQualifiedName() but compare unequal. Fix: make JType.equals reject JUnionType (e.g. check getClass()), and compare and hash the alternatives order-insensitively, e.g. by FQN.

✅ Quality: JUnionType name()/toString()/symbol()/erasure() still describe the LUB

📄 java-frontend/src/main/java/org/sonar/java/model/JUnionType.java:194-208 📄 java-frontend/src/main/java/org/sonar/java/model/JType.java:284-298
JUnionType overrides only fullyQualifiedName(), isUnionType(), getUnionTypes(), equals and hashCode. name(), toString(), symbol() and erasure() are inherited from JType and still read the LUB typeBinding. For catch (MatchException | NumberFormatException v), v.symbol().type() now has fullyQualifiedName() = "java.lang.MatchException | java.lang.NumberFormatException", but name()/toString() return "RuntimeException", symbol().type() is the plain RuntimeException type, and erasure() returns a type that is not equal to the union and has a different FQN. The same object now describes itself two different ways, which will confuse rule messages and any code that round-trips through symbol() or erasure(). Override these methods so they agree with the union (for example, name() joins the alternatives' simple names, and erasure() returns this), or document that they describe the LUB.

✅ Quality: union_type test drops type assertions; unresolved-union case untested

📄 java-frontend/src/test/java/org/sonar/java/model/JParserSemanticTest.java:1430-1442
The delta deletes both type assertions from union_type (catch (Unknown1 | Unknown2 e)) and adds nothing in their place. The removed e.symbolType()).is("java.lang.Object") check covered identifier usages, and this PR does not change that path: IdentifierTree.symbolType() still goes through sema.type(LUB). After the change, the only test with unresolved union alternatives no longer asserts anything about exceptionVariable.type(). So nothing covers what fullyQualifiedName(), isUnionType(), getUnionTypes() or isUnknown() return when the alternatives are recovered bindings. It also hides that e.symbol().type() (a JUnionType) and e.symbolType() (the LUB) now differ for the same variable. Add assertions for the new behavior in this case, and keep the usage-type assertion.

...and 4 more closed from earlier reviews

Review coverage

🧪 Functional validation 2 of 2 objectives covered

📋 Rules No rules evaluated

🤖 Auto-approval Not enabled · Set up

Implementation Status ✅ 2 of 2 objectives covered
✅ SONARJAVA-5718 - 2 of 2 objectives covered

This PR implements union type support by adding isUnionType() and getUnionTypes() to the Type interface and ensures fullyQualifiedName() on union types returns sorted alternatives separated by |.

✅ 2 covered here
  • ✅ Introduce a UnionType symbol or add isUnionType() and unionTypeAlternatives() to the Type interface
  • ✅ Ensure fullyQualifiedName() on Union types returns a unique string of sorted alternatives separated by " | "
Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown
Contributor

@benzonico
benzonico marked this pull request as ready for review September 25, 2026 18:25
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically

@github-actions github-actions Bot added the stale label Oct 3, 2026

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant