Skip to content
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

show failing clauses in template constraint match attempts #9419

Closed
wants to merge 1 commit into from

Conversation

adamdruppe
Copy link
Contributor

No description provided.

@adamdruppe adamdruppe requested a review from Geod24 as a code owner March 5, 2019 14:50
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @adamdruppe! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9419"

@thewilsonator
Copy link
Contributor

Thanks, I suspect the test suite will need a fair bit of updating (urgh).

@wilzbach
Copy link
Member

wilzbach commented Mar 5, 2019

Thanks, I suspect the test suite will need a fair bit of updating (urgh).

This should take care of updating most of the files:

./run.d AUTO_UPDATE=1

(it doesn't handle complicated cases)

@@ -788,13 +789,13 @@ extern (C++) final class TemplateDeclaration : ScopeDsymbol
ti.inst = ti; // temporary instantiation to enable genIdent()
scx.flags |= SCOPE.constraint;
bool errors;
bool result = evalStaticCondition(scx, constraint, e, errors);
auto result = evalStaticCondition(scx, constraint, e, errors);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this could be const.

@@ -1061,8 +1064,8 @@ extern (C++) final class TemplateDeclaration : ScopeDsymbol
Objects dedtypes = Objects(td2.parameters.dim);

// Attempt a type deduction
MATCH m = td2.matchWithInstance(sc, ti, &dedtypes, fargs, 1);
if (m > MATCH.nomatch)
auto m = td2.matchWithInstance(sc, ti, &dedtypes, fargs, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const?

return MatchWithSupplementalInformation(MATCH.nomatch);
}

struct MatchWithSupplementalInformation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curly braces on their own lines.

src/dmd/func.d Outdated Show resolved Hide resolved
@adamdruppe
Copy link
Contributor Author

So what this does so far is, for a test program:

import std.algorithm;
struct R {}
void main() {
	sort([1,2,3].filter!("a == 1"));
	R r;
	sort(r);
}

We'll get error messages like this:

/home/me/test/ooooo.d(21): Error: template std.algorithm.sorting.sort cannot deduce function from argument types !()(FilterResult!(unaryFun, int[])), candidates are:
../generated/linux/release/64/../../../../../phobos/std/algorithm/sorting.d(1855):        std.algorithm.sorting.sort(alias
ess = "a < b", SwapStrategy ss = SwapStrategy.unstable, Range)(Range r) if ((ss == SwapStrategy.unstable && (hasSwappableElements!Range || hasAssignableElements!Range) || ss != SwapStrategy.unstable && hasAssignableElements!Range) && isRandomAccessRange!Range && hasSlicing!Range && hasLength!Range)
/home/me/test/ooooo.d(21):        constraint clause isRandomAccessRange!(FilterResult!(unaryFun, int[])) was false
/home/me/test/ooooo.d(23): Error: template std.algorithm.sorting.sort cannot deduce function from argument types !()(R), candidates are:
../generated/linux/release/64/../../../../../phobos/std/algorithm/sorting.d(1855):        std.algorithm.sorting.sort(alias
ess = "a < b", SwapStrategy ss = SwapStrategy.unstable, Range)(Range r) if ((ss == SwapStrategy.unstable && (hasSwappableElements!Range || hasAssignableElements!Range) || ss != SwapStrategy.unstable && hasAssignableElements!Range) && isRandomAccessRange!Range && hasSlicing!Range && hasLength!Range)
/home/me/test/ooooo.d(23):        constraint clause hasSwappableElements!(R) was false
/home/me/test/ooooo.d(23):        constraint clause hasAssignableElements!(R) was false
/home/me/test/ooooo.d(23):        constraint clause cast(SwapStrategy)0 != cast(SwapStrategy)0 was false

Notice the "constraint clause .... was false" lines.

The problem remaining is if there are multiple candidates, it will only print errors from the last one it happened to examine. Otherwise though, I'm pretty happy with it. (And I don't even see that remaining problem as being a blocker, personally).

Better yet IMO would be actually formatting the candidates' constraints and highlighting which ones are true/false/short-circuited inline, but meh. Again, like I said on the forum, my goal here is not to get this merged per se, just to prove this kind of thing is possible and encourage further exploration of improvements of existing code (perhaps in addition to the DIP discussion, but I find the dip less than compelling given the possibilities this proves exist)

@UplinkCoder
Copy link
Member

One problem is new ing in this very tight loop.
Other than that this would be a viable approach.

@adamdruppe
Copy link
Contributor Author

adamdruppe commented Mar 6, 2019 via email

@thewilsonator
Copy link
Contributor

it needs to maintain some kind of tree structure.

Under my DIP the top-level would be an array,

One problem is new ing in this very tight loop.

which would solve this problem, also the problem I had was that I was having trouble printing out the source, which you seem to have managed, but you only need to do that if the match has failed.

@adamdruppe
Copy link
Contributor Author

adamdruppe commented Mar 7, 2019 via email

@Geod24
Copy link
Member

Geod24 commented Sep 8, 2019

Superseded by #9715

@Geod24 Geod24 closed this Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants