-
-
Notifications
You must be signed in to change notification settings - Fork 743
fix issue 9959 - Add functional pattern matching for object references #1266
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.
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
fix issue 9959 - Add functional pattern matching for object references #1266
Conversation
I have a hunch that it should be more efficient to get the typeid of the object and then do comparisons with other typeid's via |
@AndrejMitrovic: It'll probably will, because casting works for subclasses as well. And that's the problem - you usually do want it to work for subclasses. |
Give me a few minutes and I'll come up with a working example of what I mean. Also this function needs to make sure a class is tested for the most derived type before trying lesser-derived types. |
Or just always match the first choice, maybe adding an assert checking that no choices are hidden. I have had something similar in my personal toolbox for quite some time now (see https://gist.github.com/klickverbot/808936 for an older, less sophisticated version), and I agree it does come in handy from time to time. std.algorithm might not be the right module for this, though. |
Here is a crude implementation, with an added unittest: import core.exception;
import std.algorithm;
import std.exception;
import std.string;
import std.traits;
import std.typetuple;
// workaround for typeof(null) not working in DerivedToFront
private final abstract class _SentinelNullClass { }
private template GetClassTypes(T...)
{
alias choice = T[0];
alias choiceParameterTypes = ParameterTypeTuple!choice;
static assert(choiceParameterTypes.length <= 1,
"A choice function can not have more than one argument.");
static if (choiceParameterTypes.length == 1)
{
alias CastClass = choiceParameterTypes[0];
static assert(is(CastClass == class),
"A choice function can not accept a non-class-typed argument.");
alias Type = CastClass;
}
else
{
// alias Type = typeof(null); @bug@: DerivedToFront doesn't work with typeof(null)
alias Type = _SentinelNullClass;
}
static if (T.length > 1)
{
alias GetClassTypes = TypeTuple!(Type, GetClassTypes!(T[1 .. $]));
}
else
{
alias GetClassTypes = Type;
}
}
auto castSwitch(choices...)(Object switchObject)
{
alias Classes = GetClassTypes!choices;
alias OrderedClasses = DerivedToFront!Classes;
ClassInfo classInfo;
if (switchObject !is null)
classInfo = typeid(switchObject);
// try matching exact type
foreach (Class; OrderedClasses)
{
static if (is(Class == _SentinelNullClass)) // typeof(null)
{
if (switchObject is null)
{
return choices[staticIndexOf!(Class, Classes)]();
}
}
else if (classInfo == typeid(Class))
{
return choices[staticIndexOf!(Class, Classes)](cast(Class)cast(void*)switchObject);
}
}
// find first base class
foreach (Class; OrderedClasses)
{
static if (!is(Class == _SentinelNullClass)) // typeof(null) is handled above
if (auto castedObject = cast(Class)switchObject)
{
return choices[staticIndexOf!(Class, Classes)](castedObject);
}
}
// In case nothing matched:
throw new SwitchError("Input not matched by any choice");
}
///
unittest
{
class A
{
int a;
this(int a) {this.a=a;}
}
class B
{
double b;
this(double b) {this.b=b;}
}
class C
{
string c;
this(string c) {this.c=c;}
}
class D { }
Object[] arr = new Object[5];
arr[0]=new A(1);
arr[1]=new B(2.5);
arr[2]=new C("hello");
arr[3]=new D();
arr[4]=null;
auto results=arr.map!(castSwitch!(
(A a) => "A with a value of %d".format(a.a),
(B b) => "B with a value of %.1f".format(b.b),
(C c) => "C with a value of %s".format(c.c),
(Object o) => "Object of another type",
() => "null reference",
))();
assert(results[0] == "A with a value of 1");
assert(results[1] == "B with a value of 2.5");
assert(results[2] == "C with a value of hello");
assert(results[3] == "Object of another type");
assert(results[4] == "null reference");
}
unittest
{
class A { }
class B { }
//Nothing matches:
assertThrown!SwitchError((new A()).castSwitch!(
(B b) => 1,
() => 2,
)());
//Choices with multiple arguments are not allowed:
static assert(!__traits(compiles,
(new A()).castSwitch!(
(A a, B b) => 0)()
));
//Only object arguments allowed:
static assert(!__traits(compiles,
(new A()).castSwitch!(
(int x) => 0)()
));
}
// test derivation
unittest
{
class A
{
int a;
this(int a) {this.a=a;}
}
class B : A
{
double b;
this(double b) {this.b=b; super(1); }
}
class C : B
{
string c;
this(string c) {this.c=c; super(1.0); }
}
class D : C { this() { super("D class"); } }
Object[] arr = new Object[5];
arr[0]=new A(1);
arr[1]=new B(2.5);
arr[2]=new C("hello");
arr[3]=new D();
arr[4]=null;
auto results=arr.map!(castSwitch!(
(A a) => "A with a value of %d".format(a.a),
(B b) => "B with a value of %.1f".format(b.b),
(C c) => "C with a value of %s".format(c.c),
(Object o) => "Object of another type",
() => "null reference",
))();
assert(results[0] == "A with a value of 1");
assert(results[1] == "B with a value of 2.5");
assert(results[2] == "C with a value of hello");
assert(results[3] == "C with a value of D class");
assert(results[4] == "null reference");
} |
@AndrejMitrovic: You are using P.S.: I've noticed that I ignored the fact that classes can have interfaces - and by asserting that |
OK, I implemented the direct type checking and the overshadowing testing. |
Overshadowing? I just want it to work with the most derived type. If I have switches for class |
Also see the std.concurrency.receive API. |
@AndrejMitrovic: @klickverbot mentioned import std.stdio;
import std.variant;
import std.concurrency;
void spawnedFunction()
{
receive(
(long l) { writeln("Received a long."); },
(int i) { writeln("Received an int."); },
);
}
void main()
{
auto tid = spawn(&spawnedFunction);
int x=1;
send(tid, x);
} This will print This is also the case with two of my other Now, using first-fit is usually the easiest method, and sometimes it's the only possible method, and that's it became the standard - but even in cases where best-fit is possible they prefer to use first-fit for consistency, and possibly prevent an error when an early handler catches all the cases that a later handler accepts and provide an error or just a warning. If Now, if you did that in my original implementation, the |
I think this is the best solution. Pattern matching is a very common idiom across many programming languages, and it's important to match existing behavior. The fact that there is a compile-time error that removes any chance of ambiguity just seals the deal. |
There can also be a choice that accepts zero arguments. That choice will be | ||
invoked if $(D switchObject) is null. | ||
|
||
Throws: If none of the choice matches, a $(D SwitchError) will be thrown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about mentioning that the user can override this by using an Object
-taking delegate, as any class can be cast to Object
?
status? |
merge conflict, please rebase |
ping |
That last update was just a rebase - this PR is not ready for a merge yet. I still want to add the all-void-handlers and the always-throwing-void-handlers functionalities(like the ones I added to #1392). |
OK: now this PR is ready. Changes from where we left it more than a year ago:
|
@Dicebot Can you remove the needs work label please? This PR is again read for review, and you'll also bump it on the way(it is currently at the bottom of the list since I was the only one commenting here...) |
Thanks for pinging and thanks for doing needed work :) |
(I won't be able to review it until this weekend at the very least) |
Why is it still on the bottom of the list? Isn't it supposed to get bumped when a Collaborator comments? |
No. |
@jacob-carlborg I was also thinking that a bigger, united pattern matching function is in order, but I still believe there is room for |
I added support for multi-argument handlers. I still need to add a Since the single-argument |
2e6052d
to
4aa12c7
Compare
OK, the multi-argument handler implementation is complete, with updated documentation and unit tests and some fixes for some bugs that the new unit tests discovered(yey unit testing!). The main changes from the single-argument version:
@Dicebot: You warned me from over-complicating this, but while the implementation got a bit complicated due to all the edge cases, the interface is kept simple and I believe multiple argument support can be very useful here, for the same reason multi-methods are useful |
Overall concept looks ok. However I don't like few things:
Documentation / test thing is probably most difficult to address but is also one of most important. |
|
4aa12c7
to
a4e8c40
Compare
Well I'd like other reviewers to chime in on this but in my opinion one example that covers only 10% of functionality but does it clear practical fashion is much better that set of purely theoretical examples that cover 100% of the functionality. Reading latter is not so much different from just reading function documentation.
I see. What do you thing about special stub interface / class type defined in the same module named something like "MatchAny"? Not sure it is actually better though. |
@Dicebot How about this for a practical example: import std.conv;
interface Vertex {}
class Node : Vertex
{
Vertex left;
Vertex right;
this(Vertex left, Vertex right) { this.left = left; this.right = right; }
}
class Leaf : Vertex
{
int value;
this(int value) { this.value = value; }
}
string toString(Vertex vertex)
{
return vertex.castSwitch!(
(Node node) => toString(node.left) ~ " " ~ toString(node.right),
(Leaf leaf) => leaf.value.to!string(),
)();
}
Vertex tree = new Node(
new Node(new Leaf(1), new Leaf(2)),
new Node(new Leaf(3), new Leaf(4)));
assert(toString(tree) == "1 2 3 4"); As for adding something like At any rate, this multi-argument things does seem to get a little out of hand. Maybe I should revert to the old, single-argument implementation and leave the multi-argument and non-object-matches functionality to that full blown pattern-matching function I'll make in the future? |
What is the use case to match multiple arguments? |
@jacob-carlborg Well, I got the idea from this thread. I figured that Mutli-dispatch aids you in making choices based on a list of argument types. For example(the example given in that thread) - collision between two objects in a game, where you want to decide which action to take based on the types of two arguments. |
@idanarye Not looking in great detail but multi-arg type switch is really nice primitive. |
@DmitryOlshansky I also think multi-arg is nice, but it turned out to be more complex than the single-arg version and to raise some problems that don't exist or that are marginal in the single-arg version. We have to decide if we want the simpler, less-powerful version or the complex, more-powerful version. As for your renaming suggestion - when you say |
@idanarye I like that example :) |
a4e8c40
to
2cc526a
Compare
@Dicebot OK, the tree example is now in the PR. We still need to decide though if we want multi-argument here or not. |
ping |
A decision is required here - should I revert to the old, simple one-arg version or squash and keep the new, complicated multi-args version? Should I open a forum thread about it? |
Another high-level comment: At first glance, it seems like multi-arg support could transparently be added to the single-arg version. Is this so? Then, the obvious solution would be to merge the single-arg version now, and open a discussion about the improved multi-arg one. |
2cc526a
to
cf1aa96
Compare
Good idea, I think we should check in the single-arg version first, and then open another PR for the multi-arg version. It's better to have at least a subset of cases working (i.e., single-arg) then to have nothing at all. PRs that try to do too much tend to be overly complicated and/or take too long to review / refine / merge. Checking things in piecemeal, as long as each step leaves us with a usable, self-consistent subset of the ultimate functionality, is a better approach IMO. |
@klickverbot If GitHub comments had a I've reverted this commit to the single-arg version, but not before I splitted the branch with the multi-args version. Once we accept the single-arg version(shouldn't be a problem - it already got re factored to the point of acception by the consensus before I decided to add the mutli-args support. Once this PR gets accepted, I'll rebase the multi-args branch and open a new PR for it. |
LGTM - time to merge? |
Let's do it! |
Auto-merge toggled on |
…g-for-object-references fix issue 9959 - Add functional pattern matching for object references
Polymorphism is usually the way to write code that handles objects of different
types, but it is not always possible or desirable. When the code that uses the
objects is required to treat objects differently based on class, the D way is
using cast&assign inside
if
:This is not always convenient for two reasons:
obj
in everyif
statement. Ifobj
is a more complexexpression(like the return value of a function) you'll need to store it in an
variable beforehand. This is not that cumbersome but still worth mentioning.
if
statement is a statement - which means it does not return a value -and creates it's own scope - which means you variables declared in it are not
accessible outside. Those two constraints mean that if you need to compute a
value differently based on the object's class and use that value after the
if
s, you need to declare the variable before theif
- and that means youcan't make it
const
.My solution is the function
std.algorithm.castSwitch
, which is based on Scala's approach.It is used like this:
It answers both mentioned problems, plus it is more compact readable.
See http://d.puremagic.com/issues/show_bug.cgi?id=9959