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

The generic version of NamedTempFile can break compilation #224

Closed
Erk- opened this issue Mar 24, 2023 · 4 comments · Fixed by #225
Closed

The generic version of NamedTempFile can break compilation #224

Erk- opened this issue Mar 24, 2023 · 4 comments · Fixed by #225

Comments

@Erk-
Copy link

Erk- commented Mar 24, 2023

The more generic version of NamedTempFile introduced in #177 by @jasonwhite can break compilation in some cases without even being involved itself. It seems to happen in some cases with generic where it will turn into a infinitely recursive type.

Example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=671872049ef51f20f8bcc976177c6b62

if you comment the line use tempfile; the code will compile as expected.

edit: Made the MRE smaller.

@jasonwhite
Copy link
Contributor

Sorry about that! I had been using the generic version of NamedTempFile for months without any issues in a huge code base.

I suspect it is triggered by this impl:

tempfile/src/file/mod.rs

Lines 940 to 951 in 7c77c19

impl<'a, F> Write for &'a NamedTempFile<F>
where
&'a F: Write,
{
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
self.as_file().write(buf).with_err_path(|| self.path())
}
#[inline]
fn flush(&mut self) -> io::Result<()> {
self.as_file().flush().with_err_path(|| self.path())
}
}

Then, the for<'a> &'a T: Write bound has some weird interaction with it. I'll take a closer look today.

@jasonwhite
Copy link
Contributor

jasonwhite commented Mar 24, 2023

Here's another reproducer (without tempfile): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7682acbe9f55d9c8195877f86c4a3301

Ultimately, it seems that this is an issue with the compiler not supporting cycles in trait bounds: rust-lang/rust#96634

I think the easiest way to fix this is to change this

impl<'a, F> Write for &'a NamedTempFile<F> 
where 
   &'a F: Write, 
{
   // ...
}

to this:

impl Write for &NamedTempFile<File> {
   // ...
}

This makes it more specialized, but keeps the same requirements the same as before NamedTempFile was given a generic parameter. So, I doubt this specialization will break any code.

@jasonwhite
Copy link
Contributor

@Erk- Also, I am curious why T: Write + Read is not sufficient for your needs. It works perfectly fine in the example you linked.

@Erk-
Copy link
Author

Erk- commented Mar 29, 2023

@Erk- Also, I am curious why T: Write + Read is not sufficient for your needs. It works perfectly fine in the example you linked.

It was from a larger piece of code where T is wrapped in a Arc so if we want to be able to read and write through it we need that bound.

And thanks for the quick fix

github-merge-queue bot pushed a commit to knope-dev/knope that referenced this issue Aug 10, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tempfile](https://stebalien.com/projects/tempfile-rs/)
([source](https://togithub.com/Stebalien/tempfile)) | dev-dependencies |
minor | `3.4.0` -> `3.7.1` |

---

### Release Notes

<details>
<summary>Stebalien/tempfile (tempfile)</summary>

###
[`v3.7.1`](https://togithub.com/Stebalien/tempfile/blob/HEAD/CHANGELOG.md#371)

[Compare
Source](https://togithub.com/Stebalien/tempfile/compare/v3.7.0...v3.7.1)

-   Tempfile builds on haiku again.
- Under the hood, we've switched from the unlinkat/linkat syscalls to
the regular unlink/link syscalls where possible.

###
[`v3.7.0`](https://togithub.com/Stebalien/tempfile/blob/HEAD/CHANGELOG.md#370)

[Compare
Source](https://togithub.com/Stebalien/tempfile/compare/v3.6.0...v3.7.0)

BREAKING: This release updates the MSRV to 1.63. This isn't an
API-breaking change (so no major
release) but it's still a breaking change for some users.

-   Update fastrand from 1.6 to 2.0
-   Update rustix to 0.38
-   Updates the MSRV to 1.63.
-   Provide AsFd/AsRawFd on wasi.

###
[`v3.6.0`](https://togithub.com/Stebalien/tempfile/blob/HEAD/CHANGELOG.md#360)

[Compare
Source](https://togithub.com/Stebalien/tempfile/compare/v3.5.0...v3.6.0)

-   Update windows-sys to 0.48.
-   Update rustix min version to 0.37.11
- Forward some `NamedTempFile` and `SpooledTempFile` methods to the
underlying `File` object for
    better performance (especially vectorized writes, etc.).
-   Implement `AsFd` and `AsHandle`.
-   Misc documentation fixes and code cleanups.

###
[`v3.5.0`](https://togithub.com/Stebalien/tempfile/blob/HEAD/CHANGELOG.md#350)

[Compare
Source](https://togithub.com/Stebalien/tempfile/compare/v3.4.0...v3.5.0)

- Update rustix from 0.36 to 0.37.1. This makes wasi work on rust stable
-   Update `windows-sys`, `redox_syscall`
- BREAKING: Remove the implementation of `Write for &NamedTempFile<F>
where &F: Write`. Unfortunately, this can cause compile issues in
unrelated code
([Stebalien/tempfile#224).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/knope-dev/knope).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMS4wIiwidXBkYXRlZEluVmVyIjoiMzYuMjcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Dylan Anthony <dbanty@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants