Finalize conventions for close

[brson]

There's no precedent in std for this, and there's been suggestion that the current signature is not correct (preferring &mut self to self).

[brson]

In this case also the 'close' name may not be descriptive enough, since we're not just closing a resource, but deleting it.

[opilar]

@brson so close method should be named to remove?

[opilar]

Seems just drop does what we need. I think it would be better to merge close method into drop.

[brson]

@opilar So the issue here is that sometimes it's appropriate to close a resource explicitly, and not let it implicitly drop. Usually this is to handle an error condition, which drop doesn't allow.

Types in std don't generally have close methods, but they should, and the conventions aren't decided, so for this issue we need to decide what we want such 'close' conventions to be, and whether the existing method here conforms to them. Design question. Not sure how to resolve it, but it probably needs to be a broader discussion elsewhere. cc @dtolnay

[dtolnay]

Yep, this came up during our flate2 review where they use a confusing system of finish + flush_finish + try_finish. I filed rust-lang/api-guidelines#61 to follow up with a guideline for checked teardown that would apply to flate2, tempdir, and hopefully everything else.

[brson]

Still no clear guidelines.

[brson]

I believe the current definition of Tempdir::close of close(self) -> Result<()> is correct for this crate, per my argument here.

I don't think there is any reasonable case for retrying a failed close here.

[alexcrichton]

I posted a longer comment on the tracking issue, but I'd personally still feel that &mut self is more appropriate.

[brson]

@alexcrichton's comments in the linked thread about favoring &mut self for close I find fairly convincing in the general case. At least it seems that &mut self composes better if there are lots of fallible destructors in play, at the expense of maintaining more state.

[KodrAus]

When combining the tempdir and tempfile APIs in Stebalien/tempfile#33 we kept the close method as-is, which is consistent with the equivalent close methods on temporary files.

We still haven't reached much consensus in rust-lang/api-guidelines#61, but can continue the discussion there about the general merits of different approaches to checked teardown.

Thanks all!