jenkinsci / groovy-sandbox

(Deprecated) Compile-time transformer to run Groovy code in a restrictive sandbox
MIT License
122 stars 60 forks source link

[FIXED JENKINS-50380] checkedCast should just return object when assignable #45

Closed abayer closed 6 years ago

abayer commented 6 years ago

JENKINS-50380

Without this, we end up doing some silly stuff like actually turning Foo f = new Foo() into Foo f = DefaultTypeTransformation.castToType(Foo.class, new Foo()), and even weirder stuff with Collections that aren't in java.util. The latter get particularly messed up if for some reason, the Collection class doesn't have a SomeCollection(Object[]) constructor.

So let's avoid the silliness of actually going through with emulating DefaultTypeTransformation.castToType and just use clazz.cast return the object when the object is assignable to the class.

cc @reviewbybees

abayer commented 6 years ago

fwiw - the Groovy internals for actually generating bytecode from CastExpression end up going to https://github.com/apache/groovy/blob/93d9fe61dcdf58b3dc6df665e1396f106c7f6d74/src/main/org/codehaus/groovy/classgen/asm/InvocationWriter.java#L903 when CastExpression#isCoerce() is true (which is the only case where this is relevant), so that if the object can be assigned to the class, well, just use the object as is without any casting, so I'm updating the PR to match that.

abayer commented 6 years ago

@jglick - well, the root bug is that we shouldn't be doing the whole rigamarole for cases like Foo f = new Foo(). If you mean the specific matter of a Collection without an array constructor being reinstantiated in the process of casting it, well, if you somehow managed to concoct a case where you're trying to cast one Collection type to another without an array constructor (and without a constructor that takes another Collection) in pure Groovy, I'm 99% sure you'd get a GroovyCastException based on the code in DefaultTypeTransformation.continueCastOnSAM(Object,Class).

So perhaps we should be throwing a GroovyCastException in a case where there's no valid constructor found for the casting. I..think that would mean catching a RejectedAccessException from here and if the message is ... unclassified new ..., throwing a GroovyCastException? I guess? Feels like a separate rathole to go down from fixing the real bug here, that we're currently doing a pile o' unnecessary casting/instantiation, which can have unpleasant side effects.

abayer commented 6 years ago

I'm gonna merge and release this, fyi.