This is awesome! I can see a lot of scripts being simplified by this! I agree with krobelus that we do not need the --import option, we should just always do it!

Please update the string-match.rst docs. In particular we should clarify the behavior on multiple arguments.

An unresolved question is what scope the variables get set in. I don't think anybody wants a bunch of additional scope flags like we have for read, so maybe it always gets set as ENV_LOCAL?

Thanks, guys. I was thinking of things that could be done to improve the correctness of shell scripts, and struck upon this as low-hanging fruit that could provide some limited safety by at least reducing manual indexing and string manipulation, if not also guaranteeing the shape of the resulting variables.

I'm cautiously happy to try making it import by default. I think it could really go some ways to make regex in fish nicer, but with some caveats pertaining to scope and specific variables (more below). I did not think of doing $1, $2, etc. for unnamed groups - that's certainly a thought worth exploring!

In addition to the odd behavior that @krobelus spotted (thanks, I had focused on the behavior of single matching vs non-matching and subsequent --all matching and non-matching, but completely missed --all not matching even the first time around -- my intention is for the same behavior as the single non-matching), there are a few other issues which (in addition to updating the docs and release notes) are part of the reason I opened a PR:

• Currently the name table is re-enumerated for each result because I was originally shoe-horning this code into report_match() which doesn't have any "big picture" access, but ended up being forced to move it to the matcher itself. Splitting this into an init function that enumerates the names and creates a lambda that can be used in case of a match and in case of no match will clean up both the repeated enumeration and provide a way to clear the vars when no matches are found.
• I didn't do more than a cursory glance at the PCRE2 spec to see if a valid PCRE2 group name is always a valid fish variable name. I think they are, but we need to verify that (and figure out what to do if they're not).
• What do we do when an illegal variable name is specified, e.g. version or fish_pid?

I intentionally did not introduce scoping controls and of course @ridiculousfish caught that immediately :) I think we're both in agreement about not wanting to add too many knobs here; I have been giving scopes some thought, and I think something very similar to the default set behavior (function-local unless already extant) makes the most sense as a starting point? This is definitely going off topic, but my "ideal scope" for an interactive scripting language like a shell would be "for writes, universal and global variables are never modified implicitly, and default scope is local unless a variable is previously defined within the function; for reads, all scopes are considered." e.g. very close to the current default function-local behavior, but more conservative within blocks (e.g. let instead of var, in JS-speak).

I don't think hard-coding ENV_LOCAL as the scope would work: I can easily envision cases where you want the variables to be exposed outside the block, e.g.

# consider both with and without the following two lines
set word1
set word2

if test $version -gt 2
    foo | string match -rq "(?<word1>hello) (?<word2>world)"
else
    foo | string match -rq "(?<word1>adios), (?<word2>amigo)"
end

echo $word1 $word2

With a hard-coded local scope, the function-local declarations on line 2 would not help hoist the scope of the variables, right? (Also, ENV_DEFAULT is the same behavior as an unadorned set, right?)

The CI is failing on Linux/Clang and macOS. I've tested both Linux/Clang and macOS/gcc and can't reproduce, they run fine for me. Are those environments using a different PCRE2 or perhaps the system PCRE2?

Are those environments using a different PCRE2 or perhaps the system PCRE2?

The second Travis build uses the vendored PCRE2, the rest, best as I can tell, uses system PCRE2. Presumably the system pcre should be the same version across the linux builds at least, so I don't think that's the issue (as there are two successes and one failure with it)

What do we do when an illegal variable name is specified, e.g. version or fish_pid?

Right this currently allows overriding $version!
A fairly simple solution would be to use ENV_USER on vars.set and reuse handle_env_return.
That would suggest refactoring the logic to set each captured variable at most once per string match.
(We could also try to report the error earlier, before matching anything but that's a detail.)

I did not think of doing $1, $2, etc. for unnamed groups

On a second thought, $1 and so on are not necessary at all because we already have this:

set matches (foo | string match -rq "(?<word1>hello) (?<word2>world)")
set word1 $matches[1]
set word2 $matches[2]

The scoping issue is tricky. Probably the current default does the right thing most of the time:
function-local inside functions, otherwise global.

my "ideal scope" for an interactive scripting language like a shell would be "for writes, universal and global variables are never modified implicitly, and default scope is local unless a variable is previously defined within the function; for reads, all scopes are considered."

Agreed, I would also prefer explicit writes for universals and globals - when inside a function. Outside functions, writing globals by default seems fine. Python does this well IMO.

The little-check tests do not report stderr? It turns out I was running into an edge case of maybe_t, holding on to the (still extant) shared_ptr value after the temporary maybe_t was dropped, which triggered a fatal ASAN exception.

I know I've seen ASAN errors reported in the CI previously, but it seems they're silently dropped when little-check is executing.

I've pushed out a refactored version of the code that should address the performance concerns I had as well as fix the divergent behavior of importing variables with and without --all in case of no match. The code is in place to handle and bubble up failed variable name validation, but the validation itself is still not done; the patch still requires an explicit --import though it seems that we have consensus on dropping that so I'll include that in the next update.

The little-check tests do not report stderr

They should, I get an error if I run int foo[2]; return foo[2];

fish-shell/tests > HOME=$PWD ../build-asan/littlecheck.py -sfish=../build-asan/fish checks/regex-import.fish
Failure in checks/regex-import.fish:

  There were no remaining checks left to match stderr:1:
    ../src/fish.cpp:409:12: runtime error: index 2 out of bounds for type 'int [2]'

  when running command:
    ../build-asan/fish checks/regex-import.fish

Hmmm. What if little check errors out on an stdout mismatch first, would it also report any "incidental" stderr output that was generated simultaneously?

Let's test!

#RUN: %fish %s

echo foo
# CHECK: foo

echo stderr >&2
# CHECKERR: stderr
echo stderr2 >&2

echo right
# CHECK: wrong

echo stderr3 >&2

Run build_tools/littlecheck.py -s fish=/usr/bin/fish test.fish, and the output is:

Failure in test.fish:

  The CHECK on line 11 wants:
    wrong

  which failed to match line stdout:2:
    right

  additional output on stderr:2:
    stderr2

  Context:
    foo
    right <= does not match 'wrong'
    
  when running command:
    /usr/bin/fish test.fish

So it tells you that there is output on stderr, but only gives you the first line of it.

You can also see this on Travis:

additional output on stderr:1:

    =================================================================

(and yes, asan's first line of output is entirely useless)

We could probably let it show more (all?) of it, or we write it to a logfile and let you check it?

Thanks for clarifying the behavior, @faho. I think the problem is the use of line-based string comparison rather than diffing, which means there could (theoretically) be thousands of lines waiting to be matched you wouldn't want spewed to the console just because one stderr line failed to match, whereas with a diff-based approach you could be more certain that all the mismatch is relevant. The little-check diff patches will fix this, right?

I pushed out the (final?) code updates, only documentation and changelog updates remaining I believe.

I pushed out the (final?) code updates, only documentation and changelog updates remaining I believe.

Looks good, I don't think there's another blocker left.

If multiple arguments are specified, a named capture will be extracted from the first argument that matches.

Note: string has been made a reserved word in aa89564, which means it can no longer be used as a function name.

That's because it can't be wrapped correctly anymore after this PR - even with --no-scope-shadowing (and people thinking of using that is a stretch to begin with) it would necessarily break setting argv.

This is in line with set, read and argparse.

The only thing I'm struggling with here is the description of the variables being "imported". To me that sounds like the value of the variable is brought into the match, rather than the results of the match being set externally. Is there a better way of phrasing it? Maybe as "string match automatically sets the results all named capturing groups ((?<name>expression)) as the values of fish variables of the same name"?

"import" sort of makes sense as opposite of "exported" environment variables.
It's still weird because "exported" is used to talk about environment variables, but we import plain shell variables.

So I'm in favor of rebranding, maybe something like this (for string-match.rst and the changelog):

string match integrates with PCRE2 named capture groups: when a regex like (?<name>expression) matches, then the variable name will be set to the substrings that matched expression.