Fix Issue 13300 - pure function 'std.array.Appender!(T[]).Appender.en…

[edi33416]

…sureAddable' cannot call impure function 'test.T.__fieldPostBlit'

This PR makes ensureAddable infer purity. IMHO the entire templated code should infer the attributes from the user provided type.

[dlang-bot]

Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	13300	regression	pure function 'std.array.Appender!(T[]).Appender.ensureAddable' cannot call impure function 'test.T.__fieldPostBlit'

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + phobos#6811"

[edi33416]

@trikko made the changes such that ensureAddable will infer the function attributes

[trikko]

Maybe @jmdavis can give a better review than me

[wilzbach]

This just moves the existing @trusted into more local blocks.

[jmdavis]

Templates should always infer attributes when whether they're appropriate is affected by the template arguments. And it's almost always better to have @trusted be as localized as possible, because it makes it clear what is being @trusted, and it makes it harder to accidentally mark something as @trusted that wasn't properly vetted.

[edi33416]

@jmdavis @wilzbach after taking a closer look at the implementation, I think it has a major flaw.
When the code is !__ctfe, the extended buffer never gets initialized to T.init, so there will be random data sitting there. If T has an elaborate destructor, calling the destructor on the extended elements will lead to Undefined Behaviour.
Am I right, or did I misread the code?

[thewilsonator]

Removing auto merge.

[thewilsonator]

@edi33416 can you please take a look at why druntime's benchmark fails to link (see the build kite failure)?

[wilzbach]

The buildkite failure is unrelated as currently druntime's benchmark code gets linked with the currently built Phobos, but somehow still checks out the master version of Phobos.

[thewilsonator]

OK

[edi33416]

@wilzbach @thewilsonator any thoughts on this?

after taking a closer look at the implementation, I think it has a major flaw.
When the code is !__ctfe, the extended buffer never gets initialized to T.init, so there will be random data sitting there. If T has an elaborate destructor, calling the destructor on the extended elements will lead to Undefined Behaviour.
Am I right, or did I misread the code?

[jmdavis]

@edi33416 In the case where GC.extend is used, it's going to depend on how that's implemented. If it knows that it's operating on a block of GC-allocated memory that's typed to be whatever the Appender is for, then I think that it's doing the same thing that happens when just dealing with a dynamic array which is a slice of GC-allocated memory. So, as far as I can tell without digging into the GC's implementation, that part is fine.

However, I suspect that the part of the code after that which reallocates the array is wrong, though not in the way you're afraid of. If you take a look at what uninitializedArray's internal implementation does at https://github.com/dlang/phobos/blob/master/std/array.d#L940, it specifically avoids using GC.malloc and instead calls _d_newarrayU directly so that the block of memory will be properly marked by the GC as holding a particular type so that stuff like the destructors will be run correctly. Appender on the other hand currently uses GC.qAlloc, and glancing over it, it looks like it's basically the same as GC.malloc except that it has a different return type. It can be given a TypeInfo (which might actually then do what uninitializedArray is trying to do with _d_newarrayU - I'm not sure), but Appender is not giving it one. So, the memory is untyped, and as such, I'm pretty sure that the destructor is then never called for any of the elements in the array rather than there being an issue with it being called on memory that wasn't properly initialized.

Given that it's not actually guaranteed that any GC-allocated memory ever has its destructor called, I don't think that this is technically wrong, but it's almost certainly undesirable. Even if proper destruction isn't actually guaranteed, we really want it to be happening if at all possible.

Now, I could be misunderstanding something here, since I'm not all that well-versed in the finer details of the GC implementation, but from what I can tell, that's the situation. To give a better response, I'd have to do a lot more digging in the GC. If I were going to ask someone about this, I'd probably ask @schveiguy, since as I understand it, he's pretty familiar with how Appender works and how stuff like BlkInfo in the GC works.

[schveiguy]

However, I suspect that the part of the code after that which reallocates the array is wrong ...

uninitializedArray was fixed quite recently: #6584

However, there is an interesting thing here. The array capacity (the one the GC knows about and uses) is not set in any of the operations (in only one place it's set, and that's when passing an array that's appendable to Appender constructor). This makes complete sense, since storing the capacity requires knowing all about the druntime lifetime workings, and this is trying to avoid that mechanism since it's definitely slower.

But the drawback there is that the GC relies on the capacity to know how many elements of the array are valid for destruction.

So there needs to be some work done to make the GC properly call destructors. There are a few ways to do it, but I don't know if that's the way we want to go. The biggest issue I see is that we have to mark the data as appendable and properly store the capacity when the data is returned. But when to do that, I'm not sure. Falling back on the array runtime is going to be more expensive than Appender itself, so you don't want to call it often.

Note that historically, Appender was written before the array runtime added array element destruction.

[schveiguy]

https://issues.dlang.org/show_bug.cgi?id=19511