-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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/go2go: decide what unsafe.Sizeof/Alignof mean when applied to values of type parameter type #40301
Comments
BTW I think that having $ go version
|
CC @griesemer Looks like a bug in the type checker. Perhaps |
@AndrewWPhillips It didn't "work" before - it just may not have crashed. The question is: What does that |
I will "fix this" for now by reporting an error. We can revisit if we have a better idea of what the right approach is. |
Change https://golang.org/cl/244622 mentions this issue: |
…e parameter value Report an error instead for now until we have a better idea. (It's unclear what these operations should do as they are defined to return a compile-time constant which we can't know in general.) Fixes #40301. Change-Id: I22a991311de117bc00d52b67b4ce862ea09d855a Reviewed-on: https://go-review.googlesource.com/c/go/+/244622 Reviewed-by: Robert Griesemer <[email protected]>
@ianlancetaylor pointed out that one could define |
Reopened and retitled for decision making. |
@griesemer sorry to disagree but the code from my above comment (5 days ago) was tested and working.
|
@AndrewWPhillips Indeed! I apologize for the unqualified rushed judgement. I've checked out that older version and looked at the type-checker and the generated code. It turns out the Anyway, mystery solved. Thanks for pointing this out. |
Thanks @griesemer for tracking that down. I hope my feedback was of some use and not a distraction from the great work you are doing! BTW I had a strong feeling that |
I updated the code in my comment of Jul 25 (w/o changing the meaning) to show that in Also I still haven't found a good way to work out the minimum value of any integer type that does not used
|
This code implements minInt in two ways, both without using |
Thanks, just what I needed. BTW I believe the first solution is not portable (int can be 32 bits, can it not)? |
Correct. One could convert to a |
Just to add a data point: the change to report an error broke my implementation of |
(And I don't see a way to implement |
@griesemer FYI I benchmarked your See code here in generic "range set" container I created. |
|
I'm inclined to say One minor corner case I do see though is how to handle things like division-by-constant-zero and invalid-constant-indices. For example:
Is a Go compiler required to reject this program because of division by constant 0 and negative constant index? Or is it allowed or even required to accept it? If it's allowed to accept it, should the runtime error be at instantiation time, when the function is called, or when the offending expression/statement is evaluated? Or again, is it implementation defined? An implementation that uses monomorphization can easily reject the above at compile-time. But an implementation that uses purely runtime polymorphism would have a harder time with that. At the moment, I'm leaning towards making it implementation defined. If specifying a single behavior is desirable, I'd lean towards requiring it to behave the same as though the value were non-constant (i.e., panic at the division or index). |
A key aspect of the current generics design draft is that the only compilation error at instantiation time is "this type argument does not satisfy the constraint of the corresponding type parameter." All other errors are detected when the generic function is compiled. Changing this behavior would be unfortunate because it would force us into the C++ template error reporting model: "call to F1 fails because F1 calls F2 calls F3 calls F4 and the call to F4 causes a compilation error with this particular type argument." I think it's fairly important to avoid such cases. In particular, I think it's more important to avoid those cases than it is to permit calling |
In that case, I'd argue we should allow |
Is it more important to avoid those errors at compile-time than to avoid them at runtime? Because I would expect that in many cases the alternative will be that someone falls back to using |
My main interest in this being a compile-time constant (regardless of whether an error is reported for division-by-zero at runtime or compile-time) is the ability for the compiler to perform smarter arithmetic. For example |
We can't require that |
There's really just two choices:
Choice 2) is general, and doesn't violate the current behavior of We already have precedent for this kind of behavior in the language: I am leaning towards 2) at this point. |
Note that I'm not arguing that I see this as a separate issue from whether the expression can be evaluated at compile-time. If an array type I'm leaning towards making it an allowed implementation restriction that a compiler may reject division by zero, out-of-bounds constant array indexing, or negative array length within a generic function at compile-time if it can statically determine that there are no valid instantiations, and it must always do this for expressions that don't depend on type parameters (i.e., matching non-generic behavior). Otherwise, if there are any valid instantiations (or just the compiler's static analysis isn't sophisticated enough to rule them out), then the errors are caught at runtime when the offending code would be executed. Note: reflect.ArrayOf doesn't currently consistently panic for negative array bounds, but I think that's an issue that should be fixed; see #43603. If that's fixed, then it gives precedent for how run-time negative array lengths should be handled, similar to the precedent for how non-constant division-by-zero and index-out-of-bounds errors are handled. |
I don't want us to lock ourselves into requiring that this work: func F[T any](v T) []byte {
type array [unsafe.Sizeof(t)]byte
var a array
...
} Yes, we can probably make it work. But it will force us to add a lot of special cases to the compiler, adding significant complexity, for cases that will rarely arise in practice. And I don't see the advantage. As @griesemer points out, |
I really don't see what makes I'll remind that cmd/go2go already allows this (albeit with a clumsier spelling), despite trying to disallow it. That is, empirically based on our N=1 sample of generics implementations, it's easier to allow than to disallow. I see supporting this as wholly trivial in the monomorphization and GC-shape-stenciling approaches, as we'll always know the type sizes at build-time. I only see this as being a challenge for pure-reflect/dictionaries implementations. But in all discussions to-date, my impression is I'm the only person who thinks pure-reflect/dictionaries is a viable implementation approach. |
The current design draft is intended to permit each implementation to choose how it wants to implement a particular generic function, ranging from creating a single version of the function that works for all possible type arguments to creating a new version of the function for each set of type arguments. If array lengths can vary based on type arguments, we need to support that case, which seems like it can be a bunch of code for a case that will approximately never happen. We also need to support the full set of constant expressions and be able to handle them, so that code like
will do the right thing. I'm not arguing that this can't be done. I expect that it can be done. But it doesn't seem minor or trivial to me. And what do we gain? After all, in order to make Given that we are doing to change the language in some way no matter what we do, what significant benefit do we gain by having |
I'm somewhat surprised that array lengths can't already vary based on type arguments without I guess that's because the current draft doesn't have a type constraint for “array of any length with element type |
Either way: to what extent would this decision be reversible in the future? If I suppose that that could cause recoverable run-time panics to change to compile-time errors, but changing a run-time panic to a compile-time error is a “removal” in the classification scheme of https://golang.org/design/28221-go2-transitions, so it could potentially be allowed. |
It seems relevant enough to cross-post this here. One issue with making I think this is interesting to bring up, because it's not a limitation of one particular implementation of the generics design, but of the type system itself. So it affects all implementation strategies. The base culprit, I think, is the idea of having to type-check for "every possible type argument", given that these sets can be infinite and numerous. There are possibly other, undiscovered problems caused by it. Obviously, if the derived constants are only checked for actual instantiations that exist in the program - i.e. we report the error at instantiation time - this goes away. But that comes with all the problems @ianlancetaylor mentions above. So, personally, I would suggest starting with making them non-constants at first and perhaps change them later into constants as @bcmills is discussing.
I think whether or not func F(x int) int {
var y int = 2
return x/y
}
One difference is that the programmer can put a runtime assertion that a given size is not zero near the top of the stack. That is, I would expect a generic function using I guess a programmer could intentionally put a |
Change https://golang.org/cl/335413 mentions this issue: |
The above CL takes a straight-forward approach (which may not be the correct one, but it seems sensible at the moment):
"All types in the type set have the same alignment or size" requires that we know those types, which means they are explicitly specified via union elements in a constraint interface. |
How does that CL handle cases like |
Those cases are still incorrect, as they were before. The |
…ffsetof/Sizeof Changed the implementation such that the result is a variable rather than a constant if the argument type (or the struct in case of unsafe.Offsetof) has a size that depends on type parameters. Minor unrelated adjustments. For #40301. Change-Id: I1e988f1479b95648ad95a455c764ead829d75749 Reviewed-on: https://go-review.googlesource.com/c/go/+/335413 Trust: Robert Griesemer <[email protected]> Run-TryBot: Robert Griesemer <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Robert Findley <[email protected]>
This latest commit is now in sync with what has been written in the (tentative) new spec: The result for This permits flexibility in the implementation while maintaining the constant-ness where we would expect it. |
@griesemer why would For example, this (strange) use case is broken: https://gotipplay.golang.org/p/iAYLyu0IJQ7 |
@dobegor That's discussed in the rest of this issue: The case you bring up is essentially equivalent to this comment and what's mentioned here, so the fact that this doesn't work was taken into consideration for the decision. |
@Merovius thank you, it makes sense. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
N/A
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
$ go tool go2go build
What did you expect to see?
build succeeds (or perhaps build error message)
What did you see instead?
panic: Sizeof unimplemented for type sum [recovered]
panic: Sizeof unimplemented for type sum
goroutine 1 [running]:
go/types.(*Checker).handleBailout(0xc00009e3c0, 0xc0000cb9f8)
/path/goroot/src/go/types/check.go:252 +0xa5
panic(0xe21f40, 0xeb74e0)
/path/goroot/src/runtime/panic.go:969 +0x176
go/types.(*StdSizes).Sizeof(0xc0000a2280, 0xec3700, 0xc0000f0600, 0xc0000f06c0)
/path/goroot/src/go/types/sizes.go:152 +0x218
go/types.(*Config).sizeof(0xc0000f0500, 0xec3700, 0xc0000f0600, 0x0)
/path/goroot/src/go/types/sizes.go:258 +0xa2
go/types.(*Checker).builtin(0xc00009e3c0, 0xc0000f0680, 0xc0000f0440, 0x11, 0xc000020800)
/path/goroot/src/go/types/builtins.go:642 +0x3997
go/types.(*Checker).call(0xc00009e3c0, 0xc0000f0680, 0xc0000f0440, 0x30)
/path/goroot/src/go/types/call.go:63 +0xdb0
go/types.(*Checker).exprInternal(0xc00009e3c0, 0xc0000f0680, 0xebf680, 0xc0000f0440, 0x0, 0x0, 0x2b)
/path/goroot/src/go/types/expr.go:1603 +0x1df0
go/types.(*Checker).rawExpr(0xc00009e3c0, 0xc0000f0680, 0xebf680, 0xc0000f0440, 0x0, 0x0, 0x0)
/path/goroot/src/go/types/expr.go:1033 +0xc7
go/types.(*Checker).expr(0xc00009e3c0, 0xc0000f0680, 0xebf680, 0xc0000f0440)
/path/goroot/src/go/types/expr.go:1726 +0x5c
go/types.(*Checker).constDecl(0xc00009e3c0, 0xc0000d2480, 0x0, 0x0, 0xebf680, 0xc0000f0440)
/path/goroot/src/go/types/decl.go:419 +0x192
go/types.(*Checker).declStmt(0xc00009e3c0, 0xebfa00, 0xc0000f0480)
/path/goroot/src/go/types/decl.go:832 +0xe5
go/types.(*Checker).stmt(0xc00009e3c0, 0x0, 0xebf7c0, 0xc0000885d0)
/path/goroot/src/go/types/stmt.go:319 +0x3886
go/types.(*Checker).stmtList(0xc00009e3c0, 0x0, 0xc0000885e0, 0x1, 0x1)
/path/goroot/src/go/types/stmt.go:125 +0xd6
go/types.(*Checker).funcBody(0xc00009e3c0, 0xc0000d23c0, 0xfefe10, 0x1, 0xc0000d2420, 0xc0000b8e10, 0x0, 0x0)
/path/goroot/src/go/types/stmt.go:42 +0x268
go/types.(*Checker).funcDecl.func1()
/path/goroot/src/go/types/decl.go:792 +0x6e
go/types.(*Checker).processDelayed(0xc00009e3c0, 0x0)
/path/goroot/src/go/types/check.go:327 +0x45
go/types.(*Checker).checkFiles(0xc00009e3c0, 0xc0000cc0a0, 0x1, 0x1, 0x0, 0x0)
/path/goroot/src/go/types/check.go:295 +0x20d
go/types.(*Checker).Files(...)
/path/goroot/src/go/types/check.go:257
go/types.(*Config).Check(0xc0000f0500, 0xc0000a2528, 0x4, 0xc0000f0240, 0xc0000cc0a0, 0x1, 0x1, 0xc0000d5310, 0x1, 0x1, ...)
/path/goroot/src/go/types/api.go:387 +0x188
go/go2go.rewriteFilesInPath(0xc0000d2240, 0x0, 0x0, 0xe6fac3, 0x1, 0xc0000b8c60, 0x1, 0x3, 0x0, 0x0, ...)
/path/goroot/src/go/go2go/go2go.go:93 +0x4f0
go/go2go.rewriteToPkgs(0xc0000d2240, 0x0, 0x0, 0xe6fac3, 0x1, 0xc0000985c0, 0xc0000b8ab0, 0xc0000b8a80, 0xc0000b8a50, 0xc0000b8a20)
/path/goroot/src/go/go2go/go2go.go:46 +0x16e
go/go2go.Rewrite(...)
/path/goroot/src/go/go2go/go2go.go:30
main.translate(0xc0000d2240, 0xe6fac3, 0x1)
/path/goroot/src/cmd/go2go/translate.go:15 +0x4e
main.main()
/path/goroot/src/cmd/go2go/main.go:78 +0xa0c
The text was updated successfully, but these errors were encountered: