[llvm] [llvm-lit] Fix `TypeError` string argument expected in lit's internal shell (PR #105925)

Alexander Richardson via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 27 00:02:14 PDT 2024


================
@@ -413,7 +413,7 @@ def maybeUnescape(arg):
         for arg in args[:-1]:
             stdout.write(encode(maybeUnescape(arg)))
             stdout.write(encode(" "))
-        stdout.write(encode(maybeUnescape(args[-1])))
+        stdout.write(encode(maybeUnescape(str(args[-1]))))
----------------
arichardson wrote:

I would much prefer using `cmd.args = expand_glob_expressions(cmd.args, shenv.cwd)` to match mkdir and rm, but really the internal shell should expand globs before calling any builtin command. However, that is a more invasive change and should be done in a follow-up.

The fact that it only happens on the last arg indicates that we are missing test coverage and are most likely not doing the right thing: `str()` will just convert the glob to a string instead of expanding it.

I also notice we already expand the first argument:
```
        # Ensure args[0] is hashable.
        args[0] = expand_glob(args[0], cmd_shenv.cwd)[0]
```
I missed https://github.com/llvm/llvm-project/pull/101590, but it seems odd just performing glob expansion on the first item - I'd strongly suggest adding some tests for glob expansion and expanding the whole command - this would allow dropping the special handling from rm/mkdir. Also does this mean we don't expand globs when calling external commands?

https://github.com/llvm/llvm-project/pull/105925


More information about the llvm-commits mailing list