Go: cmd/compile: untyped types reaching backend

[mdempsky]

I created CL 94027, which validates that the SSA backend never sees untyped types. This should be safe, because the compiler frontend should eliminate all untyped types during type checking.

However, the CL doesn't currently work, suggesting the frontend is doing something (at least technically) wrong.

[mdempsky]

Fixing this might be tricky, but an easy way to help would be to find some minimal test cases that reproduce failures.

1. Apply CL 94027. (Maybe change the two f.Fatalf calls to f.Logf, so you still have a working compiler.)
2. Run "go install cmd/compile" and then "go build -a std cmd".

3. Look in the output for lines like:

   ../src/strconv/atoi.go:160:15: internal compiler error: allocating value with type untyped bool
   

4. Figure out how to reproduce that error with as little code as possible. For example, this is a pretty minimal repro case (maybe it can be further simplified):

   package p
   
   func syntaxError(... interface{}) error
   
   func ParseUint(s string) (uint64, error) {
           if len(s) == 0 {
                   return 0, syntaxError("", s)
           }
           return 0, nil
   }
   

[namusyaka]

@mdempsky I don't know if I can solve this, but since I am interested, I'll try to investigate it.

[mdempsky]

@namusyaka That would be great, thanks! Feel free to ask here if you have any questions.

[fraenkel]

The example above is caused by the generated .eq.[2]interface{} method.

[mdempsky]

@fraenkel Thanks! So just

package p
type t [2]interface{}

repros that issue.

Further steps for folks interested in helping debug/fix this:

1. Run "go tool compile -W repro.go" and look for "untyped bool". (Make sure you're sync'd past c3e8da67dd79d84ac04187b5ce577d76d4e0c032, or else this will be printed as just "bool".)

2. The output is a bit tricky to understand if you're not familiar with the compiler's AST representation (see cmd/compile/internal/gc/syntax.go for hints), but you should be able to spot things like:

   . FOR l(1) colas(true) tc(1) ARRAY-[2]interface {}
   . . LT l(1) tc(1) untyped bool
   . . . NAME-p..autotmp_4 a(true) l(1) x(0) class(PAUTO) esc(N) tc(1) assigned used int
   . . . NAME-p..autotmp_5 a(true) l(1) x(0) class(PAUTO) esc(N) tc(1) assigned used int

   or

   . . IF l(1) tc(1)
   . . . OROR l(1) tc(1) hascall untyped bool
   . . . . NE l(1) tc(1) untyped bool
   . . . . . ITAB l(1) tc(1) PTR64-*uintptr

   These two cases reveal that during typechecking we're not forcing if or for conditions to a concrete type.

3. Find the appropriate places to add x = defaultlit(x, nil). This will probably be right after the corresponding x = typecheck(x, Erv). For example, in typecheck1's case OIF, I'd try putting it right after the n.Left = typecheck(n.Left, erv).

[fraenkel]

Fixed the OIF and OFOR. Do you want separate CLs?
The next one(s) identified are:
. . . . . ANDAND l(6) tc(1) hascall untyped bool
. . . . . . ANDAND l(6) tc(1) hascall untyped bool
. . . . . . . NE l(6) tc(1) hascall untyped bool
. . . . . . . EQ l(6) tc(1) hascall untyped bool
. . . . . . CONVNOP l(6) tc(1) hascall untyped bool
. . . . . . . LITERAL-true a(true) l(6) tc(1) untyped bool

[josharian]

Separate CLs is great. Thanks for working on this!

[gopherbot]

Change https://golang.org/cl/94475 mentions this issue: cmd/compile: Convert untyped bool for OIF and OFOR

[gopherbot]

Change https://golang.org/cl/94495 mentions this issue: cmd/compile: Convert untyped bool for comparison operators

[gopherbot]

Change https://golang.org/cl/97016 mentions this issue: cmd/compile: convert untyped bool for OEQ AND OCALLFUNC in certain case

[gopherbot]

Change https://golang.org/cl/97035 mentions this issue: cmd/compile: avoid untyped bool when comparison

[gopherbot]

Change https://golang.org/cl/97001 mentions this issue: cmd/compile: convert untyped bool during walkCases

[fraenkel]

With these last 3 CLs, there are no more reported issues.

I had changed the original CL to issue Warnl instead of Fatalf so I could see all the issues.

[gopherbot]

Change https://golang.org/cl/97856 mentions this issue: cmd/compile: convert untyped bool during various walks

[fraenkel]

A bit stumped on how to fix the "last" issue I can see.
They are all showing up as:
autogenerated:1: allocating value with type untyped bool

But I cannot seem to easily identify what the generated code is.

[fraenkel]

I know where the last fix needs to go and I have an ugly hack to make it all work but just seems wrong.
After the very last finishcompare in walkcompare, we need to fix up the types. The problem is that its conditional because of the OCONVNOP that is injected. It also needs to use defaultlit2.

[mdempsky]

But I cannot seem to easily identify what the generated code is.

You can try adding ssa.Func's Name field to the logging output. It's probably code generated by alg.go (which makes me think the issue is the ORANGE->OFOR loop lowering that I mentioned in CL 97856).

I know where the last fix needs to go and I have an ugly hack to make it all work but just seems wrong.

If you're able to identify minimized test cases, I can probably help identify where the missing defaultlits are.

[fraenkel]

Its the eq func for pipe using the following:

type atomicError struct{ v interface{} }

type pipe struct {
rerr atomicError
werr atomicError
}

[mdempsky]

Ohh, I think the issue is OCONVNOP doesn't do type propagation like I thought it does.

If you change finishcompare to just:

func finishcompare(n, r *Node, init *Nodes) *Node {
    r = typecheck(r, Erv)
    r = walkexpr(r, init)
    r = conv(r, n.Type)
    return r
}

it looks to handle that test case. (As an actual fix, I'd suggest adding a convnop helper function that does the same thing as conv, but makes sure typecheck turns the newly introduced OCONV into OCONVNOP or OLITERAL.)

Does that handle all remaining cases?

[fraenkel]

Well, that just ends badly....

<autogenerated>:1: cannot convert type..eq."".scase(<node INDREGSP> = .autotmp_3, <node INDREGSP> = .autotmp_4) (type bool) to type untyped bool
<autogenerated>:1: cannot convert !type..eq."".mcentral(<node INDREGSP> = .autotmp_6, <node INDREGSP> = .autotmp_7) (type bool) to type untyped bool
<autogenerated>:1: cannot convert !runtime.memequal(<node INDREGSP> = .autotmp_6, <node INDREGSP> = .autotmp_7, <node INDREGSP> = 40) (type bool) to type untyped bool

[fraenkel]

Added a defaultlit and all looks better.

[gopherbot]

Change https://golang.org/cl/98295 mentions this issue: cmd/compile: convert untyped bool during walkrange

[namusyaka]

My last CL will get rid of untyped bools in all sample codes.

[gopherbot]

Change https://golang.org/cl/98337 mentions this issue: cmd/compile: prevent untyped types from reaching walk