Conversation
|
Why was that particular nightly version chosen and not, say, a much newer one? |
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
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.
And also add the new Assume intrinsic while we're at it.
randomPoison
force-pushed
the
legare/codex-bump-nightly-experiment
branch
from
6efc208 to
017cb29
Compare
| PatKind::Ident(BindingMode::ByValue(Mutability::Not), ref i, None) => { | ||
| i.pattern_symbol() | ||
| } | ||
| PatKind::Ident(BindingAnnotation(ByRef::No, _), ref i, None) => i.pattern_symbol(), |
There was a problem hiding this comment.
Oh weird, this one actually fixed a bug in the rewriter where we weren't handling mut patterns correctly. I added tests to exercise the relevant code paths and confirmed that the new behavior is correct.
There was a problem hiding this comment.
Could we move this fix to its own PR? This PR is already quite large, so fixing the bug separately would be helpful.
randomPoison
changed the title
| #[test] | ||
| fn test_matcher_lit() { | ||
| refactor("rewrite_expr") | ||
| .command_args(&["$x:Lit", "0"]) | ||
| .named("matcher_lit.rs") | ||
| .test(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_matcher_lit_parse() { | ||
| refactor("rewrite_expr") | ||
| .command_args(&["$x:Lit", "parse!(dbg!($x))"]) | ||
| .named("matcher_lit_parse.rs") | ||
| .test(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_matcher_pat_mut() { | ||
| refactor("rewrite_stmts") | ||
| .command_args(&["let mut __p = __e;", "let __p = __e + 1;"]) | ||
| .named("matcher_pat_mut.rs") | ||
| .test(); | ||
| } |
There was a problem hiding this comment.
What are the new tests for?
| --- | ||
| source: pdg/src/main.rs | ||
| assertion_line: 376 | ||
| assertion_line: 386 |
There was a problem hiding this comment.
Was this updated by Codex or by insta? Because I thought insta got rid of this line. Although maybe c2rust-pdg is using an old insta version? I ask because I noticed Codex added this in one of the transpiler snapshots when it shouldn't have when I was using it to update the macos and aarch64 snapshots.
| n[19]: copy n[18] => _42 @ bb21[7]: fn exercise_allocator; _42 = move _43 as *mut libc::c_void (PtrToPtr); | ||
| n[20]: copy n[1] => _4 @ bb0[1]: fn reallocarray; _4 = _1; | ||
| n[21]: copy n[20] => _1 @ bb1[3]: fn reallocarray; _0 = const pointers::REALLOC(move _4, move _5); | ||
| n[21]: copy n[20] => _1 @ bb1[3]: fn reallocarray; _0 = const _(move _4, move _5); |
There was a problem hiding this comment.
Do we know what's happening here?
| # c2rust-analyze test outputs | ||
| c2rust-analyze/c2rust_analysis_tests_minimal-* |
There was a problem hiding this comment.
Is this that random binary that's sometimes generated?
| @@ -1,3 +1,3 @@ | |||
| [toolchain] | |||
| channel = "nightly-2022-08-08" | |||
| channel = "nightly-2022-11-03" | |||
There was a problem hiding this comment.
There are a bunch of other nightly-2022-08-08s. Can you update all of them?
| ExprKind::MethodCall(_, receiver, elements, _) => { | ||
| self.handle_seq(x.id, std::iter::once(receiver).chain(elements.iter())); |
There was a problem hiding this comment.
| ExprKind::MethodCall(_, receiver, elements, _) => { | |
| self.handle_seq(x.id, std::iter::once(receiver).chain(elements.iter())); | |
| ExprKind::MethodCall(_, recv, args, _) => { | |
| self.handle_seq(x.id, std::iter::once(recv).chain(args)); |
This is what we call them everywhere else.
Also, the .iter() on args isn't necessary.
| ExprKind::MethodCall(_, receiver, elements, _) => { | ||
| // Merge the receiver and args into a single list. | ||
| let mut all = Vec::with_capacity(elements.len() + 1); | ||
| all.push(std::mem::replace(receiver, mk().err_expr())); | ||
| all.extend(std::mem::take(elements).into_iter()); | ||
|
|
||
| self.restore_seq(id, &mut all); | ||
|
|
||
| // Pull the new receiver and elements out of the merged list. | ||
| *receiver = all.remove(0); | ||
| *elements = all; | ||
| } |
There was a problem hiding this comment.
This is a really weird implementation.
| ExprKind::MethodCall(_, receiver, elements, _) => { | |
| // Merge the receiver and args into a single list. | |
| let mut all = Vec::with_capacity(elements.len() + 1); | |
| all.push(std::mem::replace(receiver, mk().err_expr())); | |
| all.extend(std::mem::take(elements).into_iter()); | |
| self.restore_seq(id, &mut all); | |
| // Pull the new receiver and elements out of the merged list. | |
| *receiver = all.remove(0); | |
| *elements = all; | |
| } | |
| ExprKind::MethodCall(_, recv, args, _) => { | |
| let mut elements = std::iter::once(mem::replace(recv, mk().err_expr())) | |
| .chain(mem::take(args)) | |
| .collect(); | |
| self.restore_seq(id, &mut elements); | |
| *recv = elements.remove(0); | |
| *args = elements; | |
| } | |
| _ => {} |
| T: GetNodeId + ListNodeIds + AsMacNodeRef + 'ast, | ||
| { | ||
| let mut history = Vec::with_capacity(nodes.len()); | ||
| let mut history = Vec::new(); |
There was a problem hiding this comment.
| let mut history = Vec::new(); | |
| let nodes = nodes.into_iter(); | |
| let mut history = Vec::with_capacity(nodes.size_hint().0); |
This would keep the previous behavior. However, we do call history.clone(), but don't call .shrink_to_fit(), so we are wasting memory there with the previous implementation, although there are much fewer reallocations.
| 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); |
There was a problem hiding this comment.
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.
| for (arg_expr, arg_key_tree) in std::iter::once(recv) | ||
| .chain(args.iter()) |
There was a problem hiding this comment.
| for (arg_expr, arg_key_tree) in std::iter::once(recv) | |
| .chain(args.iter()) | |
| for (arg_expr, arg_key_tree) in std::iter::once(recv) | |
| .chain(args) |
| // Not ptr casts, and we don't allow ptr-to-int casts. | ||
| } | ||
| CastKind::DynStar => { | ||
| unimplemented!("dyn* casts are too unstable and rare to bother supporting") |
There was a problem hiding this comment.
Weird, why did it add this?
| } | ||
| Rvalue::Cast( | ||
| CastKind::PtrToPtr | CastKind::FnPtrToPtr, | ||
| CastKind::PtrToPtr | CastKind::FnPtrToPtr | CastKind::IntToInt, |
There was a problem hiding this comment.
Is this correct? IntToInt might truncate or extend.
| } | ||
| NonDivergingIntrinsic::CopyNonOverlapping(cno) => { | ||
| self.push_operand(&cno.src, loc, WhichPlace::Operand(0)); | ||
| self.push_operand(&cno.dst, loc, WhichPlace::Operand(0)); |
4 participants
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.