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: suppress typecheck errors in a type switch case with broken type #28926

Closed
josharian opened this Issue Nov 23, 2018 · 19 comments

Comments

Projects
None yet
4 participants
@josharian
Copy link
Contributor

josharian commented Nov 23, 2018

package p

func f(e interface{}) {
	switch e := e.(type) {
	case T:
		e.M()
	}
}
$ go tool compile x.go
x.go:5:7: undefined: T
x.go:6:4: e.M undefined (type interface {} is interface with no methods)

The first error message is accurate and helpful. We should suppress the second error message, which is confusing.

@josharian josharian added the NeedsFix label Nov 23, 2018

@josharian josharian added this to the Go1.13 milestone Nov 23, 2018

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Nov 23, 2018

I call dibs. 🙂

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Nov 24, 2018

@josharian Hi. Just to be clear. This happens in select when the selected type upon does not exist. And the compiler still continues even though it should in the case of that case just stop there since the type doesn't exist. Is this assessment right?

@josharian

This comment has been minimized.

Copy link
Contributor Author

josharian commented Nov 24, 2018

This happens in select when the selected type upon does not exist.

Yes.

And the compiler still continues even though it should in the case of that case just stop there since the type doesn't exist.

The compiler continues (for a while) past many errors, since it is often helpful to print out multiple errors instead of the first one. What is needed here is to either mark the body of the case as typechecked without actually typechecking it, or mark the original type of e with Broke() so no further errors involving e get reported. Neither is perfect. With the former, if there are multiple missing type cases they will all get errors reported; with the latter, unrelated typechecking errors in the body will get reported. But this isn’t mission critical, so probably do whatever is cleanest in the code. And don’t forget to add a test—see “errorcheck” style tests in GOROOT/test.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Nov 24, 2018

Thanks a lot for the information and the reminder on the test! Much appreciated! 👍

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Nov 25, 2018

@josharian Hi. I have an idea. A vague one though and I would like to ask if that is an option at all.

So I was thinking that this error is happening because the type is non existent in the type switch. So the back assignment fails to happen to the value. What if I could somehow mark that happening an once the compiler continues it's thing it can detect that the type assert back then actually failed and that it should tell you so when it's inside the case statement. Something like:

$ go tool compile x.go
x.go:5:7: undefined: T
x.go:6:4: type assert failed for this case leg. this type is of type interface thus has no method called `M`.

Or... something in this regard. What say you?

That way, compilation could continue and the error wouldn't be meaningless.

@josharian

This comment has been minimized.

Copy link
Contributor Author

josharian commented Nov 25, 2018

I think it’d be simpler to just suppress the second error and sufficiently informative.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Nov 25, 2018

Ack

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Nov 26, 2018

@josharian so... if I do this:

			yyerrorl(lineno, "undefined: %v", n.Sym)
			n.Type.SetBroke(true)

It will stop the compiler dead in its track. Nothing further will be reported. This fixes the current issue, but like you said, it's not very ideal.

~/Temp/go/typecheck
❯ go tool compile main.go
main.go:6:7: undefined: T

I have to read the code a bit further to see if anything else is actually possible. Like, I can't just mark the case as errored because nothing is actually returned. And because of switching on interface types is allowed, I can't even use that.

Also, am I even in the right place siwthc swt.go under compile/gc? I think that's the right place to poke at things. But it's actually not that easy to debug.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Nov 26, 2018

Oh! This might help..

		typecheckdef(n)
		if n.Op == ONONAME {
			n.Type = nil
			return n
		}

So if ONONAME is detected, it will return nil. That's interesting. I think I can work out something with that.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 27, 2018

Change https://golang.org/cl/151323 mentions this issue: cmd/compile: suppress typecheck errors in a type switch case with broken type

Skarlso added a commit to Skarlso/go that referenced this issue Dec 19, 2018

cmd/compile: suppress typecheck errors in a type switch case with bro…
…ken type

Once a switch's case broke with a type check error all further checks in the case's body should be suppressed. Checking should continue normally after the broken case's body is passed.

Fixes: golang#28926

Change-Id: I936fcf784c7df15cd39fdc1e43f2c425304f417e
GitHub-Last-Rev: 03c62d5
GitHub-Pull-Request: golang#28965
@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Jan 11, 2019

@josharian ping. Hi. :-) would you please mind taking a look at the CL and see if you are satisfied with my solution? At your earliest convenience. Thanks! :-)

@josharian

This comment has been minimized.

Copy link
Contributor Author

josharian commented Jan 11, 2019

Thanks for checking in. This fell off my radar. I am unlikely to look before early next week. If you haven’t heard from me by late next week, please do ping again.

@Skarlso

This comment was marked as off-topic.

Copy link
Contributor

Skarlso commented Jan 17, 2019

@josharian pinging for the glory of the Sontaran Empire.
scan 555_fotor-600x600

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 18, 2019

Change https://golang.org/cl/158617 mentions this issue: cmd/compile: suppress typecheck errors in a type switch case with broken type

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Jan 18, 2019

@josharian I'm so sorry, I had to open a new CL. :( Since my PR's fork died after the complete rebase the PR was orphaned. I tried to use the same change-id as @bradfitz suggested, but I kept ending up fighting the gerrit bot which threw me back to the previous version of my change set even though I abandoned the PR and did the steps outline in the Gerrit work flow.

Sincerest apologies for the trouble caused. :/

I addressed your comments in the new CL here: https://go-review.googlesource.com/c/go/+/158617

@josharian

This comment has been minimized.

Copy link
Contributor Author

josharian commented Jan 18, 2019

I'm so sorry, I had to open a new CL.

No prob. Please abandon the old CL when you get a chance.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Jan 18, 2019

Yeah I don't have permission to abandon the other one cause it was created through a PR I guess.

@mdempsky

This comment has been minimized.

Copy link
Member

mdempsky commented Jan 19, 2019

Two related cases that might be worth looking into (not necessarily as part of CL 158617):

  1. "case 42" in a type switch is currently treated as "case int" (aside from emitting an error). It might be better to treat the case as broken, like we do "case doesnotexist:". For example:

     switch e := interface{}(nil).(type) {
     case 42: // want an error here
         e.M() // probably don't need an error here
     }
    

    The problem here is we're not checking ncase.List.First().Op == OTYPE before setting nvar.Type = ncase.List.First().Type.

  2. We should try to still typecheck broken case bodies, because there's possibly still code that doesn't depend on the type switch variable that we can produce meaningful errors about.

     switch e := interface{}(nil).(type) {
     case doesnotexist: // want an error here
         alsodoesnotexist() // an error here would be good too
     }
    

    Theoretically this should be as simple as just setting nvar.Type = nil and continuing with typechecking, but I couldn't immediately figure out how to prevent typecheckdef from giving false positive warnings about using .(type) outside of a switch statement.

@Skarlso

This comment has been minimized.

Copy link
Contributor

Skarlso commented Jan 19, 2019

@mdempsky Thanks, I'll extract it into a separate Issue! :)

@gopherbot gopherbot closed this in 8ee9bca Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.