How frequently do you edit AI generated code

code review
1

I use AI for writing Rust code over an year. It works like a charm, but recently I started sligtly modify the code. Do you think it's reasonable? For example:

 use std::os::windows::io::AsRawHandle;
@@ -41,7 +42,7 @@ const FILE_SHARE_DELETE: u32 = 4;
 fn is_directory(path: &str) -> bool {
     Path::new(path).is_dir()
 }
-fn open_directory(path: &str) -> std::fs::File {
+fn open_directory(path: &str) -> io::Result<std::fs::File> {
     let wide: Vec<u16> = OsStr::new(path)
         .encode_wide()
         .chain(std::iter::once(0))
@@ -60,10 +61,10 @@ fn open_directory(path: &str) -> std::fs::File {
     };
 
     if handle.is_null() {
-        panic!("Failed to open directory");
+        Err(io::Error::new(io::ErrorKind::NotFound, "Failed to open a directory"))
+    } else {
+        Ok(unsafe { std::fs::File::from_raw_handle(handle) })
     }
-
-    unsafe { std::fs::File::from_raw_handle(handle) }
 }
 pub fn get_canonical_path_without_prefix(path: impl AsRef<Path>) -> Option<String> {
     let path = path.as_ref();

AI uses panic to address exceptional situations, however, I think using Result with Err is more aprepriate way. What do you think? I use copilot, maybe other providers as Antropic better for Rust? Do you have any opinion on that?

2

to your specific question: returning Result being more appropriate than panic-king i agree w/ caveat.

in the general case, returning an app/libr specific error Type using thiserror is my standard way of dealing with errors.

when it's a server and the error occurs while configuring or starting i'd say panicking through expect, following the principle of fail-fast is to me the right strategy.

3

Thank you for the feedback. It's extremely valuable for me. First, I used io::Result to avoid compilation error in line .ok()?. I will push AI to use thiserror as you suggest in all other cases.

pub fn get_canonical_path_without_prefix(path: impl AsRef<Path>) -> Option<String> {
    let path = path.as_ref();
    let path_s = path.to_string_lossy();
    let file = if is_directory(&path_s) {
        open_directory(&path_s)
    } else {
        File::open(path)
    }.ok()?; // works for directories
    let handle = file.as_raw_handle();

Regarding except, AI rarely used it, so I completely forgot that there is such feature in Rust. I have several projects using the method, and if for some using except is completely fine, it isn't acceptable for others where an app should continue running regardless of the error. Since you decided to help, I would ask another favor. As you see, the code uses str and Path variant for the path. Maybe, it's reasonable to incorporate Path -> str conversion inside of open_directory? It will make the code clearer in my opinion. I also found your code style very interesting, so, if you have any OSS project, then share a link, so I can learn how to code right.

4

if you post some self-contained working code i'd be happy to look at it.

to your question about having path-to-str conversion inside your own logic that sounds fine. the principle here is to allow maximum compatible types to be used as argument(s) for your method(s). here's a good example to illustrate this.

i do have few crates i publish to crates.io: xapi-rs, xapi-data, ogc-cql2, and rkh-chk. thanks for your interest + cheers;

5

Thank you for sharing the examples. Indeed, your code is some piece of art and you focused on a good documentation, I certainly lacking to do.

6 
unsafe { std::fs::File::from_raw_handle(handle) }

What are you doing so much with the filesystem that you need unsafe code? I think this should be reserved for cases where you get that handle from some non-Rust library. From your code, it is not clear where this handle is coming from. If you just work in pure Rust with files and folders, I am even surprised AI could be so dumb.

- panic!("Failed to open directory");

As others already stated, this is only appropriate for some throwaway experiment. Normally error should be returned. Here also, looks strange AI does not follow some very obvious patterns. Looks like you are using some budget model.

It is seldom needed to edit AI-generated code because AI can fix the code in response to an additional request ("do not use panic!, return the error"). But if the question is how often you review it, that's another story.

7

I pretty much always edit it. How much depends on the background of the task I gave it.

For context: I code for fun, I usually get AI to help with stuff that involves a lot of typing or researching stuff which I'm not mentally focussed on right now. E.g:

  • I recently asked it to document a module for me. It did a pretty reasonable job over a few iterations - what really helped was a documentation skill which included all the guidance from the relevant sections of the rustdoc guide. I tidied up the main examples - and found some gaps in supporting APIs which I needed to fix in order to get them to be nice. Then let it finish the rest. It's good enough for now, future changes will be re-ordering the code and improving "see ... for more info" sections.
  • I'm about to get it to create something to take a String and return a valid windows file path. I could pull something together myself, but it'll take a load of time to find the exact right sequence of .map()s and functions to call on char, then I'd need to package the whole thing nicely into a couple of traits... So I'll get it to put something together and expect to rework and simplify the code once it's shown me all the functions that need be used.

As for some of the other points raised here. I have the following at the top AND bottom of my AGENTS.md

# redbook

This is a rust project for working with CDDA CD digital audio as per RedBook
(IEC 60908:1999). It consists of a library in `src` and a series of binaries
in `src/bin`

## MUST USE - MANDATORY INFORMATION & SKILLS

- This crate contains unsafe code. NEVER, EVER MODIFY OR CREATE UNSAFE CODE.
If a change to unsafe code is needed STOP, INFORM THE USER of what needs to be done,
WAIT FOR THE USER to implement that change. If new unsafe code is needed STOP,
INFORM THE USER of what needs to be done, WAIT FOR THE USER to implement that
change. If you are asked to REVIEW unsafe code NEVER CHANGE THE CODE, you
MAY NOT EDIT any files while reviewing unsafe code. YOU ARE NOT AUTHORISED TO
WRITE OR CHANGE UNSAFE CODE - ONLY THE USER MAY MAKE CHANGES TO UNSAFE CODE.

...

## Checklist

...
- [ ] NO CHANGES MADE TO UNSAFE CODE

So that is the first and last instruction it sees when loading AGENTS.md

I'm not satisfied though and intend to also implement a deterministic block using a post-... hook that will fail any edit or write which includes the unsafe keyword.

8

I can't stop telling -

when you use Rust never use unsafe, otherwise better to switch to C++.

Finally I found a person who shares my concern:

9

AI writes a pretty good code in .ts, however it needs still lot learning of Rust.

10

That is not true to me.
I hate it when it is lazy and not reach for exhaustive matching in typescript. It use a lot of string comparison in both typescript and rust. I have encountered an annoying situation where I want it to use sea-query builder but it kept using raw SQL. If good code means working code then yes, high standard? A big question mark.

11

This is too extreme. If you have a very small snippet of unsafe code in a large program, that is surely not a good reason to switch languages. It is a matter of degree.

12

I'm going to agree with @chung here. Personally, I have the wonderful situation where I only code in languages I like (idiomatically) and without any time pressure. I find AI will generally create the sort of code you find a lot "in the wild" where you look at it and think "well, I guess it works ..."

I have, however, found that it can create much better rust code than it most other languages, as rust has a couple of big advantages, which boil down to the quality of the documentation & compiler errors:

With that "little" bit of upfront, reusable, investment I can get some pretty good results and invest most of my editing into reducing verbosity (yeah - you wouldn't think it reading my prose, but I like short, concise code!), fixing the conceptual errors I had up front and coding the bit that I'm currently excited about...

13

To answer the question as phrased in the title: philosophically I think that once LLMs are used to generate code, performing quality assurance in general (code review, bug fixes, debugging activities etc) manually just doesn't scale nearly as well as the generation itself, which leads to by now well-known issues with PR queues filling up with "AI slop".

The fix is to automate the other steps as well. In other words: I'll answer the question in the title with the answer "Aspirationally never. Doing that is a smell of the (most likely hybrid) development process as it has been setup, and that smell should be fixed as much as actual code smells themselves".

I suspect that at least in the long term, the above answer will age a lot better than something like "have LLMs generate code, but manually inspect every commit".

14

I wish I had a manager like you.

15

I don't see motivation for aspiration at all. In human teams it is entirely likely that somebody writes original code and somebody else will edit it. Often the people finding and fixing bugs were not the same people that wrote the code. Or perhaps bug fixes or new features are added after the original author is no longer on the team.

Enter the LLM. I see no reason a human team member should not modify LLM authored code. No more than if they were modifying a team mates code. In effect the LLM is just another member of the team.

I have tweaked LLM generated code many times. On the basis that it is quicker just to get in there and do it rather than waste time trying to prompt the LLM to do for you.

It does happen that sometimes the LLM is surprised to find the code it is trying to work on is not what it thought it was. So it is "polite" to let the LLM know something has happened.

I don't see that such a way of working is like a "code smell" at all. No more than it was when tweaking human written code. Code smells are a totally different thing.

16

The only issue with that is that I didn't call it a code smell. Instead it's analogous to one, a smell in the development process itself.

Why is it a smell? Because one part of the system we call the development pipeline is producing output at least an order of magnitude faster than the rest of that system can consume it.

Now, I don't believe this situation will persist for long. And in the meantime, it's quite defensible to do things that way, hence the "Aspirationally" in my original answer.

But ultimately the development pipeline issue can also be viewed through the lens of queue management: if the consumer consumes at 10% the rate of the producer producing, and you don't alter that setup to raise consumption (or less ideally, reducing production), you can expect problems either in the form of dropped queue items (eg commits or entire PRs not getting proper code review) if the queue capacity is fixed, or indefinite blowup (ever-longer Merge Queues, which essentially turns them into a limbo of sorts when the production : consumption ratio is high enough).

17

Indeed, it produces a result really fast, and you get impressed quickly. A problem happens with a long term, because eventually you start to issue new requirements system can't adopt. It becomes a terrible slow in development new features. But here is no AI fault, it happens with human created systems as well. How to avoid that? There are several approaches, however it goes beyond of the discussed topic.

18

I found it funny, opening YouTube a got a video titling as:

I read every line I didn't write

But do not worry, Dillon Mulroy doesn't use Rust at Cloudflare. Rust seems a different storry and you can skip any review.

19

I guess you mean this video:How a Cloudflare Engineer Ships Production Code He Didn't Write | Dillon Mulroy

I get the feeling Rust is a great target language for LLM code generation because the Rust language and compiler is so fussy about data types, object lifetimes and so on that it leaves a lot less room for the AI to generate silly mistakes and removes a whole raft of details we might want to check and test. I think I would be a lot less trusting of the results if I were working in C.

But my feeling is also that I cannot imagine not reviewing the LLM generated code. That comes from having seen it evolve code into directions that lead to a dead end that cannot be developed further or just a total mess. On one occasion I felt some LLM generated code was a bit slow for what it did. Looking at it I realised it had missed a rather simple optimisation. After getting to implement that optimisation the thing was about 1000 times faster. No, really, it was that bad.

Famously Cloudflare caused a massive internet outage recently which was traced to the use of .unwrap() in a critical piece of code rather than handing the error properly. Perhaps a simple Rust nobble mistake. I have no idea if that code was LLM generated or not.

20

After 3 years of Rust programing I finalized the following rule:

Never use a blind unwrap()

Even using duration_since(UNIX_EPOCH) requires unwrap_or_default() for time returned by the system.

It's a kind of a frequently happened situation - it's fairly easy to write a correct but no efficient code using Rust.