Fix Issue 11067 - allow string handlers in std.sumtype.match

[niy-ati]

Summary

Apply std.functional.unaryFun / binaryFun inside matchImpl when selecting and invoking handlers, so string forms like value.match!a.member work consistently with the rest of Phobos.

Fixes #11067

Approach

Per review feedback: no separate string-handler adapter layer, and no pre-wrapping of all handlers at the match/tryMatch call site (that broke -betterC and misused SumTypes.length as handler arity). Instead, handlers are adapted only inside matchImpl via a small handlerAlias helper that delegates to unaryFun (1 arg) or binaryFun (2 args).

Test plan

• version (D_BetterC)-guarded unittest for match!a.value
• CI

[0xEAB]

The current implementation breaks -betterC compatibility.
See CI (or rerun on your own machine):

../dmd/generated/linux/release/64/dmd -conf= -I../dmd/druntime/import  -w -preview=dip1000 -preview=dtorfields -preview=fieldwise -m64 -fPIC -O -release -defaultlib= -debuglib= -L-lpthread -L-lm -of=generated/linux/release/64/betterctests/betterc_module_tests -betterC -unittest test/betterc_module_tests.d std/sumtype.d
std/exception.d(521): Error: cannot use `throw` statements with `-betterC`
        throw new E(msg ? msg.idup : "Enforcement failed", file, line);
        ^
std/exception.d(442): Error: template instance `std.exception.bailOut!(Exception)` error instantiating
        if (!value) bailOut!E(file, line, msg);
                    ^
std/uni/package.d(4087):        instantiated from here: `enforce!bool`
        enforce(idxB >= idxA && idxA >= curIndex, errMsg);
               ^
std/uni/package.d(4427):        instantiated from here: `TrieBuilder!(bool, dchar, 1114112, sliceBits!(13LU, 21LU), sliceBits!(8LU, 13LU), sliceBits!(0LU, 8LU))`
    alias CodepointTrie = typeof(TrieBuilder!(T, dchar, lastDchar+1, Prefix)(T.init).build());
                                 ^
std/uni/package.d(10789):        instantiated from here: `CodepointTrie!(bool, 8, 5, 8)`
    return const(CodepointTrie!T)(e.offsets, e.sizes, e.data);
                 ^
std/uni/package.d(10878):        instantiated from here: `asTrie!(bool, 8, 5, 8)`
        static immutable res = asTrie(controlTrieEntries);
                                     ^
std/uni/package.d(7125): Error: CTFE failed because of previous errors in `graphemeExtendTrie`
        static assert(!graphemeExtendTrie['\u200D']);
                       ^
std/uni/package.d(7125):        while evaluating: `static assert(!graphemeExtendTrie()['\u200d'])`
        static assert(!graphemeExtendTrie['\u200D']);
        ^
std/meta.d(566): Error: template instance `std.functional.fun!("a.x", "a")` error instantiating
        staticMap = AliasSeq!(staticMap, fun!arg);
                                         ^
std/sumtype.d(1987):        instantiated from here: `Map!(unaryFun, "a.x", (a) => a)`
        alias adaptHandlers = Map!(unaryFun, handlers);
                              ^
std/sumtype.d(1733):        instantiated from here: `adaptHandlers!(1LU, "a.x", (a) => a)`
        return matchImpl!(Yes.exhaustive, adaptHandlers!(SumTypes.length, handlers))(args);
                                          ^
std/sumtype.d(1743):        instantiated from here: `match!(SumType!(Point, int))`
    assert(v.match!("a.x", a => a) == 2);
            ^
std/array.d(130): Error: cannot use try-catch statements with `-betterC`
        try
        ^
std/uni/package.d(2185): Error: template instance `std.array.array!(CowArray!(GcPolicy))` error instantiating
        return Intervals!(typeof(data.array))(data.array);
                                     ^
std/uni/package.d(1978):        instantiated from here: `InversionList!(GcPolicy)`
public alias CodepointSet = InversionList!GcPolicy;
                            ^
make[1]: *** [Makefile:718: generated/linux/release/64/betterctests/betterc_module_tests] Error 1

[pbackus]

I am strongly opposed to this, and would need a much stronger reason to convince me than "because other Phobos modules do it."

unaryFun is a workaround/hack from before D had built-in function literal syntax. While we cannot remove it from existing APIs without breaking backwards compatibility, there is no need to add it to new APIs. It offers no new functionality compared to built-in lambdas, and has several significant downsides (e.g., one cannot use imported symbols in the body of a string passed to unaryFun, because of how string mixin interacts with scoping).

[LightBender]

I am in complete concurrence with @pbackus on this one. Phobos V3 will be removing the string handlers entirely and only support function literal syntax. This is retained in Phobos 2 for backwards compatibility only.

[FeepingCreature]

String literal syntax is not deprecated. There isn't even documentation guidance that it should be avoided. Quite the opposite, the documentation references it heavily- probably because it reads a lot easier than lambdas. As it is in the language, it should be in the language uniformly.

To be frank, I think the lambda syntax is not good for simple access expressions, as are very common in std.algorithm use. There's a reason that every doc example of "terse D" uses string form whenever it can. (edit: And there's a reason that every language with heavy lambda use evolves a shortcut field access syntax...)

[rikkimax]

String literal syntax is not deprecated. There isn't even documentation guidance that it should be avoided. Quite the opposite, the documentation references it heavily- probably because it reads a lot easier than lambdas. As it is in the language, it should be in the language uniformly.

It's not deprecated; it won't be removed from PhobosV2.

But it also shouldn't move forward into PhobosV3.

[FeepingCreature]

I have no opinion yet on Phobos V3. This PR is to V2 however.

[FeepingCreature]

okay update I just looked at the diff and I kind of regret raising the issue now. I was hoping we could just invoke unaryFun for it... if this is what it takes, it's probably not worth it. I may disagree about unaryFun and the string syntax, but we probably should not add a second, totally independent system for this problem.

I'm also not sure what you were trying to do with nArgs in that first commit? The number of sumtype types has no connection to the number of parameters on the match handlers. I thought the approach would be to just chuck every match template arg into unaryFun.

edit: I tested it locally and it seems to work? Not sure what the betterC issue was. Try just doing it like this?

diff --git a/std/sumtype.d b/std/sumtype.d
index 44be70a7f..5bd66b2dc 100644
--- a/std/sumtype.d
+++ b/std/sumtype.d
@@ -1833,6 +1833,16 @@ template match(handlers...)
     assert(!sameDimensions(d, b));
 }
 
+@safe unittest
+{
+    struct Foo { int value; }
+    struct Bar { int value; }
+
+    alias Sum = SumType!(Foo, Bar);
+
+    assert(Sum(Bar(3)).match!"a.value" == 3);
+}
+
 /**
  * Attempts to call a type-appropriate function with the value held in a
  * [SumType], and throws on failure.
@@ -1961,6 +1971,8 @@ private template matchImpl(Flag!"exhaustive" exhaustive, handlers...)
     auto ref matchImpl(SumTypes...)(auto ref SumTypes args)
     if (allSatisfy!(isSumType, SumTypes) && args.length > 0)
     {
+        import std.functional : unaryFun;
+
         // Single dispatch (fast path)
         static if (args.length == 1)
         {
@@ -2033,7 +2045,7 @@ private template matchImpl(Flag!"exhaustive" exhaustive, handlers...)
             {
                 static foreach (hid, handler; handlers)
                 {
-                    static if (canMatch!(handler, valueTypes!caseId))
+                    static if (canMatch!(unaryFun!handler, valueTypes!caseId))
                     {
                         if (result[caseId] == noMatch)
                         {
@@ -2066,7 +2078,7 @@ private template matchImpl(Flag!"exhaustive" exhaustive, handlers...)
 
         static foreach (size_t hid, handler; handlers)
         {
-            mixin("alias ", handlerName!hid, " = handler;");
+            mixin("alias ", handlerName!hid, " = unaryFun!handler;");
         }
 
         // Single dispatch (fast path)

[niy-ati]

thanks everyone for the feedback , I've reworked the branch based on it.

What changed now:
Dropped the separate matchStringHandler/adaptHandlers layer entirely. no second string-handler system.
Dropped pre wrapping all handlers at the match/tryMatch call site (that was the -betterC breakage @0xEAB flagged, and it wrongly used SumTypes.length as if it were handler arity).
Handlers are now adapted only inside matchImpl, via a small handlerAlias helper that delegates to std.functional.unaryFun (1 arg) or binaryFun (2 args), same approach as your suggestion, extended for multiple dispatch.
The string-handler unittest is behind version (D_BetterC) so the -betterC module test doesn't pull in the CTFE validation path.

Follow-up fix
The first rework had a compile bug: valueTypes!caseId.length is wrong in single dispatch, where valueTypes!caseId is a single type, not an AliasSeq. Fixed in 6d6e797 by using SumTypes.length for handler arity in the canMatch check.

open to adjust further if anything still looks off.

[FeepingCreature]

I think you're still misunderstanding the nArgs logic, or well one of us is. If the sumtype has 2 possible types, that doesn't mean the match callback will be called with two arguments. They're mutually exclusive.

[niy-ati]

You're right @FeepingCreature about the variant/arity distinction ,I had muddled that in my explanation.

SumType!(Foo, Bar) has two mutually exclusive alternatives, but a handler still
receives exactly one value. Variant count (Types.length / numCases) is not
handler arity.
What SumTypes.length means inside matchImpl is different: it's the number of
SumType arguments passed to match (1 for single dispatch, 2 for multiple
dispatch), not the number of alternatives inside one SumType. So for

SumType!(Foo, Bar)(Bar(3)).match!"a.value"

SumTypes.length is 1, and unaryFun is the right adapter. I should have said
that clearly,
That said, #11067 only needs the single-dispatch case, and the handlerAlias /
nArgs / binaryFun machinery is more than the issue requires. I'll drop it and
switch to the approach you suggested: always wrap with unaryFun!handler in
matchImpl (for both canMatch and the handler aliases). Non-string handlers
are unchanged because unaryFun already identity-aliases them.

[FeepingCreature]

Oh you're right! Yeah, that's my bad. I totally forgot about the multi-sumtype matching because I have never used it. Mea culpa!

That makes your template approach make a lot more sense tbh.

[pbackus]

Even with the current approach, using unaryFun and binaryFun introduces an inconsistency, where dispatching on 1 or 2 SumTypes supports string lambdas but dispatching on 3 or more SumTypes does not.

In practice I don't expect the 3+ case will come up very often, but it still needs to be documented, and inevitably someone will see it in the future and submit a PR adding nAryFunction to try and address it...

[niy-ati]

@FeepingCreature no worries, thanks for the correction. I've restored the
arity-based handlerAlias approach (unaryFun for 1 SumType argument,
binaryFun for 2).

@pbackus good catch on the 1/2 vs 3+ inconsistency. I've documented that
string handlers are only supported when matching one or two SumTypes (same
ceiling as std.functional), and string handlers with 3+ SumTypes now get a
clear compile error pointing people at function literals. I'm not adding an
nAryFunction that would expand the surface without much practical need.

[pbackus]

@niy-ati Thank you for updating the documentation.

My overall position on this PR has not changed. The entire rationale for adding this feature seems to be

1. Other modules in Phobos use unaryFun and binaryFun, so std.sumtype should too.
2. It allows certain calls to match to be written more concisely.

I find (1) unconvincing because other Phobos modules only use unaryFun and binaryFun for legacy reasons that are no longer applicable. I find (2) unconvincing because there are ways to make such calls more concise without requiring any changes to match (see issue #11067 for details).

Meanwhile, the downside of this PR is that it makes the implementation of match even more complex and slower to compile than it already is, while being completely redundant with existing functionality. I do not think making a few calls to match slightly more concise is worth (a) making every match call slightly slower to compile, and (b) taking on the maintenance burden and risk of bugs that comes with a more complex implementation.

[0xEAB]

Meanwhile, the downside of this PR is that it makes the implementation of match even more complex and slower to compile than it already is

Out of curiosity, would someone from the supporters crowd mind to benchmark this?

[pbackus]

I don't expect that this PR, on its own, will have a significant impact on compile times. The real problem (for which I take full responsibility, to be clear) is that match is already slower than many users would like it to be, and any change that introduces more mandatory template instantiations will make efforts to speed it up in the future more difficult.

[FeepingCreature]

I think D has a hard enough time convincing people to use it without forcing them to memorize valid expression syntax per call site. String syntax is supported. I happen to think it's good that it's in the language, (or rather, not exactly "good" but better than nothing), but independent of all that it's in the docs and in the examples and it works (I think) everywhere except here. If string syntax was marked as deprecated, legacy and unsupported that'd be something else, but it's not even documented as such in the docs page for unaryFun. How is some user supposed to know this sentiment? I didn't even know this sentiment. Intuitively, if you know string lambdas (and the docs give you many opportunities to learn them) you would absolutely expect them to work in match.

[pbackus]

The simplest approach for new D programmers is to never teach them the string syntax in the first place. This is what Programming in D does—see for example the chapters on ranges and lambdas, neither of which includes the string syntax.

If string syntax was marked as deprecated, legacy and unsupported that'd be something else, but it's not even documented as such in the docs page for unaryFun.

I would be happy to submit a PR to fix this.

[FeepingCreature]

Thank you. I mean, I like strings and wish they'd stay (wish they'd be replaced with better syntax, alas...), but either soft-deprecating or warning-off string syntax or this PR should be done. Anything but "we'll behave like they're deprecated but they're not deprecated and also it's not written down anywhere."

[0xEAB]

Anything but "we'll behave like they're deprecated but they're not deprecated and also it's not written down anywhere."

That’s only fair.
I’ve always found this “deprecation by convention” (or “deprecation by lore”) pattern kinda strange — see also: @property.