faf922302 breaks everything

[juleek]

After this commit: faf92230221edcf6e62dd622cdff9ba947ca76c1, I started getting:

~/.local/share/omf/themes/bobthefish/functions/fish_prompt.fish (line 1261): Unknown builtin 'path'
        echo (builtin path sort -r $argv)[1]
              ^
from sourcing file ~/.local/share/omf/themes/bobthefish/functions/fish_prompt.fish
in command substitution

fish --version
fish, version 3.3.1

Using this version because this is the default in 22.04

[juleek]

cc @bobthecow

[juleek]

this is interesting... I see you had a check (if builtin -q path), but apparently it is not working for some reason during source-ing (?):

$ builtin -q path

$ echo $status
1

$builtin path
fish: Unknown builtin 'path'

Yeah, I think this is what going on, fish for some reason parses and checks builtin even if the condition is false:

if false
   builtin path basename /a

this ^^^^^^ results in the error.

[juleek]

As a temporary workaround:

sed -i 's/builtin path /path /g' ~/.local/share/omf/themes/bobthefish/functions/fish_prompt.fish

[bobthecow]

Sorry, and thanks for looking into this! That's weird. I have no idea why the builtin check isn't gating that line properly.

I'm unfortunately unable to dig into this for a couple of weeks. If you can come up with a workaround I'd appreciate a PR. Otherwise your best bet might be checking out the revision just before that change until I can get a chance to work on it.

[rasan000]

@bobthecow @juleek I had encounted the same problem. i rewrote fish_prompt.fish as follows and it workd fine.

Putting $ in front of path.

(row 1259~1265)
function __bobthefish_closest_parent -S
    if builtin -q $path
        echo (builtin $path sort -r $argv)[1]
    else
        string join \n $argv | awk '{ print length, $0 }' | sort -nsr | head -1 | cut -d" " -f2-
    end
end

However, I dont't use linux much and my environment is virtual. So I'm not sure if this is a solution or if it will cause new problems.

The environment is as follower

• windows11 wsl(ubuntu22.04)

[bobthecow]

Putting $ in front of path.

    if builtin -q $path

        echo (builtin $path sort -r $argv)[1]

This is working for you because it's checking whether there's a builtin with the value of your system $path variable, which will almost certainly be false for everyone all the time. You could also prevent the issue by just deleting the check entirely and returning the "else" branch of the if statement. That wouldn't help people who do have a working path builtin though :)

[jean-bernard-valentaten]

Just a wild guess based on observation. I put an echo "FOO" in front of said line and proceeded to omf reload:

> omf reload
...
~/.local/share/omf/themes/bobthefish/functions/fish_prompt.fish (line 1262): unbekannter interner Befehl 'path'
        echo (builtin path sort -r $argv)[1]
              ^
from sourcing file ~/.local/share/omf/themes/bobthefish/functions/fish_prompt.fish
in command substitution

Yet my poor-mans-debugger was nowhere to be seen. My guess is that fish somehow tries to resolve the builtin commands it finds in scripts before actually executing them. Might be some preemptive fetching thing or maybe builtin is replaced by some byte-code or whatever in memory in order to speed up things. Anyway that triggered the idea to circumvent this issue by writing the builtin command to a string and have it executed:

function __bobthefish_closest_parent -S
    if builtin -q path
        set -l pathCmd "builtin path"
        echo ($pathCmd sort -r $argv)[1]
    else
        string join \n $argv | awk '{ print length, $0 }' | sort -nsr | head -1 | cut -d" " -f2-
    end
end

After omf reload the issue is gone. Unfortunately I cannot test this with fish >=3.5.0, but maybe someone can test and confirm whether this workaround works with fish that has the builtin. Here's the patch (I cannot attach the file unfortunately):

diff --git a/functions/fish_prompt.fish b/functions/fish_prompt.fish
index e1df158..f53fd8b 100644
--- a/functions/fish_prompt.fish
+++ b/functions/fish_prompt.fish
@@ -1258,7 +1258,8 @@ end
 # Polyfill for fish < 3.5.0
 function __bobthefish_closest_parent -S
     if builtin -q path
-        echo (builtin path sort -r $argv)[1]
+        set -l pathCmd "builtin path"
+        echo ($pathCmd sort -r $argv)[1]
     else
         string join \n $argv | awk '{ print length, $0 }' | sort -nsr | head -1 | cut -d" " -f2-
     end

[bobthecow]

If you extract the builtin path line into its own function (which is only called in the conditional branch) does it explode? (If so) what if you move that function into its own file under the functions dir?

[jean-bernard-valentaten]

@bobthecow i just added the following function:

function __builtInPath -S
   echo (builtin path sort -r $argv)[1]
end

The function is not called anywhere, it's mere presence is enough to produce the error.

[bobthecow]

I suspected as much. What if you move it into its own file (with the name of the function) under the functions directory, but never call it or source the file?

[bobthecow]

If I understand it correctly, https://github.com/fish-shell/fish-shell/commit/0325c1ba659a2ecd18c5765437b37077cf40ad5c added parse-time errors for builtins (11 years ago!). I’m not sure if there is a way to do conditional builtin calls or not, other than by something equivalent to “eval” as you mentioned. but, there’s a chance we can avoid parsing it entirely, by moving the offending call into a file with its own function. fish function autoloading probably is runtime, not parse time, so my guess is that it’ll work.

[bobthecow]

Since there’s no way for the existing thing to safely work, i’ve reverted the change in HEAD. Let’s keep this issue open for a bit and see if we can come up with a way to make it work.

[jean-bernard-valentaten]

Here's what I tried and what the outcome was:

1. Move the function __bobthefish_closest_parent to the file functions/__bobthefish_closest_parent.fish => Upon reload of the theme, I get the exact same error as when the function is in the file functions/fish_promt.fish
2. Move the gated part of the function to the file functions/__bobthefish_get_path_by_builtin.fish and in the true branch of the if (line 1261) call said function => This does not produce an error with my fish version, so it might be a way to circumvent the issue. You'd need to check whether it works with fish >= 3.5.0.

Here's the function:

function __bobthefish_get_path_by_builtin -S
    echo (builtin path sort -r $argv)[1]
end

HTH

[bobthecow]

yeah, after checking the error implementation, just the gated bit would have to be in an autoloaded file, because we have to prevent that file from ever being parsed.

thanks for looking into it. i'll take a shot at getting an improved implementation out in a couple of weeks, but in the meantime you can update to the latest theme version and pick up the revert.