Review Request JDK-8206121: [lworld] ensure non-nullable on setting value field via reflection and VarHandle

[Paul Sandoz]

> On Jul 5, 2018, at 10:44 PM, mandy chung <mandy.chung at oracle.com> wrote:
> 
> Frederic points out that only ACC_FLATTENABLE field whose type is listed
> in ValueTypes attribute is non-nullable and a field of value type may be nullable in LW1 (e.g. static value field).  I have revised the patch to
> be consistent with JVMS.
> 

Ok, i see (thanks Frederic for explaining), your patch looks good with regards to the consistency of the current runtime design and behavior (operating on a flattenable flattened (!) field). That’s good progress IMHO.

(Also i realize my point about Class.cast is wrong, i don’t see how that could work since it lacks the context of what classes are present in the ValueTypes attribute.)

—

However, AFAICT, with this design it does appear to introduce a discrepancy between the language and the runtime. Perhaps this is something to tackle after LW1?

The runtime appears to be assuming non-nullability and flattenability are one and the same thing rather than non-nullability being a dominant property that, among other value type properties, enables the emergent runtime property of flattening. Further, the runtime could choose to not flatten depending on certain size constraints, such as the number of fields in a value type, where as presumably it cannot choose to allow not not-null. This suggests to me ACC_FLATTENABLE is redundant, non-nullablity and therefore flattenability can be determined from presence of a field's type in the ValuesTypes attribute (and the same runtime constraints on flattening would still apply).

At the language level i would expect a strong guarantee (placing aside emotional types and backwards compatibility) that a value should never be null, but the runtime currently makes this ambiguous. For example:

 __ByValue class V { 
  … 
  void operate(V v) {
    // is v null?
    // ...
  }
}

class A {
  static V sfv = V.makeV(); 
  V fv = V.makeV();

  void fiddle() {
    // attempt to assign null to fields sfv, and fv
    // ...
  }
}

// separate compilation unit to A
// the developer knows nothing about ACC_FLATTENABLE of fields of A
class B { 
  void b(A a) {
    a.fiddle();
  
    // Javac currently does not allow assignment to null
    // All the following cause a compilation failure
    // a.sfv = null;
    // a.fv = null;
    // V v1 = null; 

    V v1 = a.sfv; // is v1 null?
    V v2 = a.fv; // is v2 null?
    b(v1, v2);
    b(v2, v1);
  }

  void b(V v1, V v2) {
    v1.operate(v2); // NPE on null receiver v1?
  }
}

Paul.




> Updated webrev:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8206121/webrev.01/index.html
> 
> MemberName::isFlattenable and Field::isFlattenable only looks at the
> modifiers/flags set by the JVM.  Frederic - I believe the type of
> the field has been resolved and validated with the field's declaring
> class's ValueTypes attribute.  Is that correct?  Therefore the library
> does not inspect ValueTypes attribute.
> 
> VarHandleObjects.ValueFieldReadOnly and ValueFieldReadWrite classes
> may be better to rename to FlatValueFieldReadOnly and
> FlatValueFieldReadWrite to be explicit.  I can do it another day.
> 
> Comments inlined below.
> 
> On 7/5/18 11:29 AM, Paul Sandoz wrote:
>> Hi Mandy,
>> It looks reasonable as an initial solution but i think the right long
>> term solution should be for Class.cast to fail. Perhaps that is
>> already tracked under a different issue?
> 
> As we chatted offline, a non-flattenable field of a value type does not
> need the null check.  For example C.f of type T and T is not value type
> when compiled and hence C.f is not flattenable.  T can be combined later
> separately as a value type.  In that case, C.f is nullable.
> 
>> Both the VarHandle code and the LF code perform cast checks for refs
>> so we should be able to piggy back off them for values without an
>> explicit null check.
> 
> That's a good suggestion.  I added ValueAccessor and StaticValueAccessor
> subclass of the ref accessor class to override checkCast method.  No
> need to change LF code.
> 
> 
>> s/ensureImmutable/ensureNonNullable/ 
> 
> It's checking immutability that throws IAE (UOE for VarHandle case).
> I changed the test to the field to non-null that may help avoid the
> confusion.
> 
>> s/ensureNullableOrNot/ensureNullable/
> 
> +1
> 
>> ? AFAICT you are not checking finality of a value type held in a ref
>> but the the assignment of null.
> 
> I'll add that.
> 
> Mandy