[
https://issues.apache.org/jira/browse/GROOVY-12000?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18079372#comment-18079372
]
Paul King commented on GROOVY-12000:
------------------------------------
h2. AI Assessment
Probing around the path GROOVY-12000 example flagged uncovered two pre-existing
soundness
bugs and two precision gaps in the {{instanceof}} flow-typing machinery.
For the specific example in the issue ({{!(number instanceof Cloneable ||
number instanceof Closeable)}})
is currently *sound but lossy*, but nearby shapes are not.
None of these were caused by the GROOVY-11983 patches — they have lived in
{{visitNotExpression}}'s blind tti-flip since the 2022 negative-flow-typing
work (GROOVY-6429 / GROOVY-8412 / GROOVY-9931) — but they sit exactly where
GROOVY-11983 was poking.
h2. Where the framework actually is today
Confirmed via {{@ASTTest}} and runtime probes:
||declared type||narrow shape||inferred type in the narrow branch||status||
|{{Object x}}|{{x instanceof List \|\| x instanceof Map}}
(then)|{{<UnionType:List+Map>}}|(/) sound + precise|
|{{CharSequence c}}|{{c instanceof Comparable \|\| c instanceof Serializable}}
(then)|{{(<UnionType:Comparable+Serializable> & CharSequence)}}|(/) sound +
precise|
|{{Number n}}|{{n instanceof Cloneable \|\| n instanceof Closeable}}
(then)|{{Number}} — union dropped|(!) sound but lossy *(this is Eric's case)*|
|{{Object x}}|{{!(!(x instanceof CharSequence) && !(x instanceof Number))}}
(then)|{{(Number & CharSequence)}} — should be a *union*|(x) *unsound*|
h2. Findings
h3. 1. Soundness bug — THEN of {{!(cond && !(x instanceof T))}}
{code:groovy}
@CompileStatic
String f(Object x, boolean cond) {
if (!(cond && !(x instanceof String))) {
return "len=${x.length()}" // compiles
}
return "else"
}
f(42, false) // → ClassCastException: Integer cannot be cast to String
{code}
When {{cond=false}} the outer expression is true but {{x}} can be anything;
the type checker still smart-casts {{x}} to {{String}}.
Trace: {{visitNotExpression}} flips the inner {{[!instanceof,x]→[T]}} entry
into a *positive* {{x→[T]}} at the if-scope, ignoring the {{&&}} gating.
Site: {{StaticTypeCheckingVisitor.java}} lines 821–826 — the unconditional
{{forEach(this::putNotInstanceOfTypeInfo)}} on the way out of the outer Not.
The existing {{testNotInstanceof9}} doesn't catch this because the THEN
branch returns {{-1}} without touching {{x}}, and the ELSE uses
{{x.hashCode()}} (an {{Object}} method that wouldn't expose the smart-cast).
h3. 2. Soundness bug — De-Morgan'd OR via double-Not
{code:groovy}
@CompileStatic
String f(Object x) {
if (!(!(x instanceof CharSequence) && !(x instanceof Number))) {
return "iv=${x.intValue()}" // compiles
}
return "no"
}
f("hello") // → GroovyCastException: String cannot be cast to Number
{code}
{{!(!A && !B)}} should yield {{A∪B}} in the THEN branch. Instead, both
{{[!instanceof,x]}} entries flip back to *positive* into the same bucket
and {{getInferredTypeFromTempInfo}} collapses {{[CharSequence, Number]}}
through {{newIntersectionTypeClassNode}} into {{(Number & CharSequence)}}.
The type checker then permits methods of *both*, but {{x}} satisfies only one.
Same root cause as (1): the flip carries no record of which logical
operator combined the operands inside.
h3. 3. Precision loss — {{Number & UnionType(...)}} collapses to {{Number}}
*(Eric's original case)*
{code:groovy}
@CompileStatic
def f(Number n) {
if (n instanceof Cloneable || n instanceof Closeable) {
// inferred: java.lang.Number — the union info is silently dropped
// can't call clone() / close() without an inner instanceof
}
}
{code}
Sound, but no narrowing benefit.
Cause: {{newIntersectionTypeClassNode}}
({{StaticTypeCheckingVisitor.java:6800}})
partitions components into {{interface}} vs {{non-interface}} buckets.
{{UnionTypeClassNode.isInterface()}} returns {{false}}, so it lands in
{{supers}} alongside {{Number}}; the function then returns {{supers.get(0)}}
and silently drops the union. The interface-declared case
({{CharSequence}} above) escapes this because the declared interface goes
into the {{interfaces}} bucket and the LUB construction survives.
h3. 4. Precision loss — OR with negative entries (TODO already in code)
{{propagateTemporaryTypeInfo}} at {{StaticTypeCheckingVisitor.java:1064}}
literally has:
{code:java}
// TODO: deal with (x !instanceof T)
lhs.keySet().removeIf(k -> k instanceof Object[]);
rhs.keySet().removeIf(k -> k instanceof Object[]);
{code}
This silently kills smart-cast for shapes like:
* {{cond || !(x instanceof T)}}
* {{!(!(x instanceof A) || !(x instanceof B))}} — would soundly be {{A ∩ B}}
h2. How this relates to GROOVY-11983
The recent patches are correct _for the cases they target_:
{{cond && !(x instanceof Y)}}'s ELSE-branch unsoundness is gone, and the
{{0f692ec17f}} follow-up correctly re-enabled the leading-Not case.
But the area underneath — {{visitNotExpression}}'s flip — was never
reworked, and {{canInvertNarrowingForElseBranch}} only gates the ELSE
branch. Neither addresses the THEN branch when the boolean visit itself
populates an unsound positive entry.
h2. Suggested direction (in priority order)
# *Fix the {{visitNotExpression}} flip to be structure-aware.* Two options:
## (conservative) Walk the operand and, like
{{canInvertNarrowingForElseBranch}}, refuse to flip when the operand is gated
by {{&&}} / {{&}} / {{^}}. Cheap; closes findings (1) and (2) at the cost of
also losing some currently-correct inferences.
## (precise) Push/pop scopes inside {{visitBinaryExpression(&&)}} the way the
{{||}} path already does, so an outer Not has clean per-operand info to flip,
and reshape {{propagateTemporaryTypeInfo}} to do the right
disjunction/conjunction. More invasive, more precise.
# *Teach {{newIntersectionTypeClassNode}} about {{UnionTypeClassNode}}.* Either
special-case it into the {{interfaces}} slot of {{LowestUpperBoundClassNode}}
(gives {{Number & <UnionType:...>}}, mirroring the interface-declared case that
already works), or distribute ({{UnionType(Number&Cloneable,
Number&Closeable)}}). The former is much less work and matches the pattern that
already produces good results for interface-typed declarations.
# *Address the {{propagateTemporaryTypeInfo}} negative-entry TODO*, _after_ (1)
lands — handling {{!instanceof}} in the disjunction lets the De-Morgan'd OR
path recover precision soundly.
# *Add THEN-branch tests to {{testNotInstanceof*}}.* The existing suite hits
ELSE thoroughly but barely exercises THEN with smart-cast-sensitive method
calls. Adding {{length()}} / {{intValue()}}-style probes in THEN would have
caught (1) and (2) immediately and will prevent regression once fixed.
Recommendation: tackle 1(a) as a first conservative pass to close the
soundness floor; pursue 1(b) and 2 once that is solid. The original
{{Number}}-union case is the least urgent — it's a precision miss, not a
correctness failure.
> STC: instanceof or combined with not
> ------------------------------------
>
> Key: GROOVY-12000
> URL: https://issues.apache.org/jira/browse/GROOVY-12000
> Project: Groovy
> Issue Type: Bug
> Components: Static Type Checker
> Affects Versions: 6.0.0-alpha-1
> Reporter: Eric Milles
> Priority: Major
>
> Consider the following:
> {code:groovy}
> @groovy.transform.TypeChecked
> void test(Number number) {
> if (!(number instanceof Cloneable || number instanceof Closeable)) {
> number
> } else {
> number
> }
> }
> {code}
> Currently this passes the check for "can invert" and produces a type like
> (Number & (Cloneable | Closeable))" for the else path. Not sure if we're
> ready for that or not.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
