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/go2go: decide what unsafe.Sizeof/Alignof mean when applied to values of type parameter type #40301

Closed
AndrewWPhillips opened this issue Jul 20, 2020 · 40 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@AndrewWPhillips
Copy link

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

$ go version
go version devel +893c5ec17b Thu Jul 16 21:30:46 2020 +0000 windows/amd64

Does this issue reproduce with the latest release?

N/A

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Andre\AppData\Local\go-build
set GOENV=C:\Users\Andre\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\Andre\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Andre\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Users\Andre\goroot
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Users\Andre\goroot\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\Andrew\AppData\Local\Temp\go-build443956446=/tmp/go-build -gno-record-g
cc-switches

What did you do?

package main

import "unsafe"

type HasZero interface {
	type int, int8 // etc
}

func f(type T HasZero)() {
	const sz = unsafe.Sizeof(T(0))
}

$ 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

@AndrewWPhillips
Copy link
Author

AndrewWPhillips commented Jul 20, 2020

BTW I think that having unsafe.Sizeof() work on parameter types is important. It worked (see code below) when I tried go2go a couple of months ago - using "contracts" for constraints.

$ go version
go version devel +af2b592260 Wed Apr 22 14:12:34 2020 -0700 Linux/amd64

contract (
	Element(T) {
		T int8, int16, int32, int64, int, uint8, uint16, uint32, uint64, uint //, uintptr
	}
)

func minInt(type T Element)() T {
	zero := T(0)
	if zero - 1 > 0 {
		return zero
	}
	// Handle signed types - may need fix if new types added to Go (like int128)
	switch unsafe.Sizeof(T(0)) {
	case 1:
		v := math.MinInt8
		return T(v)
	case 2:
		v := math.MinInt16
		return T(v)
	case 4:
		v := math.MinInt32
		return T(v)
	case 8:
		v := math.MinInt64
		return T(v)
	}
	panic("unknown type")
}

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 20, 2020
@ianlancetaylor
Copy link
Contributor

CC @griesemer

Looks like a bug in the type checker. Perhaps unsafe.Sizeof just hasn't been implemented for type lists yet.

@griesemer
Copy link
Contributor

griesemer commented Jul 23, 2020

@AndrewWPhillips It didn't "work" before - it just may not have crashed.

The question is: What does that unsafe.Sizeof(x) where x is of a type parameter type even mean? unsafe.Sizeof is a compile time constant, it must be determined when the function that contains it is compiled. I can see how we could make this work when the associated constraint contains a type list, but if there's more than one type in the type list (or they have different sizes) it's going to be very hard to maintain that "compile-time" constant aspect. We don't have any facility for something like this in the compiler at the moment, and it's not clear (to me) that we need it.

@griesemer
Copy link
Contributor

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.

@gopherbot
Copy link

Change https://golang.org/cl/244622 mentions this issue: [dev.go2go] go/types: don't crash with unsafe.Alignof/Sizeof on a type parameter value

gopherbot pushed a commit that referenced this issue Jul 24, 2020
…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 <gri@golang.org>
@griesemer
Copy link
Contributor

@ianlancetaylor pointed out that one could define unsafe.Sizeof such that it would not return a compile-time constant if the argument is of type parameter type. That would certainly be trivial from a type-checker's point of view but would break an assumption we currently have for unsafe.Sizeof (but not permitting it would also break an assumption, so perhaps that's the way to go after all).

@griesemer
Copy link
Contributor

Reopened and retitled for decision making.

@griesemer griesemer reopened this Jul 24, 2020
@griesemer griesemer changed the title cmd/go2go: panic taking size of parameter type cmd/go2go: decide what unsafe.Sizeof/Alignof mean when applied to values of type parameter type Jul 24, 2020
@griesemer griesemer added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 24, 2020
@AndrewWPhillips
Copy link
Author

AndrewWPhillips commented Jul 25, 2020

It didn't "work" before - it just may not have crashed.

@griesemer sorry to disagree but the code from my above comment (5 days ago) was tested and working.
I created a simpler example below which prints 0 1 2 8 16 24 using go2go from go version devel +af2b592260 Wed Apr 22 14:12:34 2020 -0700 linux/amd64

package main

import "unsafe"

func main() {
	f(struct{})()
	f(byte)()
	f(int16)()
	f(uint64)()
	f(string)()
	f(time.Time)()
}

func f(type T)() {
	var v T
	println(unsafe.Sizeof(v))
}

@griesemer
Copy link
Contributor

@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 unsafe.Sizeof wasn't implemented for type of type parameter type (as suspected), but there was also no panic call to remind me that it was not yet implemented, and thus it didn't crash. Instead it used the fallback value, which happens to be the word size (== 8). So, while type-checking was not crashing, it definitively was incorrect. The translator portion of the go2go tool doesn't consider the fact that unsafe.Sizeof(T(0)) is deemed a constant value by the type-checker and always generates code to call unsafe.Sizeof(T(0)) rather than always producing the constant 8. Because the tool instantiates (== generates) the function f for each possible type, approximately resulting in a f_int, f_byte, and f_uint64 version of f, it happened to work after all. One might even say it worked because we hadn't implemented it yet...

Anyway, mystery solved. Thanks for pointing this out.

@AndrewWPhillips
Copy link
Author

AndrewWPhillips commented Jul 26, 2020

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 unsafe.Sizeof on values of type parameter types was important. However, I have tried to find examples to prove my point to no avail, except for the above example of obtaining the smallest value of signed integer types. Their is probably a better way than my clumsy use of Sizeof; I would appreciate if anyone could provide help with that. [My initial thought was to shift ~0 (ie all bits on) one bit right - but for signed ints that just sign extends - if I could cast from an unsigned type to equiv. signed type I could do it that way.]

@AndrewWPhillips
Copy link
Author

AndrewWPhillips commented Jul 29, 2020

I updated the code in my comment of Jul 25 (w/o changing the meaning) to show that in go version devel +af2b592260 Wed Apr 22 14:12:34 2020 -0700 linux/amd64 unsafe.Sizeof seems to work with any "type" of type parameter type (ie no contract used).

Also I still haven't found a good way to work out the minimum value of any integer type that does not used unsafe.Sizeof. The code below does not build due to use of unsafe.Sizeof in the 2nd last line. (It builds and work perfectly in the abovementioned Apr 22 version of go2go using a "contract".)

type Element interface {
	type int, int8 // etc
}

var signedIntMin = map[uintptr]int64{
	1: math.MinInt8,
	2: math.MinInt16,
	4: math.MinInt32,
	8: math.MinInt64,
}

// minInt returns the smallest allowed integer for an element (signed/unsigned integer)
func minInt(type T Element)() T {
	zero := T(0)
	if zero - 1 > 0 {
		// unsigned integer min has all bits off (ie zero)
		return zero
	}
	return T(signedIntMin[unsafe.Sizeof(T(0))])
}

@griesemer
Copy link
Contributor

This code implements minInt in two ways, both without using unsafe.

@AndrewWPhillips
Copy link
Author

Thanks, just what I needed. BTW I believe the first solution is not portable (int can be 32 bits, can it not)?

@griesemer
Copy link
Contributor

Correct. One could convert to a T and check if it's not 0, but then one might just as well choose the 2nd solution. There's a loop, but it iterates only 4x at most. It could be unrolled easily if speed mattered. It may be faster than the type switch.

@bcmills
Copy link
Contributor

bcmills commented Jul 30, 2020

Just to add a data point: the change to report an error broke my implementation of unsafeslice.Convert (#38203) in https://github.com/bcmills/go2go/blob/6d205890ae92524c687a32d2f29f6d0fd24eb709/unsafeslice/unsafeslice.go2.

@bcmills
Copy link
Contributor

bcmills commented Jul 30, 2020

(And I don't see a way to implement unsafeslice.Convert generically without either using unsafe.Sizeof or falling back to reflect.)

@AndrewWPhillips
Copy link
Author

AndrewWPhillips commented Aug 17, 2020

.. It could be unrolled easily if speed mattered. ...

@griesemer FYI I benchmarked your minInt funcs (thanks again) and the type switch one was almost twice as fast as the loop one. So I unrolled the loop and that was almost 20X faster, as we expected. [And the speed increase was irrespective of size of type parameter (8/16/32/64) so there must be some code elimination happening but I have not checked the generated code.]

See code here in generic "range set" container I created.

@mdempsky
Copy link
Member

mdempsky commented Jan 4, 2021

unsafe.Sizeof is currently disallowed, but unsafe.Alignof can be used to compute the same value, including uses like declaring a constant or as an array length: https://go2goplay.golang.org/p/BJXdHdUkn1K

@mdempsky
Copy link
Member

mdempsky commented Jan 4, 2021

I'm inclined to say unsafe.Sizeof should be allowed, and it should be handled as a constant expression. If polymorphic code is allowed to manipulate values of parameterized type without indirection and construct new types that contain parameterized types, then I don't see any reason the implementation wouldn't be able to handle unsafe.Sizeof as though it were a constant.

One minor corner case I do see though is how to handle things like division-by-constant-zero and invalid-constant-indices. For example:

package main

func F[T any]() {
  var t T
  const size = int(unsafe.Sizeof(t))

  println("called")

  var x int
  x /= size

  var a []int
  var _ = a[size - 1]
}

func main()  {
  f := F[[0]byte]
  println("instantiated")
  f()
}

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).

@ianlancetaylor
Copy link
Contributor

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 unsafe.Sizeof (or unsafe.Alignof) on a type parameter. So I would be opposed to any approach that triggers a compilation error at instantiation time for a type argument that satisfies the type constraint.

@mdempsky
Copy link
Member

mdempsky commented Jan 4, 2021

In that case, I'd argue we should allow unsafe.Sizeof/etc, but just not treat division-by-constant-0, etc as compile-time errors when they appear within generic functions (edit: and only appear after instantiation!). I think this is consistent with the spec'd behavior of not treating duplicate types in type switch statements as compile-time errors when they only appear because of generic instantiation: https://go.googlesource.com/proposal/+/refs/heads/master/design/go2draft-type-parameters.md#generic-types-as-type-switch-cases

@bcmills
Copy link
Contributor

bcmills commented Jan 6, 2021

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 reflect.TypeOf(…).Size() or similar to implement their generic API, in which case they'll get the same division-by-zero error as a run-time panic, and it will be (at least!) equally hard to debug because the run-time call stack will be about the same as the compile-time instantiation stack.

@dsnet
Copy link
Member

dsnet commented Jan 6, 2021

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 offset / unsize.SizeOf should be able to result in a logical shift (if Sizeof is a power of 2) or even a noop (if Sizeof is exactly 1).

@ianlancetaylor
Copy link
Contributor

We can't require that unsafe.Sizeof(v), where the type of v is or depends on some type parameter, always be a compile-time constant. That would impose a specific implementation strategy, namely re-compiling the function for every type argument that is used. We definitely don't want to do that.

@griesemer
Copy link
Contributor

There's really just two choices:

  1. unsafe.Sizeof(v) is always a compile-time constant and then won't work for certain types of v that contain type parameters; or
  2. unsafe.Sizeof(v) works for any type of v and generally is not a compile-time constant. But it is a compile-time constant when the type of v doesn't contain type parameters (or only type parameters for which the compiler can statically determine the size).

Choice 2) is general, and doesn't violate the current behavior of unsafe.Sizeof(v) (where types are non-generic). In other cases, it may return a dynamically computed value.

We already have precedent for this kind of behavior in the language: len(x) does return a compile-time constant under certain conditions but generally returns a dynamically computed value.

I am leaning towards 2) at this point.

@mdempsky
Copy link
Member

mdempsky commented Jan 9, 2021

Note that I'm not arguing that unsafe.Sizeof(v) should be a "compile-time constant." I'm arguing that it should be a "Go-language constant." That is, that you can use it in const declaration values, array-length expressions, array/slice-literal keys, etc.

I see this as a separate issue from whether the expression can be evaluated at compile-time. If an array type [N]T is allowed to vary because the type expression T depends on type arguments, I don't see why we can't allow the constant expression N to depend on them too.

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.

@ianlancetaylor
Copy link
Contributor

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, len already acts differently for different types. If we're going to permit unsafe.Sizeof for parameterized types, we can do the same.

@mdempsky
Copy link
Member

mdempsky commented Jan 9, 2021

I really don't see what makes [unsafe.Sizeof(t)]byte any harder to support than [2]T. Can you please elaborate on what you perceive to be more difficult about the former than the latter?

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.

@ianlancetaylor
Copy link
Contributor

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

i := (unsafe.Sizeof(T{}) * 1e100) / 1e99

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 unsafe.Sizeof(T{}) a real Go constant, you are suggesting that we change the way that constant expressions work in generic functions. So we're already changing the language within a generic function. I'm suggesting what seems to me to be a simpler change: unsafe.Sizeof doesn't return a real Go constant when called on a parameterized type. I don't see any particular complexity in adopting that approach.

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 unsafe.Sizeof return a Go constant in a parameterized function?

@bcmills
Copy link
Contributor

bcmills commented Jan 11, 2021

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.

I'm somewhat surprised that array lengths can't already vary based on type arguments without unsafe.Sizeof.

I guess that's because the current draft doesn't have a type constraint for “array of any length with element type T”, and because string constants cannot be converted to array literals (#36890)?

@bcmills
Copy link
Contributor

bcmills commented Jan 11, 2021

Either way: to what extent would this decision be reversible in the future? If unsafe.Sizeof returns a non-constant in generic functions today, would it be possible to make it return an actual constant in the future?

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.

@Merovius
Copy link
Contributor

It seems relevant enough to cross-post this here. One issue with making len, unsafe.Sizeof and friends compile time constants, while leaving the intuitive rule "a generic function type-checks, if it compiles for every possible type argument" intact, is that it has the potential of making type-checking generic functions NP-complete (by reducing to CNFSAT). Similar examples can be constructed by using the computed constant to index into a small array and by using it as the size of an array type and converting that to [0]struct{}. I came up with this example in response to #44253. It's, I think, a more formal phrasing of the kinds of questions @mdempsky brought up above.

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.

@dsnet

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 offset / unsize.SizeOf should be able to result in a logical shift (if Sizeof is a power of 2) or even a noop (if Sizeof is exactly 1).

I think whether or not len and unsafe.Sizeof is a constant in the type system, an implementation can treat them as constants for optimization purposes, if it so chooses. Just like I would expect there today to be no difference whether you use var or const in

func F(x int) int {
    var y int = 2
    return x/y
}

@bcmills

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 reflect.TypeOf(…).Size()
or similar to implement their generic API, in which case they'll get the same division-by-zero error as a run-time panic, and it
will be (at least!) equally hard to debug because the run-time call stack will be about the same as the compile-time instantiation
stack.

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 reflect in this matter to start with if reflect.TypeOf(rv).Size() == 0 { panic("Foo called with zero sized type") }, before calling further functions.

I guess a programmer could intentionally put a const _ = 1/unsafe.Sizeof(x) // if this fails to compile, your x is too small into their generic function to get a similar thing at compile time. But it's a bit esoteric at least.

@gopherbot
Copy link

Change https://golang.org/cl/335413 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: implement unsafe.Alignof/Sizeof for type parameters

@griesemer
Copy link
Contributor

The above CL takes a straight-forward approach (which may not be the correct one, but it seems sensible at the moment):

  • If the argument to unsafe.Alignof/Sizeof is not of type parameter type, the functions work as before and return a constant.
  • If the argument is of type parameter type and all types in the type set have the same alignment or size, the result is that constant alignment or size.
  • Otherwise the result is a value, which possibly may be 0 for unsafe.Sizeof.

"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.

@mdempsky
Copy link
Member

How does that CL handle cases like unsafe.Sizeof([1]T{}) or unsafe.Offsetof(struct{t T; x byte}{}.x) when T is a type parameter type? I skimmed briefly, but couldn't immediately tell what the result would be. It seems like they're still allowed though.

@griesemer
Copy link
Contributor

Those cases are still incorrect, as they were before. The Sizeof etc. computations don't do "the right thing" with component types that are type parameters. They could use the same strategy as employed for a value of type parameter type, but it may also depend on the implementation strategy. Maybe we can't actually pin this down after all and we will have to say that the result is a value that may not be known at compile-time (because we may not want to in down a particular implementation), possibly even if the argument is just of type parameter type.

gopherbot pushed a commit that referenced this issue Jul 22, 2021
…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 <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@griesemer
Copy link
Contributor

This latest commit is now in sync with what has been written in the (tentative) new spec: The result for unsafe.Alignof/Offsetof/Sizeof is a constant if the argument type (or the struct type in case of Offsetof) is not of variable size. A type is of variable size if the size depends on type parameters, i.e., if the type is a type parameter or an array or struct with element or field types that are of variable size.

This permits flexibility in the implementation while maintaining the constant-ness where we would expect it.

@dobegor
Copy link

dobegor commented Nov 25, 2021

@griesemer why would unsafe.Sizeof be a non-constant if argument is / depends on a type parameter? As a user, I'd expect it to be a constant since type parameters are resolved at compile time.

For example, this (strange) use case is broken: https://gotipplay.golang.org/p/iAYLyu0IJQ7

@Merovius
Copy link
Contributor

@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.

@dobegor
Copy link

dobegor commented Nov 25, 2021

@Merovius thank you, it makes sense.
It seems that this issue might be closed now though?

@golang golang locked and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

10 participants