Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
cmd/compile: cannot handle alias type declaration #25838
The original fix for #18640 was only partially correct. However, removing that incorrect code leads to another cycle related issue for the test case:
type ( e = f f = g g = h h i i = j j = e )
Commented out this test case in test/fixedbugs/issue18640.go for now.
changed the title from
cmd/compile: incorrect cycle error (follow-up on #18640)
cmd/compile: incorrect cycle error reported by dowith()
Jun 12, 2018
This was referenced
Oct 31, 2018
(Slightly) simpler reproducer:
package pkg type P = *T type T P
The code is accepted if the order of the type declarations is swapped. The bug is likely in type-checking alias declarations that refer to defined types that have not yet been set up and which refer back to the alias type. We end up with an incorrect
referenced this issue
Nov 1, 2018
This bug (and the related bugs) are due to an incorrect implementation of type aliases in the presence of cycles. The bug(s) can be made to disappear by re-ordering the code (though it may not be obvious, and it may not always be possible). Furthermore, the actual underlying problem was hidden by a typo in the cycle-detection code which left the code "surprisingly correct". Here's this first problem:
In function typecheck (typecheck.go), when we detect a typechecking look (n.Typecheck() == 2), in the case of n.Op == OTYPE where we are expecting a type, we check if the loop contains any (non-alias) type definition. If so, the cycle is permitted (for now, it may still be invalid when we compute the type size). However, if we find a non-alias type definition, instead of returning the incoming node, we return the node we found in the loop (because the loop iteration variable is also named n)! Thus we continue with the wrong type which happens to be a defined type node that has a TFORW type attached to it and then much of the rest "works" w/o blowing up, albeit incorrectly. In some cases a cyclic type of defined types is created which then results in a cycle error during type size computation in the dowith function. Once we fix this, in code with alias cycles, the type-checker will simply crash.
To correctly fix this in all situations we (very likely) have to bite the bullet and introduce a TALIAS type which is simply a forwarder to the actual type, very similar to the existing TFORW, except that we won't be able to simply update the TALIAS type to the underlying type since in general we cannot have two versions of the (underlying) type - they must be the same type (pointer). To make this work, we must ensure that all accesses to a Type always forward to the aliased type if the type is a TALIAS.
So the fix is about as follows:
Much of 4) can be achieved my accessing Type information only via accessors and the accessors take care of the forwarding. Alternatively (and perhaps more efficiently), the accessors verify that we never access a TALIAS (which they already do by virtue of checking the Etype is correct), and making sure we resolve TALIAS types before calling accessors.
Introducing TALIAS sounds like a reasonable solution, and doing so probably even makes sense to address #21866. I expect this to be non-trivial though.
For brainstorming though, I think another option for addressing cycles would be to break package-scope declaration typechecking into a few more phases:
By only recursively typechecking aliases during step 2, we guarantee if we get into a loop, then it's because of an invalid alias loop.
And by deferring calculating array lengths, we have no need for calculating widths, so the TFORW placeholders are fine to use without eagerly recursively typechecking them. (I think this would amount to my suggestion on #13890.)
pushed a commit
Nov 5, 2018
I have removed the ReleaseBlocker label here. The primarily visible problem (incorrect cycle reported via dowith) has been fixed with https://golang.org/cl/147286 and people have been living ok with the remaining alias cycle issue. We clearly still need to fix this in full but if we don't get it done for 1.12 it will be ok. Fixing it may require more changes than we are willing to accept at this point of the freeze, and there may be higher priority work having a bigger impact.