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

fix: resolve path ownership problems when using sqlx_macros_unstable #3236

Merged

Conversation

lily-mosquitoes
Copy link
Contributor

@lily-mosquitoes lily-mosquitoes commented May 20, 2024

When using --cfg=sqlx_macros_unstable the codepath taken uses the path variable which was taken earlier by resolve_blocking; this is solved by having resolve_blocking take a reference, as it is more convenient than cloning.

Does your PR solve an issue?

Yes (however I did not create an issue here).
I was trying to get the effect of triggering recompilation on migration changes by setting the nightly flag as described here: https://docs.rs/sqlx/latest/sqlx/macro.migrate.html#nightly-rust-cfg-flag

But when adding to .cargo/config.toml:

[build]
rustflags = ["--cfg=sqlx_macros_unstable"]

and having the sqlx::migrate! macro in the code, the following error appears while compiling sqlx-macros-core:

error[E0382]: borrow of moved value: `path`
    |
98  |     let path = path.canonicalize().map_err(|e| {
    |         ---- move occurs because `path` has type `PathBuf`, which does not implement the `Copy` trait
...
107 |     let migrations = sqlx_core::migrate::resolve_blocking(path)?
    |                                                           ---- value moved here
...
113 |         let path = path.to_str().ok_or_else(|| {
    |                                              ^^ value borrowed here after move
...
116 |                 path
    |                 ---- borrow occurs due to use in closure
    |
help: consider cloning the value if the performance cost is acceptable
    |
107 |     let migrations = sqlx_core::migrate::resolve_blocking(path.clone())?
    |                                                               ++++++++

Alternatively I could simply clone the path variable (as suggested by the compiler), however it seems like resolve_blocking does not really need to take ownership of the path variable.

I'm not too sure how this could be tested. Please let me know if the "tests/x.py" script is a good place to setup a test for this.

When using sqlx_macros_unstable the codepath taken further uses the path
variable and it is more convenient to not take ownership but instead
pass references to needed functions.
@abonander abonander merged commit 6c1e3a4 into launchbadge:main May 30, 2024
4 checks passed
jayy-lmao pushed a commit to jayy-lmao/sqlx that referenced this pull request Jun 6, 2024
…le` (launchbadge#3236)

* fix: make resolve_blocking not take ownership of path

When using sqlx_macros_unstable the codepath taken further uses the path
variable and it is more convenient to not take ownership but instead
pass references to needed functions.

* fix: change &PathBuf to &Path in resolve_blocking
jrasanen pushed a commit to jrasanen/sqlx that referenced this pull request Oct 14, 2024
…le` (launchbadge#3236)

* fix: make resolve_blocking not take ownership of path

When using sqlx_macros_unstable the codepath taken further uses the path
variable and it is more convenient to not take ownership but instead
pass references to needed functions.

* fix: change &PathBuf to &Path in resolve_blocking
jrasanen pushed a commit to jrasanen/sqlx that referenced this pull request Oct 14, 2024
…le` (launchbadge#3236)

* fix: make resolve_blocking not take ownership of path

When using sqlx_macros_unstable the codepath taken further uses the path
variable and it is more convenient to not take ownership but instead
pass references to needed functions.

* fix: change &PathBuf to &Path in resolve_blocking
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 this pull request may close these issues.

3 participants