Skip to content

Update the rustc pin #1391

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

Merged
merged 5 commits into from
Apr 10, 2025
Merged

Update the rustc pin #1391

merged 5 commits into from
Apr 10, 2025

Conversation

Nadrieril
Copy link
Collaborator

This updates the rustc pin to a nightly from a few days ago. I did it like this to avoid the many developments of the attribute handling rework.

Other relevant changes:

I'm leaving it as draft while I propagate the change to charon.

@Nadrieril Nadrieril marked this pull request as ready for review April 9, 2025 12:47
@Nadrieril Nadrieril requested a review from a team as a code owner April 9, 2025 12:47
@Nadrieril
Copy link
Collaborator Author

Alright, everything is ready!

Copy link
Collaborator

@W95Psp W95Psp left a comment

Choose a reason for hiding this comment

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

Thanks you, that's great! 😃
Let's merge!

@Nadrieril Nadrieril enabled auto-merge April 9, 2025 13:20
@Nadrieril Nadrieril added this pull request to the merge queue Apr 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 9, 2025
W95Psp added a commit to cryspen/libcrux that referenced this pull request Apr 9, 2025
@Nadrieril Nadrieril added this pull request to the merge queue Apr 9, 2025
@Nadrieril Nadrieril removed this pull request from the merge queue due to a manual request Apr 9, 2025
@karthikbhargavan
Copy link
Contributor

karthikbhargavan commented Apr 9, 2025

We need to also test this on libcrux, bertie, and pq11, both of which are somewhat critical right now. Let's please not merge in a rush if the proofs there break.

@W95Psp
Copy link
Collaborator

W95Psp commented Apr 9, 2025

I'm looking at bertie. @maximebuyse can you try this version of hax on PQ11? I don't really know what branch to test there.

@maximebuyse
Copy link
Contributor

I'm looking at bertie. @maximebuyse can you try this version of hax on PQ11? I don't really know what branch to test there.

I have a few problems. One is that some names have changed in the core lib. I can push fixes for that. But there is something else which is more concerning:

mod Err1 {
    pub struct Error();
}

struct Error();

impl From<Err1::Error> for Error {
    fn from(value: Err1::Error) -> Self {
        Error()
    }
}

fn f(x: Result<i32, Err1::Error>) -> Result<i32, Error> {
    Ok(x?)
}

Open this code snippet in the playground
In this situation, with current hax main, we have the error conversion in the extracted F* (it is implicit in Rust with the ?). With the changes in this PR, there is no conversion, so the extracted F* doesn't lax-check.

@Nadrieril
Copy link
Collaborator Author

Nadrieril commented Apr 9, 2025

I diffed the THIR json for this example between main and this PR, and there are virtually zero differences except two tiny changes because attributes changed their representation. I am confused, because I can indeed reproduce the difference in the F* output.

@Nadrieril
Copy link
Collaborator Author

Nadrieril commented Apr 9, 2025

Ah wait, this PR is a week out of date with latest hax, let me update. EDIT: nope, no change.

@maximebuyse
Copy link
Contributor

I diffed the THIR json for this example between main and this PR, and there are virtually zero differences except two tiny changes because attributes changed their representation. I am confused, because I can indeed reproduce the difference in the F* output.

This is fixed now, the question marks phase in the engine was relying on a hardcoded impl number which has changed with the rustc pin. With the fix we should be more robust. I also pushed the naming changes for the core lib so everything is good for pq11. I think we can merge! 🥳

@maximebuyse maximebuyse enabled auto-merge April 10, 2025 10:01
@maximebuyse maximebuyse added this pull request to the merge queue Apr 10, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 10, 2025
@W95Psp W95Psp merged commit 89a84ec into cryspen:main Apr 10, 2025
15 of 16 checks passed
@Nadrieril Nadrieril deleted the update-rustc branch April 10, 2025 11:28
github-merge-queue bot pushed a commit to cryspen/libcrux that referenced this pull request Apr 10, 2025
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.

4 participants