Possible issues with extraReferenceTypeChecks

[m4tiz5zktonj]

I have some questions about extraReferenceTypeChecks option. Due to severe complexity of compiler theory and to the fact, that I have superficial knowledge there, I can't exactly tell whether code examples below are "not an issue" or not. To me they look like bugs, and I'm sorry for taking your time, if they aren't.

---

class Parent {}
class Child extends Parent {}

function returnChildTuple() as [Child] {
    return [new Child()];
}
function returnParentTuple1() as [Parent] {
    // OK
    return [new Child()];
}
function returnParentTuple2() as [Parent] {
    // Unsafe return from $.returnParentTuple2: converting [Child] to [Parent] is not type safe
    return returnChildTuple();
}

I'm seeing this since 2.0.114.

---

import Toybox.Lang;

function returnTupleAsArray(b as Boolean) as Array<Number> {
    // Unsafe return from $.returnTupleAsArray: converting [Number] or [] to Array<Number> is not type safe
    return b ? [0] : [];
}

Though some other code, that calls returnTupleAsArray, may break returned tuples' typing, the code in general should be safe, as return type of returnTupleAsArray is array, not tuple, and caller will treat result as array.

---

import Toybox.Lang;

var arrayOfNumbers as Array<Number> = [];

function updateArrayOfNumbers1() as Void {
    // OK
    arrayOfNumbers = [0];
}
function updateArrayOfNumbers2() as Void {
    var numberTuple = [0];

    // Unsafe assignment to arrayOfNumbers: assigning [Number] to Array<Number> is not type safe
    arrayOfNumbers = numberTuple;
}

I don't expect warning for updateArrayOfNumbers2, as this function is basically the same as updateArrayOfNumbers1.

---

import Toybox.Lang;

function updateArray1(array as Array<Number>, value1 as Number?, value2 as Number?) as Void {
    // Invalid assignment to array[0]. Expected Number but got Null or Number - OK
    array[0] = value1;

    // array is Array<Null or Number> now - OK

    // No warnings - OK
    array[1] = value2;
}

function updateArray2(array as Array<[Numeric]>, value1 as [Number]?, value2 as [Number]?) as Void {
    // Invalid assignment to array[0]. Expected [Number or Long or Float or Double] but got Null or [Number] - OK
    array[0] = value1;

    // array is Array<Null or [Numeric] or [Number]> now - OK

    // Unsafe assignment to array[1]: assigning Null or [Number] to Null or [Number or Long or Float or Double] or [Number] is not type safe
    array[1] = value2;
}

I've asked about no warnings for array[1] in updateArray1 code last week, and it was closed as "works as intended" (that is OK), but my real code is a bit more complex and looks more like updateArray2, and 2.0.114 introduces a warning for array[1], that I don't expect to see now, just like in updateArray1.

---

Thank you!

[markw65]

I'll split my response to cover your different examples in different comments.

The problem with returnParentTuple2() is that someone could change the returned tuple from a [Child] to a [Parent]. In your particular case, that doesn't matter, because that tuple is entirely owned by the caller of returnParentTuple2().

But consider:

class Parent {}
class Child extends Parent { function child_func() {} }
const childTuple as [Child] = [new Child()];
function returnChildTuple() as [Child] {
    return childTuple;
}
function returnParentTuple2() as [Parent] {
    // Unsafe return from $.returnParentTuple2: converting [Child] to [Parent] is not type safe
    return returnChildTuple();
}
function doStuff() {
  childTuple[0].child_func();
}
function bad() {
  var p = returnParentTuple2();
  p[0] = new Parent();
  doStuff(); // crash
}

The issue here is that currently, while analyzing returnParentTuple2 my type checker doesn't know whether returnChildTuple() is returning a brand new tuple with no other references (as in your example), or a possibly shared tuple, (as in mine). But that warning is the only thing that indicates a potential problem here.

I am planning to add more analysis, so that eg if a function only returns literal arrays, I mark it as being "safe", in the same way that currently literal arrays are considered "safe" (which is why returnParentTuple1() doesn't report an error).

In fact, that particular check is pretty straightforward. But I will also need to deal with cases like:

function returnParentTuple1() as [Parent] {
    var child = [new Child()];
    return child; // currently not ok
}

which will require yet more state to be tracked by the type analysis (essentially every use of child has to be checked to see if the value might "leak" somewhere).

[markw65]

    return b ? [0] : [];

This is an oversight on my part. As I said, I make an exception for literal expressions, because I know that nobody else has a reference. But I forgot about conditional expressions containing literals. That's an easy fix. I'll try to get a new version out soon that does that, and also deals with functions that only return "safe" expressions. The escape analysis might have to wait.

Edited to add:

as return type of returnTupleAsArray is array, not tuple, and caller will treat result as array

The return type being Array is the problem though; it means the caller will be able to change the shape of the tuple without any warnings. And again, in this case, the expression is guaranteed not to have escaped anywhere else, so no other code cares that it remains a tuple, so it's a non-issue.

The point is that I need to issue this warning any time I can't prove that the value didn't escape - and as I said above, this is an obvious case that I missed. But in general, no matter how good my analysis, there will always be cases where the value doesn't actually escape, but I can't prove it. Or where the value did escape, but the code it escaped to didn't care, etc.

[markw65]

I don't expect warning for updateArrayOfNumbers2, as this function is basically the same as updateArrayOfNumbers1

I touched on this in my first response. To make that work, I need to do some escape analysis to make sure that numberTuple doesn't leak. Meanwhile, an explicit cast to array will fix the issue...

[markw65]

function updateArray2(array as Array<[Numeric]>, value1 as [Number]?, value2 as [Number]?) as Void {
    // Invalid assignment to array[0]. Expected [Number or Long or Float or Double] but got Null or [Number] - OK
    array[0] = value1;

    // array is Array<Null or [Numeric] or [Number]> now - OK

    // Unsafe assignment to array[1]: assigning Null or [Number] to Null or [Number or Long or Float or Double] or [Number] is not type safe
    array[1] = value2;
}

This one can be simplified:

function updateArray2(array as Array<[Numeric]>, value as [Number]) as Void {
    array[0] = value2; // unsafe
}
function bad(array as Array<[Numeric]>) as [Number] {
  var value = [42];
  updateArray2(array, value);
  array[0][0] = 42.0;
  return value; // value now contains [42.0]
}

Basically, once you stick value into array it can be modified through array in ways that violate its type contract. This one is definitely "as intended".

To make this type safe, you could declare updateArray2 as

function updateArray2(array as Array<[Number] or [Float] or [Long] or [Double]>, value as [Number]) as Void {
    array[0] = value2; // safe
}

This one is safe because you can't assign anything to array[0][0] without a warning. Reading from array is pretty much the same as before - array[0][0] has type Numeric - but writing requires explicit casts (hopefully guarded by instanceof checks).

[m4tiz5zktonj]

To make this type safe, you could declare updateArray2 as

Just to give more context. I simplify my real code as much as possible (maybe, too much) to produce simple test case, that illustrates some issue. This particular function has another issue, that is reported here and is not fixed yet (I understand, it may never be fixed - I'm OK with it and got used to it). That's why updateArray1 and updateArray2 functions have strange argument typings: I don't try to put null to array of non-null elements, but analyzer doesn't understand that.

Here is little less simplified version of my function:

import Toybox.Graphics;
import Toybox.Lang;

function drawPolygons(dc as Dc, size1 as Number, size2 as Number, b as Boolean) as Void {
    var polygon = new Array<Graphics.Point2D>[size1];

    for (var i = 0; i < size2; i++) {
        var point1 = b ? polygon[size1 - 2] : null;
        var point2 = b ? polygon[size1 - 1] : null;

        if (b) {
            // "Invalid assignment to polygon[0]. Expected [Number or Long or Float or Double, Number or Long or Float or Double] or [Float, Float] but got Null or [Number or Long or Float or Double, Number or Long or Float or Double] or [Float, Float]" - OK for PMC without fix for code from #30 issue comment
            polygon[0] = point1;
            // Unsafe assignment to polygon[1]: assigning Null or [Number or Long or Float or Double, Number or Long or Float or Double] or [Float, Float] to Null or [Number or Long or Float or Double, Number or Long or Float or Double] or [Float, Float] is not type safe
            polygon[1] = point2;
        }

        polygon[3] = [0.0, 0.0];

        // "Argument 1 to $.Toybox.Graphics.Dc.fillPolygon expected to be Array<[Number or Long or Float or Double, Number or Long or Float or Double]> but got Array<Null or [Number or Long or Float or Double, Number or Long or Float or Double] or [Float, Float]>" - OK for PMC without fix for code from #30 issue comment
        dc.fillPolygon(polygon);
    }
}

I tried assigning empty array to point1 and point2 (with or without as Graphics.Point2D) instead of null, but I couldn't get it working. Besides, as we are fighting for having more available memory, I don't want fix it this way, so I just ignore it for now.

[markw65]

I understand that this is simplified code, but why not move the assignments under the if?

        if (b) {
            var point1 = polygon[size1 - 2];
            var point2 = polygon[size1 - 1];
            polygon[0] = point1;
            polygon[1] = point2;
        }

or if they're used after the if:

        var point1 = null;
        var point2 = null;
        if (b) {
            point1 = polygon[size1 - 2];
            point2 = polygon[size1 - 1];
            polygon[0] = point1;
            polygon[1] = point2;
        }

I will give some more thought to how I can infer that point1 and point2 are non-null based on b's truthiness, but it seems like it would take a pretty major reworking of the type checker.

[m4tiz5zktonj]

why not move the assignments under the if

I use polygon as circular buffer for drawing polygons, and point1 and point2 are the references to last and second to last used points. These extra references to the points may not be necessary, and b condition is "need points from previous polygon", that is calculated inside the loop. And there's another code between point1 and point2 assignment and usage for resizing polygon (creating a new array of different size) based on other condition, that doesn't depend on b. So, I can't join declaration (and/or assignment) and usage inside single if (b) {} check, as I need to put these points into possibly new polygon array.

[markw65]

Some of these have been mitigated in v2.0.115 of the extension. The rest will depend on implementing some form of escape analysis.