RFR (S) 8151223: String concatenation fails with implicit toString() on package-private class

[Maurizio Cimadamore]

Hi Alexm
thanks for the patch; the idea is generally sound - I have few questions 
about the implementation of the 'sharpestAccessible' method when it 
comes to some javac quirks:

* note that when you recur on supertypes, you will end up with 
Type.noType, a type whose tag is NONE. In other words, supertype(Object) 
== Type.noType. This means that the current patch is (i) risking a bit 
when calling isAccessible on Type.noType - I'm not 100% sure if that's 
supported - might just work accidentallly and (ii) the subsequent test:

if (!sharpestClass.equals(syms.objectType)) {
+ return sharpestClass;
+ }


Is redundant (and wrong) - in other words, you will always exit the loop 
with some type that is != Object (I think), which makes the interface 
part useless.

* when it comes to interface sharpening, your code prefers D to B when 
calling the sharpening routine on InaccessibleA:

class InaccessibleA extends InaccessibeB implements InaccessibleC { }
class InaccessibleB implements B { }
interface InaccessibleC implements InaccessibleD { }
interface InaccessibleD extends D { }
interface B { }
interface D { }


In other words, the routine does a depth-first search on superclasses, 
followed by a depth-first search on interfaces. Would it make sense to 
have a routine in which all direct supertypes are treated equally? I.e. 
you look for an accessible supertype in the list of direct supertypes 
(Types.directSupertypes) - if one is found you return it, if none is 
found you repeat the check on the superclass.

* I'm not sure what happens when the input of the routine is not a 
classtype? I.e. an array or a type-var? Now, it seems like type-vars are 
probably erased before this point (or you'd have issue in the current 
impl too) - but arrays can definitively come up. Now, testing an array 
type doesn't make a lot of sense, since the array class is fictional; 
and array supertypes are not meaningful - i.e. Serializable and 
Cloneable - you don't want to end up with such types in the indy. I 
think array types should be special cased, so that the sharpening logic 
works on the element type. This means that this routine probably needs 
to become a type visitor (Types.SimpleTypeVisitor) - which has different 
behaviours for different type shapes.

The rest looks good.

Maurizio



On 07/03/16 18:46, Aleksey Shipilev wrote:
> Hi,
>
> There seems to be a corner case in our current String concatenation
> handling: we are recording the exact argument types to call
> StringConcatFactory with, to allow JDK to use that information for
> optimization. This works well, until we meet a private class. Then, we
> get an IllegalAccessError while trying to execute a bootstrap method
> referencing an inaccessible class:
>     https://bugs.openjdk.java.net/browse/JDK-8151223
>
> This does not happen with "old" concat flavor, because we are calling
> StringBuilder.append(Object) there, which does not leak away the private
> type.
>
> Luckily, we are not mandated to always call StringConcatFactory with an
> exact type. We can detect inaccessible classes during compilation, and
> use the closest accessible super-type instead. We could just strip to
> java.lang.Object in those cases, but it seems better to sharpen the type
> to the first accessible/non-ambiguous class/interface up the hierarchy.
>
> Webrev:
>     http://cr.openjdk.java.net/~shade/8151223/webrev.01
>
> Testing: failing test, new regression test; JDK java/lang/String;
> Langtools com/,jdk/,tools/
>
> Thanks,
> -Aleksey
>