spec: define when return is necessary #65

Closed
gopherbot opened this Issue Nov 11, 2009 · 27 comments

Comments

Projects
None yet
8 participants

by bob.appleyard:

func example(x int) int {
    if x == 0 {
        return 5;
    } else {
        return x;
    }
}

Gives the error "function ends without a return statement"

Changing it to:

func example(x int) int {
    if x == 0 {
        return 5;
    } else {
        return x;
    }
    panic("unreachable");
}

Solves the problem, however this is quite clearly a workaround.


GOOS=linux
GOARCH=amd64

Local revision:

changeset:   3988:b773b8255a8f
tag:         tip
user:        Russ Cox <rsc@golang.org>
date:        Wed Nov 11 13:08:35 2009 -0800
summary:     avoid clash with stdio's getc, ungetc.
Contributor

agl commented Nov 11, 2009

Comment 1:

We've been living with this bug for a long time, but it's good to have a bug entry for 
it. Thanks.

Labels changed: added compiler-bug.

Status changed to Accepted.

Contributor

rsc commented Nov 11, 2009

Comment 2:

Labels changed: added language-change, removed compiler-bug.

Contributor

rsc commented Nov 11, 2009

Comment 3:

Status changed to Thinking.

Comment 4 by amdragon:

I implemented proper return flow checking in the interpreter and wrote down the set of 
rules that it followed.  They were the obvious rules, but somewhat difficult to state 
concretely.  I can dig up those rules, if you want.

Comment 5 by joepeck02:

Just noting, it is also necessary for switch statements:
  func example(x int) int {
    switch {
      case x==0: return 5;
      default: return x;
    }
  }
Cheers on the progress!

Comment 6 by ibw@isaacwagner.me:

Though if you look through the Effective Go documentation, it is suggested that you 
don't code with those "guaranteed" else's. I agree that it should be resolved, but I 
think that leaving it the way it is may help everyone get into the habit of coding 
func example(x int) int {
    if x == 0 {
        return 5;
    }
    return x;
}
as opposed to
func example(x int) int {
    if x == 0 {
        return 5;
    } else {
        return x;
    }
}
I personally think the first one is cleaner and more elegant.
Contributor

rsc commented Dec 2, 2009

Comment 7:

Labels changed: added languagechange, removed language-change.

Comment 8 by bob.appleyard:

#6 may satisfy a certain taste, but I would be more concerned about anyone coming to
read the code later. In toy examples this is not of much concern, but if this were a
big function, that the if statement returns before the rest of the body would be less
than obvious. Some poor maintainer reading this elegant code will have to check
everything in the if statement, when it would be better if they could just forget it
and fix what needs to be fixed.
I hope #4 has been noticed.
Contributor

rsc commented Jan 7, 2010

Comment 9:

For the log: this is a well-known issue we plan to address, but there are
higher-priority 
issues right now.   There's no need to try to persuade us that it's a problem.
For what it's worth, #6 is certainly the preferred style for that particular code, but
the 
problem comes up in loops that don't break, etc.  Especially in a big function, having 
the if statement return early on helps keep the code in the function from creeping to 
the right.
Contributor

rsc commented Apr 27, 2010

Comment 10:

Status changed to LongTerm.

Contributor

rsc commented Apr 8, 2011

Comment 11:

Issue #1679 has been merged into this issue.

Contributor

rsc commented Jun 16, 2011

Comment 12:

Actually, the spec appears not to define what the behavior here is.

Owner changed to @griesemer.

Contributor

robpike commented Jul 3, 2011

Comment 13:

Issue #2034 has been merged into this issue.

Comment 14 by Damir.Azdajic:

Won't you have to check for unused code blocks in the optimizer stage anyways?
So why not delay the return check to the optimizer stage and deal with it there?
Take this modified example, which also outputs the same return error in GoLang:
func example(x int) int {
    return 19; //Good for debugging, bad if one forgets about it
    
//But should the following code be included in the final binary since it's unreachable?
//If the answer is to exclude the code, then it's more of a reason to postpone return
checking to the optimizer stage
    if x == 0 {
        return 5;
    } else {
        return x;
    }
}
First when generating the intermediate code for each function, automatically append a
return at the end of the function upon hitting the last }.
Then in the optimizing stage, for every intermediate opcode have a flag denoting
traversed or not traversed,
and run through the code taking both paths. If a branch tries to go to an already
traversed location, hits a return, or break, then the end of that path has been found.
Let's assume the intermediate code looks like so:
function example
..input x int
..output t0 int
..t0 = 0 #Spec states function returns are initiated to 0 on function entry
..t0 = 19 #Carry out our return (obviously the t0 = 0 would get ditched once optimized)
..return
..cmp x, 0
..bne label0
..t0 = 5
..return
..bra label1 #Automatically added by if/else block generation - but rendered useless
because of the "return" above
label0:
..t0 = x
..return
label1:
..return #The automatically appended return at end of function, done by the intermediate
code generation stage
And once we run through it:
function example
..input x int
..output t0 int
..t0 = 0 (Traversed)
..t0 = 19 (Traversed)
..return (Traversed)
..cmp x, 0 (Untraversed)
..bne label0 (Untraversed)
..t0 = 5 (Untraversed)
..return (Untraversed)
..bra label1 (Untraversed)
label0:
..t0 = x (Untraversed)
..return (Untraversed)
label1:
..return (Untraversed)
Then we drop all the untraversed lines and end up with this:
function example
..input x int
..output t0 int
..t0 = 0 (Traversed) #Once the symbols are optomized, this line should be dropped
obviously
..t0 = 19 (Traversed)
..return (Traversed) #Return is last
But let's say the function was this:
func example(x int) int {
    if x == 0 {
        return 5;
    } else {
        return x;
    }
}
So the intermediate code would be this:
function example
..input x int
..output t0 int
..t0 = 0 #Spec states function returns are initiated to 0 on function entry
..cmp x, 0
..bne label0
..t0 = 5
..return
..bra label1 #Automatically added by if/else block generation - but rendered useless
because of the "return" above
label0:
..t0 = x
..return
label1:
..return #The automatically appended return at end of function, done by the intermediate
code generation stage
Then in the optimizing stage, when traversing we get:
function example
..input x int
..output t0 int
..t0 = 0 (Traversed 1)
..cmp x, 0 (Traversed 2 )
..bne label0 (Traversed 3) #both branches taken
..t0 = 5 (Traversed 3.a.1)
..return (Traversed 3.a.2)
..bra label1 (Untraversed)
label0:
..t0 = x (Traversed 3.b.1)
..return (Traversed 3.b.1)
label1:
..return (Untraversed)
Drop the untraversed lines:
function example
..input x int
..output t0 int
..t0 = 0 (Traversed 1)
..cmp x, 0 (Traversed 2 )
..bne label0 (Traversed 3) #both branches taken
..t0 = 5 (Traversed 3.a.1)
..return (Traversed 3.a.2)
label0:
..t0 = x (Traversed 3.b.1)
..return (Traversed 3.b.1) #Return is last, okay
Furthermore, I think the compiler should not treat this as an full-stop-error, but
instead as a warning.
The spec already claims all return values are declared to 0 upon entry of the function.
So the lack of an explicit return only implies a defined state - a return of 0
Contributor

adg commented Oct 19, 2011

Comment 15:

> Furthermore, I think the compiler should not treat this as an full-stop-error, but
instead as a warning.
The Go compiler doesn't issue warnings.
Contributor

rsc commented Oct 19, 2011

Comment 16:

The goal here is to maintain consistency across the compilers.
If you delay return checking to an "optimizer stage", then you
inherently get different compile errors depending on your
optimization settings, and that's not consistent.
The rule here is trivial: the function must end with a
return statement or a panic statement or have no
return values.
Contributor

rsc commented Dec 9, 2011

Comment 17:

Labels changed: added priority-later.

Contributor

ianlancetaylor commented Aug 20, 2012

Comment 18:

Issue #3979 has been merged into this issue.

Owner

bradfitz commented Aug 22, 2012

Comment 19:

I was just refactoring some code and was surprised when some code similar to this
accidentally compiled:
$ cat return.go 
package main
func foo() int {
    {
        return 5
    }
}
func main() {
    println(foo())
}
$ go run return.go 
5
$  go version
go version weekly.2012-03-27 +ef432c94ce9c
Contributor

rsc commented Sep 12, 2012

Comment 20:

Labels changed: added go1.1maybe.

Comment 22 by nickretallack:

I agree with #14.
Honestly, I find it frustrating that the compiler stops due to this error.  This, as
well as the unused variable errors, should not be errors.  The compiler should be smart
enough to strip these out and compile the program anyway, since it makes exploratory
debugging much easier.  There is really no reason for the compiler to be so picky.  It
can issue warnings.
And it full on stops even in situations when it is wrong, like this one.  That's
especially frustrating.
I find this code to be more consistent and readable than the other:
 
    if x == 0 {
        return 5;
    } else {
        return x;
    }

Comment 23 by Demura.Igor:

Somebody said that style
  if x == 0 {
    return 5
  }
  return x
is more safe and elegant, but I don't agree, I think with `else` I can:
be sure that control from `true` branch never reaches the `false` branch and also is
more natural. How do you read this code? "return 5 if x == 0 otherwise x" and not like 
"return 5 if x == 0 return 0":) Please fix it.
Contributor

rsc commented Nov 26, 2012

Comment 24:

Labels changed: added restrict-addissuecomment-commit.

Contributor

rsc commented Dec 10, 2012

Comment 25:

Labels changed: added size-xl.

Contributor

rsc commented Mar 1, 2013

Comment 26:

The current plan is http://golang.org/s/go11return.
Contributor

griesemer commented Mar 4, 2013

Comment 27:

This issue was closed by revision 9905cec.

Status changed to Fixed.

Contributor

rsc commented Mar 4, 2013

Comment 28:

This issue was closed by revision ecab408.

griesemer was assigned by gopherbot Mar 4, 2013

adg locked and limited conversation to collaborators Dec 8, 2014

@minux minux pushed a commit to minux/goios that referenced this issue Feb 27, 2015

@4ad 4ad liblink, cmd/7a: add C_VCONADDR class
This change adds the C_VCONADDR representing $r(SB), the address of a
relocatable symbol. The old C_ADDR, r(SB) without $, means data at a
relocatable symbol.

C_VCONADDR matches C_VCON in optab. We could have overloaded the meaning
of C_VCON, but adding a new class simplifies the logic in addpool.

Fixes #65
076d34d

rsc added this to the Go1.1 milestone Apr 14, 2015

rsc removed the go1.1maybe label Apr 14, 2015

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.