fix: empty code error

[j4k0xb]

Closes #111

[0xdevalias]

Surfacing some of my notes/comments from debugging for additional context/musings on the solution:

---

So the above seems to be the debugging as to the 'why' of what is going on; though I'm not familiar enough with the codebase to suggest where the best point of implementing the fix is.

If I was to suggest the most minimal/localised fix, it would be to the if (!stringified?.code) check within visitAllIdentifiers (Ref)

Though an argument could also be made that filtering out empty/similar files at the unminify level could have it's merits (Ref); though that would then make it less generic/flexible, as theoretically a plugin in the chain could make it a non-empty file before the openaiRename actually needs to process it.

So I still think the best fix is probably within visitAllIdentifiers.

In fact, in looking through related issues, this just seems to be a variation of the same root cause that I raised in this issue:

• Better error handling when humanify local ran on empty file (Error: Failed to stringify code) #54

I also think the general CLI output should be improved to explain what is happening better. eg. a message before running webcrack, outputting the actual filenames (not just the file number), etc.

These issues also seem to tangentially relate to some aspect of this latter suggestion:

• Where is the output? #92
• Improve model download speed, progress display, etc #58

Originally posted by @0xdevalias in #111 (comment)

---

But still not a fan of skipping files...

@j4k0xb I guess that really depends where 'skipping' is implemented. The hacky workaround in unminify (Ref) was never suggested as a proper fix to this, it was only a hacky workaround, as I said at the bottom of it:

While this wouldn't really be suited for a proper fix, it should hopefully be enough to do what you need to work around this bug in the meantime.

But for example, because unminify reduces over the passed in plugins, and feeds the code into them; the openaiRename plugin:

humanify/src/plugins/openai/openai-rename.ts

Lines 6 to 20 in 64a1b95

	export function openaiRename({
	apiKey,
	model
	}: {
	apiKey: string;
	model: string;
	}) {
	const client = new OpenAI({ apiKey });
	return async (code: string): Promise<string> => {
	return await visitAllIdentifiers(
	code,
	async (name, surroundingCode) => {
	verbose.log(`Renaming ${name}`);
	verbose.log("Context: ", surroundingCode);

Or even visitAllIdentifiers itself:

humanify/src/plugins/local-llm-rename/visit-all-identifiers.ts

Lines 15 to 25 in 64a1b95

	export async function visitAllIdentifiers(
	code: string,
	visitor: Visitor,
	onProgress?: (percentageDone: number) => void
	) {
	const ast = await parseAsync(code);
	const visited = new Set<string>();
	const renames = new Set<string>();
	if (!ast) {
	throw new Error("Failed to parse code");
	}

Could have an 'early guard statement' when code is empty, that provides some feedback and then returns before even needing to attempt to parse the AST.

My suggestion is about identifying the appropriate place to check that the inputs are relevant, and react appropriately if not. There's no reason to parse the AST and have all the other code in that file run just to turn it back into an empty string if we can just handle that case specifically at the beginning. While it's potentially a negligible performance gain, at the very least it makes the handling explicit, and makes the code easier to reason about/less chances for bugs to occur because of it.

Like without thinking too deeply about it, I would probably implement something like this:

   export async function visitAllIdentifiers( 
     code: string, 
     visitor: Visitor, 
     onProgress?: (percentageDone: number) => void 
   ) {
+    if (code === "") return ""
+
     const ast = await parseAsync(code); 
     const visited = new Set<string>(); 
     const renames = new Set<string>(); 
     if (!ast) { 
       throw new Error("Failed to parse code"); 
     }

And because visitAllIdentifiers doesn't seem to currently have any patterns around logging, to keep with that, I would also make a change in openaiRename (and other similar plugins) like this:

  export function openaiRename({ 
     apiKey, 
     model 
   }: { 
     apiKey: string; 
     model: string; 
   }) { 
     const client = new OpenAI({ apiKey }); 
    
     return async (code: string): Promise<string> => { 
+    if (code === "") {
+      verbose.log("Skipping empty file");
+      return code
+    }
+
       return await visitAllIdentifiers( 
         code, 
         async (name, surroundingCode) => { 
           verbose.log(`Renaming ${name}`); 
           verbose.log("Context: ", surroundingCode);

Originally posted by @0xdevalias in #54 (comment)

[jehna]

@0xdevalias thanks for pointing this out, added a commit to verify that there's no extra processing on the top level

@j4k0xb thank you so much for your great pull requests! Creating a new release with them soon

[0xdevalias]

@0xdevalias thanks for pointing this out, added a commit to verify that there's no extra processing on the top level

Originally posted by @jehna in #134 (comment)

@jehna This was a suggestion I made only as a quick/dirty hacky workaround. By skipping files at the unminify level you technically weaken the flexibility of the plugins; as it means a plugin can no longer take potentially empty code as input, and have valid code as output. While that's technically not an issue with how things are currently implemented; the other solutions I mentioned would solve this better/more flexibly.

• Better error handling when humanify local ran on empty file (Error: Failed to stringify code) #54 (comment)