Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: compiler does vet things #54019

Closed
go101 opened this issue Jul 23, 2022 · 8 comments
Closed

cmd/compile: compiler does vet things #54019

go101 opened this issue Jul 23, 2022 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge

Comments

@go101
Copy link

go101 commented Jul 23, 2022

What version of Go are you using (go version)?

$ go version
go version go1.19rc2 linux/amd64

Also for many previous versions.

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

func main() {}

func foo() (err error) {
	{
		err := bar()
		if err != nil {
			return
		}
	}
	
	return
}

func bar() error {
	return nil
}

What did you expect to see?

Complies okay.

What did you see instead?

Fails to compile.

$ go run main.go
# command-line-arguments
./main.go:9:4: result parameter err not in scope at return
	./main.go:7:3: inner declaration of var err error
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 23, 2022
@seankhliao
Copy link
Member

Duplicate of #5425

https://go.dev/ref/spec#:~:text=functions%20are%20executed.-,Implementation%20restriction%3A,-A%20compiler%20may

Implementation restriction: A compiler may disallow an empty expression list in a "return" statement if a different entity (constant, type, or variable) with the same name as a result parameter is in scope at the place of the return.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Jul 23, 2022
@go101
Copy link
Author

go101 commented Jul 24, 2022

This breaks expectations and does prevent some valid code from compiling.

@go101
Copy link
Author

go101 commented Oct 9, 2022

I plead to reopen this issue, for three reasons:

  1. go vet is also able to catch this.
  2. gc doesn't catch all similar such cases.
  3. I recently wrote several such (expected) valid code, but which is rejected by gc. The following is a simplified example (there are more expected-valid variants):
package main

import "fmt"

func g() (int, error) {
	return 0, fmt.Errorf("not implemented")
}

func f() (err error) {
	if n, err := g(); err == nil {
		return
	} else {
		return fmt.Errorf("%w: %d", err, n)
	}
}

func main() {
	fmt.Println(f())
}

Just let compilers do compiler things is a good philosophy.

@griesemer @ianlancetaylor

@ianlancetaylor
Copy link
Contributor

@go101 As @seankhliao said, the language spec explicitly calls this out as an implementation restriction. The implementation restriction was added to the tools in https://go.dev/cl/5245056. That predates the proposal process; it was part of the set of changes made for the release of Go 1. You can see it mentioned at https://docs.google.com/document/pub?id=1ny8uI-_BHrDCZv_zNBSthNKAMX_fR_0dc6epA6lztRE which is linked from the blog entry https://go.dev/blog/go1-preview. I don't think there is any new information since then, so I don't think we are going to change that decision.

@go101
Copy link
Author

go101 commented Oct 10, 2022

The problem is the restriction might do false positive things, which is okay for govet, but is weird for go compilers.

Personally, I don't get why this warning is any more harmful than the other warnings reported by govet. So I don't understand why it is promoted to the compiler level.

there is any new information since then

If this breaks user expectations and causes unhappy user experiences is not new information ...

@go101
Copy link
Author

go101 commented Oct 10, 2022

BTW, there is a poll: https://twitter.com/go100and1/status/1578997940487876608, which shows 2/3 voters think it should compile.

@ianlancetaylor
Copy link
Contributor

The compiler gives an error on this code because we observed that people thought they could use a naked return to return an error, and didn't realize that the named err result parameter was being shadowed by a locally declared err variable. People were writing that kind of code in practice, but it was never what they wanted.

It's true that we might make a different decision today, but I think that given that the compiler has acted this way for 10 years, we need a really strong reason to change it. And "weird for Go compilers" is a valid reason, but it's not a strong reason.

@go101
Copy link
Author

go101 commented Oct 11, 2022

we observed that people thought they could us .... but it was never what they wanted.

This is also true for many cases reported by vet.

And this decision was made before 1.0, so I doubt the validity of the reason. At least for me, it is not true.

Personally, I think this is a “draw legs on a snake” thing, because it breaks user expectations, which is a strong enough reason.

@golang golang locked and limited conversation to collaborators Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

4 participants