Skip to content

[llvm-lit] Fix Unhashable TypeError when using lit's internal shell #101590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 14, 2024
Merged

[llvm-lit] Fix Unhashable TypeError when using lit's internal shell #101590

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7aab48
[llvm-lit] Unhashable type Support
Harini0924 Jul 31, 2024
2bb5b51
Merge branch 'main' into fixing_unhasable
Harini0924 Aug 7, 2024
2408040
[llvm-lit] Fix handling of GlobItem in args[0] to ensure it is hashable
Harini0924 Aug 7, 2024
25b1bd7
[llvm-lit] Simplify args[0] Hashability Check and Expansion
Harini0924 Aug 8, 2024
bdea995
[llvm-lit] Fixed a comment
Harini0924 Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions llvm/utils/lit/lit/TestRunner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,13 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
# echo-appending to a file.
# FIXME: Standardize on the builtin echo implementation. We can use a
# temporary file to sidestep blocking pipe write issues.

# Ensure args[0] is hashable before using it in inproc_builtins
if isinstance(args[0], GlobItem):
expanded_args = expand_glob(args[0], cmd_shenv.cwd)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to get your feedback on the recent changes I made. Specifically, I added a check to ensure args[0] is hashable before using it in inproc_builtins. If args[0] is an instance of GlobItem, I'm expanding it to its full path. I wasn't sure if this approach is the most efficient or if there's a better way to resolve the GlobItem TypeError. Any insights or suggestions for improvement would be greatly appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a check if it's Hashable though, it's just a check if the instance is a GlobItem.

Why not just do the expansion at the call site though? expand_glob() is already checking if it's a GlobItem. It also won't modify args[0], which seems preferable, since expand_glob_expressions() goes out of its way not to modify args[0].

inproc_builtin = inproc_builtins.get(expand_glob(args[0], cmd_shenv.cwd)[0], None)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the implementation to ensure args[0] is hashable before using it in inproc_builtins. The previous code:

# Ensure args[0] is hashable before using it in inproc_builtins
if isinstance(args[0], GlobItem):
    expanded_args = expand_glob(args[0], cmd_shenv.cwd)
    if expanded_args:
        args[0] = expanded_args[0]

has been modifed to:

# Ensure args[0] is hashable
args[0] = expand_glob(args[0], cmd_shenv.cwd)[0]

You raised a valid point about performing the expansion at the call site, as expand_glob() already checks if args[0] is a GlobItem and does not modify args[0], which aligns with the approach taken by expand_glob_expressions(). However, the current error TypeError: unhashable type: 'GlobItem' occurs on the line:

inproc_builtin = inproc_builtins.get(args[0], None)

After changing this line to:

inproc_builtin = inproc_builtins.get(expand_glob(args[0], cmd_shenv.cwd)[0], None)

the same TypeError occurs on subsequent lines where args[0] is called. Instead of repeatedly applying the same functionality to args[0] until line 866 where args is expanded with:

args = expand_glob_expressions(args, cmd_shenv.cwd)

it would be more efficient to expand args[0] before invoking any in-process built-ins. This approach resolves the TypeError for this and all subsequent lines where args[0] is used.

By making this change, we address the issue holistically, ensuring the robustness of the implementation against unhashable types like GlobItem.

Let me know if this approach works for you or if you have any further suggestions.

if expanded_args:
args[0] = expanded_args[0]

inproc_builtin = inproc_builtins.get(args[0], None)
if inproc_builtin and (args[0] != "echo" or len(cmd.commands) == 1):
# env calling an in-process builtin is useless, so we take the safe
Expand Down
Loading