Don't allow coercions for type variable arguments

[mppf]

Before this PR, the following program would compile and run. After, it will not.

proc foo(type t:int) {
  writeln("int version");
}
foo(int(8));

This PR disables implicit coercions for type arguments with a type specifier (such as type t:int in the example) - unless those coercions are implementing class subtyping. The idea here is to make type t:SomeType behave similarly to where t:SomeType for a generic type t argument. See issue #7779 for discussion.

Fixes #7779

• passed full local testing

Reviewed by @bradcray - thanks!

[mppf]

@bradcray - would you mind reviewing this change disallowing coercion for type variable arguments? Thanks.

[bradcray]

The compiler code change looks OK to me.

The PR message should really include more detail. When looking at a git log from the
command line, we shouldn't need to fire up / rely on GitHub in order to understand what
a PR does, what behavior was changed, what the new tests show, etc.

The new tests seem vaguely related to the inheritedInitWrongType future that I'm
hoping to add in PR #8141 and makes me wonder whether this PR will improve
the behavior of that test (I haven't tried).

The change in behavior for the existing test gives me pause in that I tend to think of
the ability to use a subclass where a superclass is expected as being more a case of
subtyping than of coercion. Do you not?

[mppf]

Re PR message, good point, I've updated the PR message.

Re inheritedInitWrongType / inheritedInitWrongName future, this branch doesn't seem to help that particular case, even though I have noticed that _new seems to use type t:SomeClasstype arguments. There must be a confounding factor for that particular test case.

Re this comment:

The change in behavior for the existing test gives me pause in that I tend to think of
the ability to use a subclass where a superclass is expected as being more a case of
subtyping than of coercion. Do you not?

I agree with you that it's more subtyping than coercion. However, these two ideas are very closely related - enough so that the compiler implements this kind of subtyping as coercion. From a philisophical standpoint I think that's correct. However we could certainly contemplate making the behavior for sub-classes be different from integer types that coerce in this case.

Even so, in my last remark in the issue - which I think you were agreeing with - argues that we want concrete type t arguments to behave more similarly to generic instantiation than differently. For a generic function (say proc f(x)) called with a parent class and a child class, the generic argument would be instantiated twice instead of re-using the version for the parent. If the concrete type t is to work in the same manner, wouldn't we be assuming that the Chapel programmer wanted to manually create the type t:Something versions analogous to the instantiations?

A reasonable counterpoint to that argument is that we'd like proc f(type t:SomeType) to generally behave similarly to proc f(type t) where t:SomeType. That would mean that in the following expanded version of the test in question, the 'foo' section should output the same as the 'bar' section:

record R {
  var x;
}

class Parent {
  var a:int;
}

class Child : Parent {
  var b:int;
}

class MyClass {
  var c:int;
}

proc foo(type t:int) {
  writeln("int");
}
proc foo(type t:integral) {
  writeln("integral");
}
proc foo(type t) {
  writeln("any type");
}
proc foo(type t:R) {
  writeln("R");
}
proc foo(type t:Parent) {
  writeln("Parent");
}
proc foo(type t:object) {
  writeln("object");
}
writeln("foo");
foo(int);
foo(int(8));
foo(real);
foo(R(complex));
foo(Parent);
foo(Child);
foo(MyClass);

proc bar(type t) where t:int {
  writeln("int");
}
proc bar(type t) where t:integral {
  writeln("integral");
}
proc bar(type t) where !(t:int || t:integral || t:R || t:Parent || t:object) {
  writeln("any type");
}
proc bar(type t) where t:R {
  writeln("R");
}
proc bar(type t) where t:Parent {
  writeln("Parent");
}
proc bar(type t) where (t:object && !t:Parent) {
  writeln("object");
}

writeln("bar");
bar(int);
bar(int(8));
bar(real);
bar(R(complex));
bar(Parent);
bar(Child);
bar(MyClass);

Anyway, it would be trivially easy to filter out class types in the new code disabling coercions if we decide to do so.

At present, I'm leaning towards this "It should be similar to a where clause" interpretation, so I'll adjust the PR accordingly & run testing.

[mppf]

Note that the new & improved test case works as expected except for an issue with :integral in where clauses which is described in issue #8172 and will be fixed in PR #8173.

[mppf]

Note that the tests with initializers that were not working now don't work again after the change for class types. Now they are futures that refer to issue #8176.

[mppf]

@bradcray - I think this is ready to merge, please let me know if your review is complete, either way. Thanks!

[bradcray]

I haven't been able to find the time to get back to it since your new comments and commits were added. I'll hope to do so tomorrow, but if you'd prefer to get someone with more cycles to review it, that'd be fine as well.

[mppf]

When you have the time for it. Thanks.

[bradcray]

This looks good to me. Over on this branch I created a few more tests to make sure things work as I'd expect on both master and your branch. They seem to, but you should double-check me (these tests try to validate that R and R(?) work as I'd expect for a generic record R with and without default values for its generic field.

[bradcray]

I didn't quite follow what happened in that last batch of commits. Some tests that weren't futures when I reviewed the PR now seem to be? What does that mean?

[mppf]

I meant to update them to be futures earlier - as in this comment - #8083 (comment) - since this branch moved from disallowing the type arguments with class subtyping to allowing it. I created issue they refer to at the same time... I had just never actually committed the .future files. The testing run reminded me of that.

[bradcray]

Oh, I failed to understand that. I was reviewing based on the PR message and the files themselves and didn't follow all the intermediate comments. Were those tests not among the main motivating cases for this PR in the first place? Does it suggest that my intuition in this comment was wrong?

[mppf]

Yes, it was a motivating case.

Was your intuition wrong? No, just that this change doesn't fix the problem with _new described in #8176 - those need a where clause or something.

[bradcray]

FWIW, having thought about this more since last week: I think that if a PR is reviewed and then the author has a big "Oops, forgot to do these things", they should alert the reviewer to allow the opportunity for a re-review rather than doing a "commit new things + merge" in one fell swoop. Would it have changed anything in this case? Probably not, but it felt confusing to have reviewed one thing and then had something different show up on master. (with more careful reading, could I have understood the consequences and even caught the mistake myself or even tested the new tests to see that they didn't work? Yes, but my schedule doesn't usually afford such care when I'm the reviewer).

[mppf]

@bradcray - OK, I'll endeavor to let you know next time something along those lines changes post-merge.

[bradcray]

Sorry, that's not my point. My point is that it should really be done pre-merge. That is, if before hitting the merge button, if you realize "Oh wait, I need to make the following additional changes before merging to keep everything working" then the reviewer should be brought back into the loop because they arguably haven't reviewed what you're actually merging. In this case, in reviewing the code I thought "Oh good, we're making these cases work now" only to find in scanning the merge "Oh wait, we're not..." which made me feel like I didn't really know what I'd reviewed. And the lack of delay between "commit new changes" and "merge" didn't give me an opportunity to do so.

[mppf]

@bradcray - we understand each other, I just typo'd my response. I meant to say "pre-merge".
(Edit: what I really meant was "post review").