Acknowledged
5 months ago

SDK 8.0.0 beta: issue with conditionals from 7.4.1 is still present

There's a problem in early SDK 7.4.* versions (that have been pulled back) and in SDK 8.0.0 beta with enabled strict type checking:

import Toybox.Lang;

function f(x as Number) as Number? {
    // Object of type 'PolyType<Any or $.Toybox.Lang.Number>' does not match return type 'PolyType<Null or $.Toybox.Lang.Number>'.
    return x != null ? x : null;
}

Such code builds without errors in 7.3.1 and 7.4.3.

9811827
  • 5 months ago

    And even if the code was incorrect (due to comparing a non-nullable type to null), that wouldn't explain/excuse the behaviour reported in OP (that the type of the return value is subsequently assumed to be Any or Number.)

  • 5 months ago

    > But this code is incorrect IMHO. x should be declared as Number?, and it should also give you an error/warning about that (ok, that is maybe not garmin but only Prettier Monkey C)

    But it's not incorrect code in Monkey C (by definition, since it builds fine in 7.3.1 and 7.4.3, assuming that's intended behavior). Perhaps you mean that it *should* be incorrect (in your opinion? Or that it's incorrect code in PMC's judgement?

    Incidentally, this type of thing (comparing a non-nullable type to null) is not an error/warning in TypeScript either. (I mention this since Monkey Types clearly takes a lot of inspiration from TypeScript, and it's also interesting to see how this sort of thing is handled by a well-established language.)

    https://github.com/microsoft/TypeScript/issues/11920

    > disallow comparing to null and undefined unless they are valid cases in strict null mode #11920

    >> We intentionally allow this for the sake of defensive programming (i.e. defending against missing inputs from non-TS code). If there's enough demand we could add a flag or something.

    \

    To be fair, many of the commenters disagree with this behaviour.

    I can see a similar rationale for CIQ / Monkey C. It regularly comes up in forum discussions that many devs will write defensive code where they check various API fields for null values, even when the fields are typed to be non-nullable. In many cases, these checks may be superfluous, but in some cases they may actually be helpful (to cover the case when the field is actually null, due to either a bug in the implementation or a bug in the API documentation / type specification.)

    Isn't there a known issue where getHeartRateZones() can return null even though it's typed to not return null? It's been mentioned more than once.

    If Monkey Types actually enforced a rule where it's an error or warning to compare a non-nullable type to null, then that could break lots of existing apps (meaning the compiler would start returning warnings or errors for previously valid code), and actually lead to more bugs at runtime (in the case where a seemingly superfluous defensive null check would actually avoid a bug).

  • 5 months ago

    This (not exactly, but logically very similar) code is produced by PMC, but I think, this is not a PMC's problem.

    I have a function in barrel, that takes nullable argument and two apps, that use this barrel, and one app can pass null, while another one can't. PMC optimizes code of the second app in the way, that it passes non-nullable variable "x" in "x != null ? y(x) : null" expression. Yes, ideally we would expect PMC to optimize code even more to return "y(x)" directly, but I'm OK with current optimizations for now.

    In other words: yes, this code looks bad, but it was generated, and I think it should build and work as it syntactically correct.

  • 5 months ago

    But this code is incorrect IMHO. x should be declared as Number?, and it should also give you an error/warning about that (ok, that is maybe not garmin but only Prettier Monkey C)