Make std::fs::{File, ReadDir, DirEntry} always needs_drop even when unsupported.

[kpreid]

This eliminates the portability hazard (and inconsistent linting of clippy::drop_non_drop) of types having destructors on some platforms and not others.

It is arguably a breaking change, but a breaking change that would also be required if any of these platforms later gains filesystem support.

An alternative to this change would be to implement Drop on the std::fs types instead of the std::sys types. That would be more reliable, but would be more of a public API change since T: Drop bounds are unfortunately possible.

[rustbot]

r? @clarfonthey

rustbot has assigned @clarfonthey.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 13 candidates
• Random selection from ChrisDenton, Darksonn, JohnTitor, Mark-Simulacrum, clarfonthey

[joboet]

It is arguably a breaking change, but a breaking change that would also be required if any of these platforms later gains filesystem support.

I'd argue that this isn't breaking, needs_drop doesn't promise anything except for a lack of false-negatives, so a change to a positive is fine – especially since this improves consistency.

@bors r+ rollup
r? joboet

[rust-bors]

📌 Commit ae174cf has been approved by joboet

It is now in the queue for this repository.

[kpreid]

It is arguably a breaking change, but a breaking change that would also be required if any of these platforms later gains filesystem support.

I'd argue that this isn't breaking, needs_drop doesn't promise anything except for a lack of false-negatives, so a change to a positive is fine – especially since this improves consistency.

I was imprecise and meant all the effects of "has a destructor" rather than literally needs_drop alone. For example, this PR will make the following program stop compiling on wasm32-unknown-unknown:

pub const fn foo(file: std::fs::File) {}

[joboet]

Oh, right, non-const Drop is a thing...

@bors r-

I thought this was trivial, but it isn't.

[rust-bors]

This pull request was unapproved.

View changes since this unapproval

[theemathas]

Crater can't tell us the impact of this change, since crater only builds for linux...

[clarfonthey]

Yeah, feel like this would be best with some more discussion, although we're potentially not meeting for another 1-2 weeks, so, maybe it might be worth opening a Zulip thread.

[Mark-Simulacrum]

We've done this in the past for other, similarly shaped, changes -- e.g., #155113, #155153. I think we should do this here too.

@rfcbot fcp merge libs

[rust-rfcbot]

@Mark-Simulacrum has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @cuviper
• @dtolnay
• @joshtriplett
• @the8472
• @thomcc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/libs-ping: FCP proposed for libs, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.