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

feat: use edition 2024 #1736

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: use edition 2024 #1736

wants to merge 4 commits into from

Conversation

iajoiner
Copy link
Contributor

The only warning I got was

warning: relative drop order changing in Rust 2024
   --> src/test_utils.rs:305:11
    |
304 |     let mut iter = v.into_iter();
    |         --------
    |         |
    |         `iter` calls a custom destructor
    |         `iter` will be dropped later as of Edition 2024
305 |     match (iter.next(), iter.next()) { (Some(item), None) => {
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |           |
    |           this value will be stored in a temporary; let us call it `#1`
    |           up until Edition 2021 `#1` is dropped last but will be dropped earlier in Edition 2024
...
310 | }
    | - now the temporary value is dropped here, before the local variables in the block or statement
    |
    = warning: this changes meaning in Rust 2024
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-tail-expr-scope.html>
    = note: most of the time, changing drop order is harmless; inspect the `impl Drop`s for side effects like releasing locks or sending messages
    = note: `--force-warn tail-expr-drop-order` implied by `--force-warn rust-2024-compatibility`

@iajoiner iajoiner changed the title feat: use Edition 2024 feat: use edition 2024 Feb 21, 2025
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @iajoiner! This looks reasonable to me!
cc @alamb

@iffyio
Copy link
Contributor

iffyio commented Feb 21, 2025

@iajoiner could you take a look at the ci failure?

@iajoiner
Copy link
Contributor Author

@iffyio Yes!

@@ -30,7 +30,7 @@ include = [
"Cargo.toml",
"LICENSE.TXT",
]
edition = "2021"
edition = "2024"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my only concern is if this will force users of slparser to use newer versions of the rust compiler (as in once we release sqlparser with this change, will it require the use of rust 1.85?)

I haven't studied the implications yet

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @iajoiner and @iffyio -- the code changes that I skimmed quickly look good to me. As I mentioned my only real concern is the implication for required rust versions (but maybe there aren't any)

@iffyio
Copy link
Contributor

iffyio commented Feb 21, 2025

Oh good point, yeah it does look like MSRV is 1.85 for 2024 edition, did a quick test

Caused by:
  feature `edition2024` is required

  The package requires the Cargo feature called `edition2024`, but that feature is not stabilized in this version of Cargo (1.84.0 (66221abde 2024-11-19)).
  Consider trying a newer version of Cargo (this may require the nightly release).
  See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#edition-2024 for more information about the status of this feature.

In which case I'm thinking this might not be a desirable change to make currently

@alamb
Copy link
Contributor

alamb commented Feb 23, 2025

🤔 interestingly we don't seem to have an MSRV policy in this crate (at least not that I could find in https://github.com/apache/datafusion-sqlparser-rs/blob/main/README.md)

We do have such a thing in DataFusion here: https://github.com/apache/datafusion?tab=readme-ov-file#rust-version-compatibility-policy

Maybe it is worth considering adding some documentation / policy in the sqlparser crate too 🤔

@iffyio
Copy link
Contributor

iffyio commented Feb 24, 2025

Adding an MSRV sounds good to me!

@alamb
Copy link
Contributor

alamb commented Feb 24, 2025

Adding an MSRV sounds good to me!

Filed an issue to track:

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