disallow sink openArray[T] types

[zerbina]

Summary

No specification for how sink openArray[T] should work exists, and the current implementation only works without issues in the simple case where the argument is a literal array-constructor expression that is not constant.

For most other cases, either the seq or string used as the argument are not cleaned up or, for constant expression, a segmentation fault occurs at run-time when trying to mutate the openArray.

Passing an immutable openArray to a sink openArray, while in theory supported, also results in a run-time error.

Details

• remove the "cannot create implicit openArray` report; it's unused now
• disable the test that made use of sink openArray

[saem]

Bors r+

(•_•)

sink openArray

( •_•)>⌐■-■

You've been sunk for the last time

(⌐■_■)

[bors]

Build failed:

• All check passed

[zerbina]

I had not considered generics (i.e. sink T) here: if T resolves to openArray at instantiation time, the compiler now also reports an error (it did so previously only with var and lent). The add procedure for seq uses sink, thus essentially making the usage of openArray[T] as the element type impossible.

A possible solution

If the argument to the sink type operator is an unresolved generic, don't compute the type yet (i.e. by creating a tySink type) but leave it as an uncomputed type expression (i.e. tyFromExpr). Once the generic argument is resolved, the sink operation is applied, but with the special rule of it being a no-op if the resolved argument is an view type.

In other words: if the T in sink T resolves to a view type, don't treat it as an error and use the view type directly.

Thoughts?

[saem]

Given current definitions:

• sink: "I'm giving this away" or it's "consumed" if we're to use more Pony terminology
• openArray: a view, which means very much not owned

I think sink openArray[T] should very much be an error.

---

I think the issue is that openArray is not a view, it's more like an interface-hardcoded-concept-meets-view-thingy. I think first step is figuring out what openArray actually means, then we can figure out how it should interact with sink.

If we need a decision in the interim because this blocks other things, let's keep the behaviour as it is in your PR, I believe it's actually correct, and adapt any stdlib bits if required.

[zerbina]

I think sink openArray[T] should very much be an error.

I think so too. But I'm not so sure whether it's a good idea to also report an error for sink T where T resolves to openArray.

Adapting the parts in the that are causing the issues is possible:

proc add*[T: not openArray](x: var seq[T], item: sink T) {.magic: "AppendSeqElem".}

proc add*[T: openArray](x: var seq[T], item: T) {.magic: "AppendSeqElem".}

but when using first-class openArrays, other generic code might run into the same problem, hence my suggestion with adjusting the sink type operator.

The PR is not blocking anything else right now, so I'm turning it into draft until we got the details figured out.

[krux02]

I think the issue is that openArray is not a view, it's more like an interface-hardcoded-concept-meets-view-thingy. I think first step is figuring out what openArray actually means, then we can figure out how it should interact with sink.

In my code I've seen it always as a language supported pointer+length pair. Something that I've done in C with pointer + length as two distinct parameters I can now do In one. Nice. I've also writter C code wrappers that expose a pointer+length pair as an openArray to Nim for seamless interop.

In C a pointer+length parameter can very much move the ownership of the data. But that doesn't apply to Nim. Here an openArray isn't a value type, it is merely the pointer+length tuple that is extracted from either a seq or an array. It doesn't reflect ownership on its own.