• Replies 38 replies
  • Subscribers 25 subscribers
  • Views 2559 views
  • Users 0 members are here
Options
Related

Odd compiler errors with SDK 7.4.1

dbrobert
1 month ago

I'm aware of the previous export problem with SDK 7.4.0, and while my issue below may be a duplicate of this post but I wanted to start a new thread with a more appropriate thread subject/title.

I was compiling and running locally (in the sim) one of my apps just fine with SDK 7.3.1, however when I downloaded SDK 7.4.1 the unchanged code was reporting 6 new errors.

Two of the errors were "Statement is not reachable" errors, which I easily tracked down to functions not marked as potentially returning Null.

The other errors, however, are very strange and are contained with the onTap() function of an InputDelegate.

var coords = clickEvent.getCoordinates();
for (var i=1; i <= 12; i++) {
  // do whatever
  var drawable = _view.findDrawableById("someID");
  if ( != null) {
    // do stuff
    break;
  }
}
The above code generates two errors:
  • Trying to access an uninitialized variable.
  • Cannot perform operation 'add' on types 'Uninitialized' and '$.Toybox.Lang.Number'.

I've isolated this down to the i++ portion of the for-loop and don't see how I can possibly work around this. I have tried changing it to a while-loop, moving the code location, changed variable names, defined the counter/index outside of the loop, changed the way the addition is done, etc but the end result is always the same errors.

Top Replies

All Replies

  • 30 days ago in reply to myaro

    I don‘t know, if I got you right… but the compiler does not error when building the programm if typecheck = off. And the app runs on devices wirthout error. Only Typecheck = default does say it‘s unreachable.

  • 30 days ago in reply to myaro
    myaro said:

    The documentation directly supports my assertion that we should write defensive code and the compiler can't check all issues:

    Monkey C is a duck typed language.... 

    The Monkey C compiler does not verify type safety, however, and instead runtime errors occur when functions mishandle objects. This provides great flexibility to developers, but in exchange the compiler cannot perform static type checking like in C++ or Java. Using operators like instanceof and has can help avoid potential typing issues.

    That part of the doc was written before Monkey Types was introduced. Obviously it's absolutely incorrect to (now) say that the compiler cannot perform static type checking, as that's exactly what Monkey Types is supposed to do.

    I think it should actually be rewritten, especially since Garmin wants devs to use Monkey Types by default.

    It would be like someone writing TypeScript documentation and claiming that types can't be checked statically because the underlying runtime is JavaScript. No, the whole point of the additional type checking layer is to check types statically.

    myaro said:
    The SDK cannot in one place say "We don't guarantee types are checked"

    To further my point, I do not interpret the docs you quoted as Garmin saying type checking may not work properly (to paraphrase what you said). (Whether it does or does not in reality is a different argument.)

    I think Garmin absolutely intends for type checking to work properly, and they intend for the API types defined in their SDK to be specified properly. If there's a problem with the return type of getHeartRateZones() (for example), then it should be fixed.

    As matter of fact, there have been issues with incorrect types in the SDK which were later fixed.

    A mistake in specifying the API types in the type checking system is just as bad as the same mistake in the docs (both are probably generated from the same source). Even if you don't use type checking, you may be following the docs religiously (as opposed to coding defensively), meaning you may not apply a null check to a field which is not documented to possibly have a null value.

    I agree that in the real world, it's a good idea to code defensively, and if you choose to add redundant null checks (for example), the compiler shouldn't return a hard error. At best it should be a warning.

    However, there's always the possibility that if optimization is enabled, such code (which the compiler thinks is redundant / unreachable) would be optimized away in any case. So removing the error (without replacing it with a warning) might lead to an even worse situation where your code doesn't run at all.

    Something similar has already happened with the change to evaluate has checks related to API features at compile time.

    This means that if your device is defined to have API version X at compile time, then any has checks for features that requires API version Y (where Y > X) will be silently removed, even if it's possible that the device could be upgraded to API Y in the future. This is obviously a problem, so Garmin introduced a flag to disable this behavior (--disable-api-has-check-removal). I think this flag should actually be the default behavior though, and devs should have to opt in to enabling "api has check removal".

  • 30 days ago in reply to FlowState
    FlowState said:
    I agree that in the real world, it's a good idea to code defensively, and if you choose to add redundant null checks (for example), the compiler shouldn't return a hard error. At best it should be a warning.

    We're in total agreement on all points. I'm happy to see progress towards better type support.

    This quote is my main point -- "redundant" null / type checks shouldn't be fatal errors.  

    Trust but verify.

  • 30 days ago in reply to myaro
    myaro said:
    "redundant" null / type checks

    There is an alternate school of thought which says that it's actually better to avoid things like redundant null checks, because they only cause the underlying problem (an unexpected null) to propagated further down the line, and they mask a real error. In this line of thinking, it's better for apps to crash as early as possible (so this kind of error can be caught in testing).

    That's the opposite extreme though, and obviously it requires thorough testing. (Which is especially tough in Garmin's ecosystem of ~150 devices that support CIQ.)

  • 30 days ago in reply to FlowState

    This makes no sense, like the error message. Unreachable code should be a warning, not an error. 

    The point that most of the commenters missed is that we're not talking about how to optimize our code! We're talking about how to prevent crashes (and unhappy users) that are caused by a Garmin bug that is out of our control and we can't fix (and in many cases practically Garmin can't either) Fixing incorrect SDK documentation is probably the only thing we can realistically expect to happen. Fixing buggy firmware is less likely to happen, and even when it happens it won't reach all devices.

  • 30 days ago in reply to flocsy🤠
    flocsy🤠 said:
    This makes no sense, like the error message. Unreachable code should be a warning, not an error. 

    Unreachable code is being made a warning in an upcoming release. 7.4.2 was just released but does not contain this change. 7.4.2 does roll back the patch that caused the majority of the issues in this thread though, making it more like 7.3.1 in terms of the unreachable code error.

  • 30 days ago in reply to flocsy🤠
    flocsy🤠 said:
    This makes no sense, like the error message.

    It's a real school of thought that code should fail immediately when certain assumptions (such as x is never null) are violated, rather than hiding the error. I didn't say it was appropriate it in all cases. Sorry you don't think it makes sense.

    This is just a reddit discussion but I've seen the the same sentiment repeated more formally (e.g. in blogs from well-regarded devs working for Microsoft or in the linux kernel):

    www.reddit.com/.../

    > Should excessive null checking be avoided?

    >> I think the message here is “don’t spend effort trying to cover up errors”

    >> If it’s going to crash, you want it to crash loudly and verbosely. Covering up errors only causes headache down the road.

    It's actually the same philosophy behind using assert() statements which crash the program in debug mode, but are silently removed for release. Obviously this only works if you can test your program thoroughly to valid all of these assumptions.

    Again, I don't think it's necessarily this necessarily applies to CIQ development, especially since it's usually not feasible to test on all the real devices.

    And it probably doesn't apply to the return value of something like getHeartRateZones[], assuming that's sensible for a user to have no zones, which it might be, even if it rarely happens in practice.

    flocsy🤠 said:
    Unreachable code should be a warning, not an error. 

    I agree.

  • 30 days ago in reply to FlowState
    flocsy🤠 said:
    The point that most of the commenters missed is that we're not talking about how to optimize our code!

    I didn't miss this point. My point is that if the compiler thinks certain code is unreachable, who's to say the compiler wouldn't also optimize said code out of existence if optimization is enabled?

    e.g.

    function foo(x as String) {
      if (!(x instanceof String)) { // sanity check
        // vital code to handle unexpected case
      } else {
        // normal code
      }
    }

    If type checking is enabled, how do we know that the true branch of the if statement (where x is not a string) won't be optimized out of existence, thereby defeating the purpose of the sanity check?