feat: Terminating analysis

[KemalBekir]

This PR solves: This issue

• Added test for positive and negative outcomes
• Implements the terminating description from the Go spec:
• Analysis performed during preprocessing

[petar-dambovaliev]

It looks good.
Some of the tests are failing because the code doesn't seem to cover the case where functions are external and injected in the VM.

Like strconv/strconv.go

package strconv

func Itoa(n int) string        

You need to identify external functions and not do analysis on them.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.26%. Comparing base (05b8afc) to head (1605300).
Report is 52 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1750      +/-   ##
==========================================
- Coverage   47.44%   45.26%   -2.19%     
==========================================
  Files         384      464      +80     
  Lines       61220    68515    +7295     
==========================================
+ Hits        29045    31010    +1965     
- Misses      29745    34902    +5157     
- Partials     2430     2603     +173     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[petar-dambovaliev]

300 lines (without the tests). I like it.

[mvertes]

I agree that the terminating statement analysis proposed here is complying to the Go specification and seems correct in its implementation.

However I'm not sure that it is the best response (or at least not sufficient) to the class of errors we want to address here: missing or wrong return statements.

We should have instead a check of each return statement to be compliant with the function signature (including the special case of pre-declared returned values allowing an empty return), and the detection of a missing return at the end of function body.

This check will still be necessary, even if we implement termination analysis.

If we have this check, then termination analysis becomes much less useful, except to catch the possibility of skipping a return if all the previous branches have one, which is just an optimisation.

To me the gain of termination analysis in this context is marginal. I don't know if it is justified in the context of an interpreter. Or maybe I missed something ?

[petar-dambovaliev]

I agree that the terminating statement analysis proposed here is complying to the Go specification and seems correct in its implementation.

However I'm not sure that it is the best response (or at least not sufficient) to the class of errors we want to address here: missing or wrong return statements.

We should have instead a check of each return statement to be compliant with the function signature (including the special case of pre-declared returned values allowing an empty return),

This is exactly what this PR does
and the detection of a missing return at the end of function body.

This check will still be necessary, even if we implement termination analysis.

If we have this check, then termination analysis becomes much less useful, except to catch the possibility of skipping a return if all the previous branches have one, which is just an optimisation.

To me the gain of termination analysis in this context is marginal. I don't know if it is justified in the context of an interpreter. Or maybe I missed something ?

The idea of this was to catch bad code like

func foo(b bool) int {
   if b {
     return 1
  }
}

Which corrupts the VM stack.
Whether the return statement returns the correct type or not is up to the type checker.
The only job of the termination analysis is to make sure there is a return statement where one is required.
This is essentially what you said the detection of a missing return at the end of function body.
The other stuff that you referred to is already implemented in the type checking code.

[mvertes]

After discussing and thinking more, I agree that your solution is the way to go 👍 .

Could you add also the following test which should pass ?

func f(a int) int {
    switch a {
    case 1:
        return 1
    default:
        return 0
    }
    // no return needed here
} 

Also the same for if:

func f(a int) int {
    if a > 0 {
        return 1
    } else {
        return 0
    }
    // no return needed
}

[zivkovicmilos]

Thank you for the contribution 🙏

I've left some comments that should be addressed before we go ahead and merge -- they're mostly style related, but also address some sketchy parts of the PR

[zivkovicmilos]

Thank you for resolving the comments 🙏

I've just left a single point about a typo, otherwise we are good to go 🚀

[michelleellen]

@KemalBekir we host a weekly developer call and would love to have you join if you're interested. Are you also on our Discord channe?