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: duplicate swtich case constant bool expressions are accepted by compiler #28357

Closed
go101 opened this Issue Oct 24, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@go101

go101 commented Oct 24, 2018

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

go version go1.11.1 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

package main

func main() {
	switch {
	case true:
	case true: // ok
	}
	
	switch 123 {
	case 1:
	case 1: // duplicate case 1 in switch
	}
	
	switch "abc" {
	case "a":
	case "a": // duplicate case "a" in switch
	}
}

What did you expect to see?

The first switch block should also compile error.

What did you see instead?

Compiler think the first switch block is valid.

BTW, gccgo also think the string switch is valid.

@go101

This comment has been minimized.

go101 commented Oct 24, 2018

Ah, looks this is mentioned in spec:

Implementation restriction: A compiler may disallow multiple case expressions evaluating to the same constant. For instance, the current compilers disallow duplicate integer, floating point, or string constants in case expressions.

But why boolean case expresssions are exceptions?

And does "the current compilers" includes gccgo, if true, can we think this is a bug of gccgo (for thinking the string switch is valid.)

@go101

This comment has been minimized.

go101 commented Oct 24, 2018

The following code also compiles failed by gc, but passed by gccgo.

package main

func main() {
	var a interface{}
	switch a {
	case nil:
	case nil: // duplicate case nil (value <nil>) in switch
	}
}

Is gc too restricted (than spec) here?

@go101

This comment has been minimized.

go101 commented Oct 24, 2018

And this

package main

func main() {
	var a int64
	switch a {
	case int64(123):
	case int32(123):
	}
}

gc reports:

./aa.go:7:2: invalid case int32(123) in switch on a (mismatched types int32 and int64)
./aa.go:7:2: duplicate case int32(123) (value 123) in switch
	previous case at ./aa.go:6:12

I think the second error message is not necessary. (gccgo does it right here)

@go101 go101 changed the title from cmd/compile: duplicate constant bool expressions are accepted by compiler to cmd/compile: duplicate swtich case constant bool expressions are accepted by compiler Oct 24, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented Oct 24, 2018

I'd imagine this is to allow switch statements of the form:

switch {
case someFunc():
case someVar:
case someConst:
case someOtherConst:
}

Two true constants in that switch statement may actually be useful. For example, they might both be true on Linux, but either of them might be false on another operating system via build tags.

Either way, @griesemer will know.

@bcmills bcmills added this to the Unplanned milestone Oct 24, 2018

@josharian

This comment has been minimized.

Contributor

josharian commented Oct 24, 2018

// Don't check for duplicate bools. Although the spec allows it,

@josharian

This comment has been minimized.

Contributor

josharian commented Oct 24, 2018

This is intentional, and we aren’t going to change it for the reasons given in the comment referenced above.

@josharian josharian closed this Oct 24, 2018

@go101

This comment has been minimized.

go101 commented Oct 24, 2018

then would it be better to also relax for other kinds of types?

@go101

This comment has been minimized.

go101 commented Oct 24, 2018

And can't

case GOARCH == "arm" && GOARM == "5":
case GOARCH == "arm":

be written as

case GOARCH == "arm":
    if GOARM == "5" {
       . ..
    } else {
        ...
    }

? Though I admit that the former is a little more graceful.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Oct 25, 2018

Technically, nil is not a constant, just a predeclared value. So gc is technically overeager per the current spec when complaining about duplicate nil cases. Though since nil is a singleton than can't change, it behaves like a constant and perhaps it should be in that list (in the spec). I filed #28378 for that.

More generally, "implementation restrictions" are really "language restrictions": If the primary compilers follow the implementation restrictions, it doesn't really matter if a Go program is valid per the spec; "correct" compilers may refuse them anyway. For all practical purposes, those programs are "incorrect". And it also does mean that all compilers really should follow the same implementation restriction fairly closely (unless they are considered "buggy").

We call these restrictions "implementation restrictions" because we're a bit too nervous to make them part of the language proper: Some of these rules are rather pragmatic (e.g., we ignore duplicate bools above), may change over time, and generally have a slightly "unsavory" smell to them compared to the rest of the spec. Also, some of the restrictions would put undue burden on the specifics of a particular implementation. For instance, the implementation of the compiler's constant arithmetic differs considerably between cmd/compile, gccgo, and go/types. The latter uses 100% exact arithmetic (rational numbers) unless the fractions become too huge, while the former two always use just floats with very large mantissas and exponents. It's easily possible to create programs that are 100% correct and accepted by go/types but refused by the compilers. And vice versa. It's not clear that we want to pin down the implementation enough such that such corner cases are not possible. It might be better to avoid such code. Software engineering is a real world thing and compromised by that real world.

@go101

This comment has been minimized.

go101 commented Oct 25, 2018

@griesemer thanks for the wonderful explanation. I agree totally.

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