Issue 3223 migrate to rust edition 2024

[abmclin]

Resolves #3223

I've performed the steps outlined in https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-project-to-a-new-edition.html and committed changes before running cargo fmt and committing again.

As expected there were lot of code changes due to maintaining drop order, the changes made code clarity worse, so I reverted those changes.

There were changes made to macro rules, changing exprs to expr_2021 due to this reason https://doc.rust-lang.org/edition-guide/rust-2024/macro-fragment-specifiers.html#:~:text=To%20support%20the%20old%20behavior,existing%20macros%20does%20not%20change.

I scanned the codebase and it doesn't look like const and _ expressions are used in scryer so the macro rule change seemed unnecessary and I reverted expr_2021 back to expr. But I'm not familiar enough with the codebase to say if it's ok to do that or not. Please chime in if it's a potential issue.

Changes were made to resolve explicit borrowing in implicit borrow match patterns errors and new warnings generated about missing unsafe blocks in unsafe functions.

I ran cargo clippy and it generated two warnings about the dead code enum which was already known, and another warning:

warning: large size difference between variants
  --> src\variable_records.rs:60:1
   |
60 | /  pub enum PermVarAllocation {
61 | |/     Done {
62 | ||         shallow_safety: VarSafetyStatus,
63 | ||         deep_safety: VarSafetyStatus,
64 | ||     },
   | ||_____- the largest variant contains at least 208 bytes
65 | |      Pending,
   | |      ------- the second-largest variant carries no data at all
66 | |  }
   | |__^ the entire enum is at least 208 bytes
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.94.0/index.html#large_enum_variant
   = note: `#[warn(clippy::large_enum_variant)]` on by default
help: consider boxing the large fields or introducing indirection in some other way to reduce the total size of the enum
   |
63 -         deep_safety: VarSafetyStatus,
63 +         deep_safety: Box<VarSafetyStatus>,
   |

Does this clippy lint need addressing or can just ignore it?

[abmclin]

I also ran tests on Linux and Windows, there don't seem to be any breaking tests due to the new changes.

[triska]

Amazing work, thank you a lot!

For PRs that are not yet meant to be applied, we can convert them to a draft until they are ready to be applied.

[Skgland]

The review request¹ combined with the WIP title give me a bit of a mixed signal.
Usually I would expect review to happen once a PR is ready. Otherwise when requesting a review for incomplete work I would expect the request to include either a note on what is still WIP so the review can skip the incomplete part or for it to request review of a specific (ready) part.

---

My fault not yours, I should have noted this earlier, so just ignore this section.
In hindsight I think I would have liked a commit of the unmodified result of cargo fix --edition to see the changes that were undone
and given how this touches a lot of files I would have preferred it to be done by a regular contributor.

---

Does this clippy lint need addressing or can just ignore it?

Clippy isn't checked in CI so I would recommend to just ignore that in this PR.

---

My only complaint regarding the PR content is the size/placement of the added unsafe blocks (see inline comments).

Footnotes

1. https://github.com/mthom/scryer-prolog/issues/3223#issuecomment-4180291987 ↩

[Skgland]

For PRs that are not yet meant to be applied, we can convert them to a draft until they are ready to be applied.

@abmclin as the PR author you should have a Convert to Draft link/button in the underlined place:

e.g.

[abmclin]

Hi @Skgland sorry about giving a mixed signal. I'm used to using WIP tag to also serve as an indication that a review is needed not necessarily to indicate that it is incomplete. I'm happy to convert to a draft instead.

I do realize that lot of files are touched in this PR. Although you said to ignore the comment about adding a commit prior to my edits but if it would make it easier — I don't mind — adding a prior commit to show the changes applied by cargo fix --edition before my own edits. I don't mind doing that if it helps to understand what happened.

[Skgland]

Hi @Skgland sorry about giving a mixed signal. I'm used to using WIP tag to also serve as an indication that a review is needed not necessarily to indicate that it is incomplete. I'm happy to convert to a draft instead.

That is an odd use of WIP in my opinion. I don't think the review status should be signaled using a PR Title tag or using draft status. As I tried to convey in my earlier comment, for me a WIP tag or draft status indicates that the PR Author is not yet done making changes and as such review should usually wait as it could become outdated/obsolete by said changes (the PR is still a moving target). No need to be sorry, the mixed signalling appears to be a result of different expectation on how the WIP tag is used.
That a PR still needs to be reviewed would instead be indicated by the absence of an approving review or the presents of a review requesting changes.

Regarding WIP vs Draft status: I would expect both to be used in the same circumstances (PR author is not yet done making changes).
My preference is to use the Draft status where available and only use a WIP tag where available.
Using the draft status makes filtering easier with is:draft -is:draft and the GitHub UI won't allow merging a draft PR preventing it from accidentally being merged prematurely.

I feel like I am rambling too much on this and going off-topic, sorry.

I do realize that lot of files are touched in this PR.

It's fine, the changes are basically all minor mechanical changes and it isn't a complete project refactoring.

Although you said to ignore the comment about adding a commit prior to my edits but if it would make it easier — I don't mind — adding a prior commit to show the changes applied by cargo fix --edition before my own edits. I don't mind doing that if it helps to understand what happened.

In the meantime I have checked out the latest master commit and ran cargo fix --edition myself to check, so no need.
Though I had some hickups and had to use --broken-code¹ to get that to work properly and then had to fix a couple (~four?) things manually. I would assume you had to do so as well?

If you could fix the unsafe blocks as I pointed out in my review, after that I think this is good to go.

Footnotes

1. https://doc.rust-lang.org/edition-guide/editions/advanced-migrations.html#partial-migration-with-broken-code ↩

[abmclin]

Yes I had to use --broken-code option for the initial pass of cargo fix --edition and manually fix those broken bits of code.

I am now going through each of your code comment.

[abmclin]

I have made the requested changes. Let me know if I should squash the commits prior to merging.

[Skgland]

Took me some time to review as I was busy visiting family over the easter weekend.

A few more notes.

[abmclin]

@Skgland thanks for the additional comments.

Based on your examples, I also found a few additional unsafe scope refinement opportunities that I went ahead and fixed.

[Skgland]

looks good now 👍

[abmclin]

Thanks @Skgland I'm thinking to wait until the rustyline 18.0.0 version bump PR is resolved then rebasing this PR branch onto master and making it ready for review.