Fix issue #13352 - Only perform Algebraic arithmetic with allowed types. #2452

s-ludwig · 2014-08-22T09:39:55Z

Adds a static check allowed!T before trying any particular numeric type. Also checks for an exact type match in case of a raw (non-Variant) operand and prefers that to one of the fixed numeric types. Finally, it loosens the restriction for opArithmetic to only work for equal Variant types.

https://issues.dlang.org/show_bug.cgi?id=13352

s-ludwig · 2014-08-22T10:09:46Z

Hm, it seems like the rest of VariantN uses assert(false) instead of throwing exceptions. Although that feels a little fragile, that probably means that I should be doing that in opArithmetic, too. Any thoughts?

s-ludwig · 2014-08-22T10:14:40Z

Ah scratch that, there is VariantException used for this in several places.

bearophile · 2014-08-22T18:59:17Z

I think it's not a good idea to allow arithmetic operations among values of Algebraic types. I think functional languages don't do that because it muddles the semantics of what an Algebraic data type is.

yebblies · 2014-08-22T19:13:09Z

I think it's not a good idea to allow arithmetic operations among values of Algebraic types. I think functional languages don't do that because it muddles the semantics of what an Algebraic data type is.

It you think of Algebraic as a restricted Variant then it makes sense.

bearophile · 2014-08-22T19:40:26Z

I want to pattern match with a switch on an Algebraic. I don't want to read code written by other people where there's a + among two Algebraic values.

Please take a look at what an algebraic data type is, its purposes, look at how they are used and meant in functional languages like Haskell and ML where they are native, and don't add random features to them just because you can in D.

Sometimes the point of a language (or library) construct comes from what you can't do with it (like you can't mutate a const, can't print from a pure function, etc).

s-ludwig · 2014-08-22T19:55:07Z

The thing is that this is already possible and I don't see that it's going to be removed. Based on that I think that crippled support for such operations is worse than one with clean semantic rules (even if even with this pull request they are still far from perfect).

Apart from that, there are use cases where such behavior is definitely desired (JSONValue), but maybe there should have been two distinct types with one possibly based on the other one. What I'd also really like to have is a kind of TaggedAlgebraic (maybe there is a better name) with a type ID that can be used with switch for pattern matching.

s-ludwig · 2014-08-22T23:00:01Z

BTW, what's up with the autotester, It seems to just constantly retest everything since many hours. The same seems to be true for most other open pull requests. @braddr, any idea, or is this normal behavior?

quickfur · 2014-08-22T23:42:34Z

AFAIK, whenever any of dmd, druntime, or phobos changes (nowadays basically due to PR merges), the autotester will restart from scratch. I suppose the idea is to ensure that test results are always with the latest bleeding-edge toolchain, and that automerges don't introduce problems into the repos because a code conflict or incompatibility arises between a recently-merged PR and a PR being automerged, that the autotester would miss if it didn't reset everything after each merge.

mihails-strasuns · 2014-08-23T01:23:22Z

Yes, this is the case. You can open "Details" to see if there were failures in build history though.

mihails-strasuns · 2014-08-23T01:28:18Z

I agree with @s-ludwig about this - while strict tagged union based algebraic type for pattern matching may be interesting addition, changing anything fundamental about existing implementation is not an option and needs to be as good as possible in already defined niche. That PR does the right thing in my opinion.

If anything I am more concerned about new restrictions breaking any of existing code. Are there any cases where it did compile before (even if worked wrong) and won't after the PR is merged?

s-ludwig · 2014-08-23T11:22:40Z

I'm relatively sure that it should only extend the type combinations that work. Cases that before failed for sure, because the result was a !allowed!T type, can now fall back to another, possibly allowed type.

The future direction should probably be to extend the code, so that it iterates over the actual types of the Algebraic instead of just trying the built-in numeric types. But that needs some care so that the priority and ambiguity rules of the language are properly mirrored.

quickfur · 2014-09-04T17:33:58Z

ping
Should we merge this?

mihails-strasuns · 2014-09-06T16:47:42Z

This is in dire need of more reviewers (change is not trivial) @DmitryOlshansky @monarchdodra @JakobOvrum ?

DmitryOlshansky · 2014-09-08T07:35:51Z

I recall looking at this PR before and overall I like it. Will re-review later today.

ghost · 2014-09-10T07:57:23Z

std/variant.d

+            string tryUseType(string tp) {
+                import std.string : format;
+                return q{
+                    static if (allowed!%s && T.allowed!%s)


You can use %1$s for the format specifier everywhere and then pass tp only once.

Adds a static check allowed!T before trying any particular numeric type. Also checks for an exact type match in case of a raw (non-Variant) operand and prefers that to one of the fixed numeric types. Finally, it loosens the restriction for opArithmetic to only work for equal Variant types. Conflicts: std/variant.d

ghost · 2014-09-10T21:11:17Z

Ok, it LGTM now. Anyone else have comments?

mihails-strasuns · 2014-09-11T05:53:39Z

Still LGTM :)

JakobOvrum · 2014-09-11T06:21:12Z

std/variant.d

-                result = mixin("get!(double) " ~ op ~ " other.get!(double)");
-            else
-                result = mixin("get!(real) " ~ op ~ " other.get!(real)");
+            string tryUseType(string tp) {


This should use the Phobos bracket style

ghost · 2014-09-11T08:14:01Z

Auto-merge toggled on

Fix issue #13352 - Only perform Algebraic arithmetic with allowed types.

mihails-strasuns added the needs review label Sep 6, 2014

s-ludwig force-pushed the issue_13352 branch from 5ef6476 to c39b621 Compare September 8, 2014 07:45

ghost reviewed Sep 10, 2014
View reviewed changes

s-ludwig added 4 commits September 10, 2014 12:52

Use VariantException instead of a bare Exception.

7b805bd

Use positional arguments for format().

fd5ff8a

Don't throw static exceptions.

6c04790

s-ludwig force-pushed the issue_13352 branch from c39b621 to 6c04790 Compare September 10, 2014 10:54

JakobOvrum reviewed Sep 11, 2014
View reviewed changes

Code clean up.

c9d8ee5

ghost pushed a commit that referenced this pull request Sep 11, 2014

Merge pull request #2452 from s-ludwig/issue_13352

67db0d6

Fix issue #13352 - Only perform Algebraic arithmetic with allowed types.

ghost merged commit 67db0d6 into dlang:master Sep 11, 2014

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #13352 - Only perform Algebraic arithmetic with allowed types. #2452

Fix issue #13352 - Only perform Algebraic arithmetic with allowed types. #2452

s-ludwig commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

bearophile commented Aug 22, 2014

yebblies commented Aug 22, 2014

bearophile commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

quickfur commented Aug 22, 2014

mihails-strasuns commented Aug 23, 2014

mihails-strasuns commented Aug 23, 2014

s-ludwig commented Aug 23, 2014

quickfur commented Sep 4, 2014

mihails-strasuns commented Sep 6, 2014

DmitryOlshansky commented Sep 8, 2014

ghost Sep 10, 2014

ghost commented Sep 10, 2014

mihails-strasuns commented Sep 11, 2014

JakobOvrum Sep 11, 2014

ghost commented Sep 11, 2014

Fix issue #13352 - Only perform Algebraic arithmetic with allowed types. #2452

Fix issue #13352 - Only perform Algebraic arithmetic with allowed types. #2452

Conversation

s-ludwig commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

bearophile commented Aug 22, 2014

yebblies commented Aug 22, 2014

bearophile commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

s-ludwig commented Aug 22, 2014

quickfur commented Aug 22, 2014

mihails-strasuns commented Aug 23, 2014

mihails-strasuns commented Aug 23, 2014

s-ludwig commented Aug 23, 2014

quickfur commented Sep 4, 2014

mihails-strasuns commented Sep 6, 2014

DmitryOlshansky commented Sep 8, 2014

ghost Sep 10, 2014

Choose a reason for hiding this comment

ghost commented Sep 10, 2014

mihails-strasuns commented Sep 11, 2014

JakobOvrum Sep 11, 2014

Choose a reason for hiding this comment

ghost commented Sep 11, 2014