Skip to content

Roll toolchain to nightly-2022-11-03#1614

Merged
randomPoison merged 43 commits intomasterfrom
legare/codex-bump-nightly-experiment
Apr 23, 2026
Merged

Roll toolchain to nightly-2022-11-03#1614
randomPoison merged 43 commits intomasterfrom
legare/codex-bump-nightly-experiment

Conversation

@randomPoison
Copy link
Copy Markdown
Contributor

@randomPoison randomPoison commented Feb 25, 2026

Copilot choked and died on this, but Codex seems to have succeeded. I let it spin for like an hour and it got code compiling and tests running, so let's let CI run and see if anything is broken that I didn't catch locally.

Roll the nightly toolchain 6 weeks forward to nightly-2022-11-03. I've reviewed and cleaned up the changes and they all seem reasonable to me now, so this is ready for review.

Comment thread c2rust-analyze/tests/filecheck/aggregate1.rs Outdated
Comment thread c2rust-refactor/gen/ast.txt
Comment thread c2rust-refactor/src/ast_builder/builder.rs
Comment thread c2rust-refactor/src/ast_manip/util.rs
Comment thread c2rust-refactor/src/collapse/deleted.rs Outdated
Comment thread c2rust-refactor/src/driver.rs
Comment thread c2rust-refactor/src/driver.rs Outdated
@Rua
Copy link
Copy Markdown
Contributor

Rua commented Mar 3, 2026

Why was that particular nightly version chosen and not, say, a much newer one?

Copy link
Copy Markdown
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Why was that particular nightly version chosen and not, say, a much newer one?

We were mostly just testing out codex on this. We decided to first start with a smaller jump so that it could handle it better. We may try a longer jump next, but there's a larger risk things go wrong, and in case we introduce some regression, it'll be harder to debug/rollback, and a much larger set of changes will be much more difficult to review as well.

@randomPoison randomPoison force-pushed the legare/codex-bump-nightly-experiment branch from 6efc208 to 017cb29 Compare March 10, 2026 22:16
Comment thread c2rust-refactor/src/ast_manip/util.rs
Comment thread c2rust-refactor/src/collapse/deleted.rs Outdated
Comment on lines -158 to 165
ExprKind::MethodCall(_seg, args, _span) => {
ExprKind::MethodCall(_seg, recv, args, _span) => {
if let Some(fn_sig) = opt_fn_sig {
if let Some(&ty) = fn_sig.inputs().get(0) {
illtyped |= self.ensure(recv, ty);
}
for (i, arg) in args.iter_mut().enumerate() {
if let Some(&ty) = fn_sig.inputs().get(i) {
if let Some(&ty) = fn_sig.inputs().get(i + 1) {
illtyped |= self.ensure(arg, ty);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have a bunch of different ways of handling the recv/args split. Could we try to collapse that to just one? That is, have a fn combine_recv_and_args(recv: P<Expr>, args: Vec<P<Expr>>) -> impl Iterator<Item = P<Expr>> that we use everywhere. We can iterate over it, use .zip (e.g., here with fn_sig.inputs()), .nth (would work here, too), .enumerate, which I think should cover almost all of the use cases and keeps the usage much more standardized.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, I don't think we need a helper function here, I think the common pattern of iter::once(recv).chain(args) is good enough. I did a pass over the places where we're handling MethodCall and unified a couple to use that approach.

Comment thread c2rust-refactor/src/transform/literals.rs Outdated
Comment thread c2rust-analyze/src/dataflow/type_check.rs
}
Rvalue::Cast(
CastKind::PtrToPtr | CastKind::FnPtrToPtr,
CastKind::PtrToPtr | CastKind::FnPtrToPtr | CastKind::IntToInt,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this correct? IntToInt might truncate or extend.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The comment below this specifically mentions covering casts like usize to size_t, so this seems reasonable, but without spending more time digging I can't be sure.

Comment thread c2rust-analyze/src/last_use.rs Outdated
Comment thread c2rust-refactor/gen/ast.txt
Comment thread c2rust-refactor/src/ast_manip/util.rs
Comment thread c2rust-refactor/src/driver.rs
@@ -260,7 +260,7 @@ fn run_polonius<'tcx>(
//pretty::write_mir_fn(tcx, mir, &mut |_, _| Ok(()), &mut std::io::stdout()).unwrap();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would save us a lot of review time (and the agent itself a lot of cycles) if it updated every c2rust component separately next time. Then we could prioritize c2rust-refactor higher, and leave the others for later (I don't know how much we care about the ALLSTAR stuff right now).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. I think before attempting larger toolchain rolls we need to trim down the code and disable stuff we don't actively want to support. There are a lot of changes in code that we're effectively treating as dead, and that adds a lot of noise to the PR. We also need to revive more of the c2rust-refactor test suite, since there are a bunch of transforms that are currently dead, making changes to that code hard to verify and thus harder to review.

Comment thread c2rust-bitfields-derive/Cargo.toml Outdated
Comment thread c2rust-refactor/src/analysis/type_eq.rs Outdated
Comment thread c2rust-refactor/src/collapse/deleted.rs
Comment thread c2rust-refactor/src/context.rs Outdated
// We do not have a SpanNodeKind for certain nodes
Some(Node::TypeBinding(_)) => None,
Some(Node::TraitRef(_)) => None,
Some(Node::ExprField(_)) => None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These aren't technically FieldDefs. ExprField and PatField contain an Expr and Pat, respectively, so I'm wondering if we do need a new NodeSpanKind here or two. Maybe one shared between the two of them:

        Some(Node::ExprField(expr)) => Some(NodeSpan::new(expr.span, Field)),
        Some(Node::PatField(pat)) => Some(NodeSpan::new(pat.span, Field)),

since ExprField and PatField can't collide with each other, but they may collide with their inner values.

Otoh since ExprField is going to be something like ident: value, maybe there isn't the chance of a collision. Do we have a test for these?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, I don't think we need new node types, we already have ExprField and NodeField that we were using in a couple of places. I've updated this function to return span info for those.

What exactly do you mean by "collision" here? Are you referring to overlapping spans? My understanding is that ExprField and PatField can't overlap with each other because they show up in different positions (expressions vs patterns), but maybe you're referring to something else.

As far as I know none of our existing transforms hit this code path, so we don't have any tests to cover it. I can try adding one, but I'm hesitant to put together a contrived test for this functionality. I'd rather have tests for real transforms that naturally hit this code path, and I don't see a good option for that currently.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you referring to overlapping spans?

That's right, we were getting multiple nodes with identical spans because sometimes an AST node will include an inner node with the same span. For example, a PathExpr and the inner Path node overlap.

My understanding is that ExprField and PatField can't overlap with each other

That's right, but I was worried that ExprField and the inner Expr might collide. On second thought, that's probably not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would field shorthand e.g. in let field = ...; FooStruct { field } cause the spans to collide? The shorthand is usable in both expressions and patterns.

Copy link
Copy Markdown
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

There's one more nightly-2022-08-08 left in c2rust-rust-tools/src/lib.rs. Otherwise, I think it LGTM now.

@randomPoison
Copy link
Copy Markdown
Contributor Author

@kkysen Ah right good catch. I updated the comment to not specifically reference the exact nightly, I'd prefer that we don't have to keep updating references to the exact nightly every time we do a toolchain roll.

@kkysen
Copy link
Copy Markdown
Contributor

kkysen commented Apr 22, 2026

I'd prefer that we don't have to keep updating references to the exact nightly every time we do a toolchain roll.

I usually do because it makes it very easy to find. For example, once that nightly toolchain reaches edition 2024, that comment should be removed/changed, but it's now harder to find it.

@randomPoison randomPoison force-pushed the legare/codex-bump-nightly-experiment branch from 166550a to 06dcec8 Compare April 23, 2026 00:00
@randomPoison
Copy link
Copy Markdown
Contributor Author

Ah sure, I see how that'd be useful. Changed it to keep the comment and just update the nightly name.

@randomPoison randomPoison merged commit 0389785 into master Apr 23, 2026
18 of 20 checks passed
@randomPoison randomPoison deleted the legare/codex-bump-nightly-experiment branch April 23, 2026 19:42
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.

5 participants