[enh] Issue 3673 - Allow putting constraint after inheritance list

[ghost]

Will the syntax files in dlang.org have to be fixed? Let me know..

I don't even know why we've used the awkward "constraint followed by base class list" syntax before, it looks rather ugly:

class Foo(T)
    if (is(T == bool)) : Base 
{ 
}

Now allowed:

class Bar(T) : Base
    if (is(T == bool))
{ 
}

http://d.puremagic.com/issues/show_bug.cgi?id=3673

[alexrp]

Thank you so much for doing this. This tiny little syntax quirk has annoyed me so much because I always type the latter out first only to realize that it won't work and I have to use the first version...

Implementation LGTM.

[ghost]

N.P.

Thanks for reviewing a lot of stuff lately so quickly!

[yebblies]

This will allow template constraints on non-templates, and multiple template constraints in some places.
Even worse, if a base class list exists, the constraint before it is discarded

class A {};
class B(T) : A if (true) {}
class C(T) if (false) : A {}
class D : A if (false) {} // Makes no sense
class E(T) if (false) : A if (true) {} // Makes even less sense

void main()
{
    auto a = new A(); // fine
    auto b = new B!int(); // fine
    auto c = new C!int(); // should fail
    auto d = new D();
    auto e = new E!int();
}

[ghost]

That's what I get for making a pull at 6 AM. :)

[yebblies]

If you looked at the times on my pull requests you'd probably see something similar.
Could you make it "cannot have more than one template constraint"?

[ghost]

Yes, I was wondering about what message to put there.

[ghost]

Now what about the D language spec? http://dlang.org/template.html

Does another Constraint opt have to be added somewhere?

[yebblies]

Class templates will have two forms:

ClassTemplateDeclaration:
    class Identifier ( TemplateParameterList ) Constraintopt BaseClassListopt ClassBody
    class Identifier ( TemplateParameterList ) BaseClassListopt Constraintopt ClassBody

[9rnsr]

This is a syntactic change. So the reviewing by @WalterBright or @andralex is necessary.

[ghost]

Ok, made another pull for spec: dlang/dlang.org#180

[ghost]

@9rnsr: Andrei is the one who filed the bug and marked it as P2.

[yebblies]

@AndrejMitrovic That was two and a half years ago, though. This is ready to pull, with Andrei and/or Walter's approval.

[9rnsr]

@AndrejMitrovic Oh, I didn't see the original reported of bug 3673.
But it's reported in 2010/01/04. So I think it's necessary re-reviewing by them.

[ghost]

@AndrejMitrovic Oh, I didn't see the original reported of bug 3673.

@9rnsr: It's because I didn't add a link in the description, I'll do it from now on (http://prowiki.org/wiki4d/wiki.cgi?PullRequest is now updated to reflect this).

[ghost]

@andralex @WalterBright : any opinions about this?

[andralex]

I agree with the suggested relaxation. Thanks!

[andralex]

ping @WalterBright

[alexrp]

Seems fine to me (same for doc change).

[ghost]

This is a bad pull, I'll refix.

[ghost]

Much better now, before it accidentally replaced one constraint with another, it should never do that.

[9rnsr]

I think this is too verbose and implementation dependent message.
In other words, this message is too specialized and does not work in general.
(For example, class C(T) if (constraint1) if (constraint2) {} does not report "more than one template constraint" error.)

In here "members expected" is simple, enough, and works in general.

[ghost]

That just sounds like a better reason to fix the error message for that test-case.

[9rnsr]

Yes. Current just one test case looks good, but can this change report consistent error messages with following test cases?
(I don't test it with this)

class B {};

class D1(T) : B if (false) {}  // your test case
class D2(T) if (false) : B {}
class D3(T) if (false) : B if (false) {}
class D4(T) : B if (false) if (false) {}
class D5(T) if (false) if (false) : B {}
class D6(T) if (false) if (false) : B if (false) if (false) {}

Good error report for incorrect code is not so easy. If you can implement it in generic, it will be valuable.

[ghost]

I think it's worth taking a go at this, so I'll work on it. It may not benefit regular code, but if you use mixins and such code it could help diagnose problems.

[ghost]

@9rnsr: How do you like the implementation? I've added your test-cases.

[9rnsr]

In here, I dislike reporting "more than one template constraint" message. The reported thing is definitely trivial, and your approach still doesn't work for much more invalid code.

class D3(T) if (false) : B if { }
class D3(T) if (false) : B if() { }
class D3(T) if (false) : B if(mixin) { }

In parsing phase, reporting useful message against invalid code input is very, VERY difficult. And also, your change would improve just only few cases. I think your change does not pass cost/benefit test.

[9rnsr]

P.S.: I know your good work for error message improvements. But, this is different from them. Your works so far was for errors in semantic phase. The program structure had been already built as AST, so error message had never become off the mark. But here, it is parsing error. The input of your error report routine is infinite. To define correct error output for the infinite input is almost impossible in heuristic way.

[ghost]

To define correct error output for the infinite input is almost impossible in heuristic way.

We could collect all the constraints while parsing, and then move the error into semantic if there's more than one constraint. Would that do the trick?

Edit: Nevermind, I see what you mean.

[ghost]

I've reverted to the simple implementation, if all is green it should be ready to pull.

[yebblies]

I agree that improving the error message when the user screws up the syntax is beyond the scope of this pull, but the message when both constraints are present should tell you that.
Could you please also make sure there is a test case in there for trying to constrain a non-template.

[ghost]

@yebblies: I've added such a test-case. It's green now.

[9rnsr]

LGTM. I'll merge this!

[yebblies]

I just noticed it still prints 'members expected'. This is an unhelpful error message and we can do better.

[9rnsr]

It is intended. First @AndrejMitrovic tried to report "helpful" message, but I was against it. I've argued that such particular improvement for "helpful" message would report pointless message in many other cases.
See discussion log first. If you would still have question after that, I'll reply to that.

[yebblies]

I have read the discussion log, I just don't agree.

The second template constraint will always be parsed, so this error message only appears when there are two template constraints! It should say that!

Maybe you meant for the second template constraint to never be parsed if the first one is found? Because that's not what's implemented.

It will happily parse one template constraint, then another, then say 'members expected'.

[9rnsr]

The second template constraint will always be parsed

Ah, that's my oversight. The second parsing should be invoked only when if (tpl && !constraint).

Heuristic way is not always perfect. Moreover, in this case, the obtained benefit would be always less than its code line cost. So I hate parser code bloating for "better error message".

[yebblies]

Ok I agree with that.

"members expected" is a poor error message any way, something like "'{' expected after base class list, not '%s'" would be more helpful.

If we have already parsed a template constraint, and the next token is 'if', I think the message "cannot have two template constraints" would be appropriate. Yes it is a heuristic, but it is a very good one. I don't think that code is very likely to occur in the wild though, so it might not be worth the effort.