[
https://issues.apache.org/jira/browse/GROOVY-12584?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135424#comment-18135424
]
ASF GitHub Bot commented on GROOVY-12584:
-----------------------------------------
paulk-asert opened a new pull request, #3111:
URL: https://github.com/apache/groovy/pull/3111
…shSet or ArrayList throws a bare ClassCastException
The cast selector's collection fast path tested the subtype relation the
wrong way round, so it never fired for Set, AbstractSet, Collection or List
targets. It fired only for an abstract subclass of HashSet or ArrayList, where
it built a plain HashSet or ArrayList that then failed the cast with a
ClassCastException instead of a GroovyCastException.
Remove the fast path so every collection cast goes through
DefaultTypeTransformation.castToType, as classic call sites do. A corrected
fast path would have diverged from classic call sites (HashSet instead of
LinkedHashSet for a Set target). The unused HASHSET_CONSTRUCTOR and
ARRAYLIST_CONSTRUCTOR handles are deprecated.
> Indy cast of a Collection to an abstract subclass of HashSet or ArrayList
> throws a bare ClassCastException
> ----------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12584
> URL: https://issues.apache.org/jira/browse/GROOVY-12584
> Project: Groovy
> Issue Type: Bug
> Components: Compiler
> Affects Versions: 4.0.33, 5.1.3, 6.0.0
> Reporter: Paul King
> Priority: Minor
>
> h3. Problem
> The invokedynamic cast selector
> (\{{Selector.CastSelector.handleCollections}}) has a fast path that is meant
> to turn a \{{Collection}} into a \{{HashSet}} or \{{ArrayList}} for an
> abstract target type. Its check is the wrong way round:
> {code:java}
> private static boolean isAbstractClassOf(final Class<?> type, final Class<?>
> callSiteType) {
> return (callSiteType.isInterface() ||
> Modifier.isAbstract(callSiteType.getModifiers())) &&
> type.isAssignableFrom(callSiteType);
> }
> {code}
> {\{isAbstractClassOf(HashSet.class, target)}} is true only when the target is
> an abstract *subclass* of \{{HashSet}}. So for the targets it was presumably
> written for (\{{Set}}, \{{AbstractSet}}, \{{Collection}}, \{{List}}) the fast
> path never fires, and the cast goes to
> \{{DefaultTypeTransformation.castToType}}, as it does with classic call
> sites. For an abstract subclass of \{{HashSet}} or \{{ArrayList}} it does
> fire. It builds a plain \{{HashSet}}/\{{ArrayList}}, which then fails the
> cast to the target type with a bare \{{ClassCastException}}, where classic
> call sites throw \{{GroovyCastException}}.
> h3. Reproducer
> {code:groovy}
> abstract class ASet extends HashSet {}
> abstract class AList extends ArrayList {}
> Object list = [3, 1, 2, 1]
> Object set = [3, 1, 2] as LinkedHashSet
> ASet s = list // or: AList l = set
> {code}
> ||Cast||indy||classic call sites||
> |\{{ASet s = list}}|\{{ClassCastException: Cannot cast java.util.HashSet to
> ASet}}|\{{GroovyCastException: ... Could not find matching constructor for:
> ASet(Integer, Integer, Integer, Integer)}}|
> |\{{AList l = set}}|\{{ClassCastException: Cannot cast java.util.ArrayList to
> AList}}|\{{GroovyCastException: ... Could not find matching constructor for:
> AList(Integer, Integer, Integer)}}|
> |\{{Set s = list}}, \{{AbstractSet s = list}}|\{{LinkedHashSet [3, 1,
> 2]}}|same|
> |\{{List l = set}}|\{{GroovyCastException}}|same|
> The cast fails either way; only the exception type and message differ.
> \{{GroovyCastException}} extends \{{ClassCastException}}, so existing
> \{{catch}} blocks are unaffected by a fix.
> The code dates back to the early indy work and is identical on
> \{{GROOVY_2_5_X}} (\{{vmplugin.v7}}), \{{GROOVY_3_0_X}}, \{{GROOVY_4_0_X}},
> \{{GROOVY_5_1_X}}, \{{GROOVY_6_0_X}} and master.
> h3. Proposed fix
> Remove \{{handleCollections()}} and \{{isAbstractClassOf()}} so that every
> collection cast goes through \{{castToType}}, matching classic call sites.
> Correcting the check instead would make indy diverge from classic call sites:
> * \{{Set s = list}} would produce a \{{HashSet}} (losing order) instead of
> today's \{{LinkedHashSet}}.
> * \{{List l = set}} would start succeeding, where it currently throws
> \{{GroovyCastException}} on both paths.
> Removing the fast path also removes a latent hazard: the cached handle \{{new
> HashSet(collection)}} is not null-safe, and since GROOVY-12557 a cast from a
> final static type is no longer guarded against a later \{{null}} value.
> {\{IndyGuardsFiltersAndSignatures.HASHSET_CONSTRUCTOR}} and
> \{{ARRAYLIST_CONSTRUCTOR}} are public. Keep them, deprecated, on master; they
> would then have no in-tree users.
> A test should cover both abstract-subclass casts expecting
> \{{GroovyCastException}} under indy, plus the
> \{{Set}}/\{{AbstractSet}}/\{{Collection}} shapes to confirm the result types
> don't change.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
