Convert range constraints to Conjunctive Normal Form by andralex · Pull Request #5461 · dlang/phobos

andralex · 2017-06-11T00:23:02Z

Convert the currently opaque and unstructured constraints for ranges to simple CNF formulae. This makes the constraints simpler and clearer. But the largest opportunity is having the compiler recognize these formulae and print the failing clause. There are plans to include a mechanism for user-defined messages, too.

andralex · 2017-06-11T00:24:03Z

For some reason I can't squash the commits together with git rebase -i HEAD~4. Help please?

wilzbach · 2017-06-11T00:25:51Z

For some reason I can't squash the commits together with git rebase -i HEAD~4. Help please?

Should I squash everything into one or only the last four?

andralex · 2017-06-11T00:30:10Z

Should I squash everything into one or only the last four?

https://www.youtube.com/watch?v=KBKXu3Kg4yg

dnadlinger · 2017-06-11T00:30:39Z

Somehow, you ended up with a series of merges:

*   d3f75790a - (HEAD -> cnf, dlang/pull/5461, andralex/cnf) Convert range constraints to Conjunctive Normal Form (20 minutes ago)<Andrei Alexandrescu>
|\
| *   3b658fe94 - WIP (10 hours ago)<Andrei Alexandrescu>
| |\
* | \   4a0b99ace - WIP (10 hours ago)<Andrei Alexandrescu>
|\ \ \
| |/ /
|/| /
| |/
| * 6a7da721a - Add and use more introspection amenities (26 hours ago)<Andrei Alexandrescu>
| * 13528b3a5 - --amend (31 hours ago)<Andrei Alexandrescu>
| * d2ad7f503 - WIP (35 hours ago)<Andrei Alexandrescu>
* | 72c57039a - --amend (31 hours ago)<Andrei Alexandrescu>
* | ee3b8a51c - WIP (31 hours ago)<Andrei Alexandrescu>

I just did git merge master; git reset master; git commit -a --author=Andrei, recreating the commit from scratch while keeping the working tree intact.

dnadlinger · 2017-06-11T00:33:58Z

(EEEEVERYTHING is squashed!)

dnadlinger · 2017-06-11T00:43:22Z

Leaving any implementation details aside, this looks to be a good change. The opaque test blocks have alway irked me, precisely because they make better error messages impossible. (It sounds like you have a concrete compiler patch in the works?)

dnadlinger · 2017-06-11T00:43:45Z

std/range/primitives.d

+enum bool isInputRange(R) =
+    is(typeof((ref R r) => r))
+    && is(ReturnType!((R r) => r.empty) == bool)
+    && is(typeof(lvalueOf!R.front))


Didn't the original code also require the front to be copyable?

is(typeof((ref R r) => r)) requires the range to be copyable.

I was referring to front (i.e. the element type, not the range itself).

dnadlinger · 2017-06-11T00:45:11Z

std/range/primitives.d

-    }));
-}
+enum bool isInputRange(R) =
+    is(typeof((ref R r) => r))


Doesn't this check for copyability, whereas the original checked for non-@disabled init?

hmmmm... you may be on to something. How would init be disabled?

dnadlinger · 2017-06-11T00:50:19Z

Seems this broke DScanner: https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fphobos/detail/PR-5461/3/pipeline/160

andralex · 2017-06-11T00:59:13Z

@klickverbot changed the input range constraint to the original

WalterBright · 2017-06-11T01:13:52Z

This is a huge improvement in style, debuggability, and comprehensibility.

andralex · 2017-06-11T01:17:58Z

How do I repro the dscanner error locally? Thanks.

wilzbach · 2017-06-11T01:38:05Z

How do I repro the dscanner error locally? Thanks.

It's libdparse, which is failing - not Dscanner ;-)

Build Phobos from your current branch
Git clone libdparse (it's now part of dlang-community)
Run the failing test from Jenkins

In this case libdparse has a custom test suite:

cd test
DMD=~/dlang/dmd/generated/linux/release/64/dmd  ../test/run_tests.sh

But usually running the tests with DUB is more convenient (works here as well):

dub test --compiler=$HOME/dlang/dmd/generated/linux/release/64/dmd

andralex · 2017-06-11T01:56:12Z

For an odd reason libdparse doesn't like is(typeof(lvalueOf!R.popFront)) but loves is(typeof((R r) => r.popFront)). I'm unclear on what's going on. I will note I've encountered a few oddities e.g. isBidirectionalRange must decide whether r.popBack is callable, which does a UFCS lookup which in turn us liable to evaluate isBidirectionalRange again.

andralex · 2017-06-11T02:03:10Z

So... vibed seems to have trouble with this. I tried:

dub fetch vibe
dub fetch vibed
dub fetch vibe.d

(search the net...)
dub fetch vibe-d

Fetching vibe-d 0.7.31...
Placing vibe-d 0.7.31 to /Users/andrei/.dub/packages/...
Getting a release version failed: Missing package description for package at /Users/andrei/.dub/packages/vibe-d-0.7.31
Retry with ~master...
Fetching vibe-d ~master...
Placing vibe-d ~master to /Users/andrei/.dub/packages/...
Error executing command fetch: Missing package description for package at /Users/andrei/.dub/packages/vibe-d-master

Tried the same command again, got:

Fetching vibe-d 0.7.31...
Placing vibe-d 0.7.31 to /Users/andrei/.dub/packages/...
Getting a release version failed: vibe-d (0.7.31) needs to be removed from '/Users/andrei/.dub/packages/vibe-d-0.7.31' prior placement.
Retry with ~master...
Fetching vibe-d ~master...
Placing vibe-d ~master to /Users/andrei/.dub/packages/...
Error executing command fetch: vibe-d (~master) needs to be removed from '/Users/andrei/.dub/packages/vibe-d-master' prior placement.

And again after rm -rf /Users/andrei/.dub/packages/vibe-d-master:

Fetching vibe-d 0.7.31...
Placing vibe-d 0.7.31 to /Users/andrei/.dub/packages/...
Getting a release version failed: vibe-d (0.7.31) needs to be removed from '/Users/andrei/.dub/packages/vibe-d-0.7.31' prior placement.
Retry with ~master...
Fetching vibe-d ~master...
Placing vibe-d ~master to /Users/andrei/.dub/packages/...
Error executing command fetch: Missing package description for package at /Users/andrei/.dub/packages/vibe-d-master

CyberShadow · 2017-06-11T02:58:36Z

vibed seems to have trouble with this

I think that's just yet another dub bug. Try nuking ~/.dub I guess.

wilzbach · 2017-06-11T03:33:59Z

I think that's just yet another dub bug. Try nuking ~/.dub I guess.

Yes, dub is kinda buggy :S
Vibe.d shouldn't compile anyways due to the std.experimental.allocator regressions ;-)

andralex · 2017-06-11T03:48:11Z

FWIW deleted ~/.dub, same problem

wilzbach · 2017-06-11T05:14:30Z

FWIW deleted ~/.dub, same problem

Weird - btw if you were looking at the ICE on Jenkins in vibe.d, Walter already has you covered: dlang/dmd#6875

There's still a regression manifesting from #5427 though ...

tgehr · 2017-06-11T15:23:19Z

std/range/primitives.d

-}
+enum bool isInputRange(R) =
+    is(typeof((R r) => R.init))
+    && is(ReturnType!((R r) => r.empty) == bool)


This constraint is stronger than before. (The old constraint checked for cast(bool)r.empty being valid.)

Correct. I'd like to keep it that way.

tgehr · 2017-06-11T15:24:03Z

std/range/primitives.d

+enum bool isInputRange(R) =
+    is(typeof((R r) => R.init))
+    && is(ReturnType!((R r) => r.empty) == bool)
+    && is(typeof((R r) => r.front))


This constraint is weaker than before. (front can now return 'void').

Will fix, thanks.

tgehr · 2017-06-11T15:25:01Z

std/range/primitives.d

-    }));
-}
+enum bool isInputRange(R) =
+    is(typeof((R r) => R.init))


The delegate seems redundant.

The previous constraint verified that this works: R r = R.init;. That passes even for ranges with both this() and this(this) disabled, so you're right about that.

The only types rejected by this would be those that play shenanigans with init, i.e. have it return a different type etc. We don't want to support such styles. I'll remove the line.

@tgehr std.algoritm.mutation, std.algoritm.iteration, and std.algoritm.searching don't pass unittests without this line. I'll leave it here for this PR and will investigate separately.

My suggestion was not to delete the line entirely; I don't see why it should be typeof((R r) => R.init) (with unused parameter r) instead of something like is(typeof(R.init) == R).

@tgehr cool, that works. Updated.

The only types rejected by this would be those that play shenanigans with init, i.e. have it return a different type etc. We don't want to support such styles. I'll remove the line.

I thought that we were going to make it illegal to redefine init (certainly, changes were made to TypeInfo so that we could move towards doing that). Until we do, we do need to be a bit careful, but allowing it to be redefined just seems like a disaster waiting to happen - especially when you consider code like this.

Yes, that property should become reserved at some point.

dnadlinger · 2017-06-11T16:38:04Z

std/range/primitives.d

+        @property bool empty();
+        void front();
+    }
+    //static assert(!isInputRange!VoidFront);


Why commented out?

jmdavis · 2017-07-13T15:48:17Z

If, in the future, we decide that all random access ranges with length should define $ (which I think they should), then adding hasOpDollar could be useful.

Well, if we would ever get some form of https://issues.dlang.org/show_bug.cgi?id=7177 implemented, then requiring that $ work when the range is random access would be reasonable...

andralex · 2017-07-13T16:32:05Z

OK, so I did a bit of git cleanup and now everything is nicely in one commit. Ready to merge?

JackStouffer · 2017-07-13T18:15:00Z

@andralex Looks like the reuse of the git hash is confusing circle and the doc builder.

Can you change the commit message and then force push?

andralex · 2017-07-13T18:20:12Z

@JackStouffer much obliged

JackStouffer · 2017-07-13T20:07:25Z

Uhh ... it didn't work.

Let's try this trick then

dlang-bot · 2017-07-13T20:07:33Z

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

JackStouffer · 2017-07-13T20:11:15Z

Well, that did get circle to start, but it looks like the problem with the git history is worse than I thought.

Any ideas @wilzbach @CyberShadow?

CyberShadow · 2017-07-13T20:12:04Z

Closing an reopening a PR has zero effect on the documentation tester. I'm not sure what went wrong yet (I added more logging yesterday) but to force a retest you can either rebase/amend (to change the tip commit SHA1) or wait until something else is merged, so that it's tested with a new merge target.

wilzbach · 2017-07-14T23:06:49Z

Well, that did get circle to start, but it looks like the problem with the git history is worse than I thought.
Any ideas @wilzbach @CyberShadow?

It seems to be a recent problem that I have seen on a few other PRs lately - not so sure on what's going on ...
@CyberShadow do your logs provide any insight?

CyberShadow · 2017-07-14T23:07:20Z

I just answered that in another PR not 5 minutes ago.

wilzbach · 2017-07-14T23:09:26Z

I just answered that in another PR not 5 minutes ago.

Didn't see that until now. Sorry

andralex · 2017-07-15T22:19:25Z

OK what would be a reasonable course of action here?

- rebased diff of dlang#5461

wilzbach · 2017-07-15T22:43:25Z

OK what would be a reasonable course of action here?

Tried the typical "kick the CI with a rebase" trick, but without success. I will close this PR and maybe we just had terrible luck here due to so many people using the contributor push feature?

dnadlinger force-pushed the cnf branch from d3f7579 to 6d5797c Compare June 11, 2017 00:28

dnadlinger force-pushed the cnf branch from 6d5797c to 9bb01e3 Compare June 11, 2017 00:31

dnadlinger reviewed Jun 11, 2017

View reviewed changes

andralex force-pushed the cnf branch from 9bb01e3 to 5974d10 Compare June 11, 2017 00:58

andralex force-pushed the cnf branch 2 times, most recently from 6fd9846 to 4a7bc13 Compare June 11, 2017 01:54

tgehr reviewed Jun 11, 2017

View reviewed changes

andralex force-pushed the cnf branch from 4abf2ab to bc41de2 Compare June 11, 2017 16:35

dnadlinger reviewed Jun 11, 2017

View reviewed changes

andralex force-pushed the cnf branch from bc41de2 to f22c83f Compare June 11, 2017 16:39

andralex force-pushed the cnf branch from 9eff4de to 88f228b Compare July 13, 2017 15:45

andralex force-pushed the cnf branch 7 times, most recently from 3f2e3c7 to 0c2d36a Compare July 13, 2017 16:30

andralex force-pushed the cnf branch from 0c2d36a to 99d50c1 Compare July 13, 2017 18:20

JackStouffer closed this Jul 13, 2017

JackStouffer reopened this Jul 13, 2017

wilzbach force-pushed the cnf branch from 99d50c1 to ec3ab69 Compare July 15, 2017 22:35

wilzbach pushed a commit to andralex/phobos that referenced this pull request Jul 15, 2017

Convert range constraints to Conjunctive Normal Form

ec3ab69

- rebased diff of dlang#5461

Convert range primitives to CNF like a boss

82c3371

wilzbach force-pushed the cnf branch from ec3ab69 to 82c3371 Compare July 15, 2017 22:40

wilzbach closed this Jul 15, 2017

wilzbach mentioned this pull request Jul 15, 2017

Convert range primitives to CNF like a boss #5613

Merged

Uh oh!

Comments

Conversation

andralex commented Jun 11, 2017

Uh oh!

andralex commented Jun 11, 2017

Uh oh!

wilzbach commented Jun 11, 2017

Uh oh!

andralex commented Jun 11, 2017

Uh oh!

dnadlinger commented Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnadlinger commented Jun 11, 2017

Uh oh!

dnadlinger commented Jun 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger commented Jun 11, 2017

Uh oh!

andralex commented Jun 11, 2017

Uh oh!

WalterBright commented Jun 11, 2017

Uh oh!

andralex commented Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Jun 11, 2017

Uh oh!

andralex commented Jun 11, 2017

Uh oh!

CyberShadow commented Jun 11, 2017

Uh oh!

wilzbach commented Jun 11, 2017

Uh oh!

andralex commented Jun 11, 2017

Uh oh!

wilzbach commented Jun 11, 2017

Uh oh!

tgehr Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgehr Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger commented Jun 11, 2017 •

edited

Loading

dnadlinger Jun 11, 2017 •

edited

Loading

dnadlinger Jun 11, 2017 •

edited

Loading

andralex commented Jun 11, 2017 •

edited

Loading

wilzbach commented Jun 11, 2017 •

edited

Loading

tgehr Jun 11, 2017 •

edited

Loading

tgehr Jun 11, 2017 •

edited

Loading