param proc in child class vs param field in parent

[mppf]

In investigating https://github.com/chapel-lang/chapel/pull/19306#issuecomment-1062369335 I encountered this pattern (with DimensionalArr and BaseArrOverRectangularDom)

class Parent {
  param rank : int;
}

class Child : Parent {
  proc rank param return 2;
}

var x = new unmanaged Child(1);
writeln(x.rank);
delete x;

It is unclear to me what this program should do. Today, it results in an ambiguity error.

https://chapel-lang.org/docs/language/spec/classes.html#overriding-base-class-methods would seem to hint that the proc rank in Child is overriding and so would need the override keyword. (The spec does say that field getters are added -- https://chapel-lang.org/docs/language/spec/classes.html#variable-getter-methods ).

However, virtual dispatch is not involved for a param method (or for parenless methods).

So, the first question is -- Should the proc rank in Child require the override keyword?

The second question is -- is an ambiguity error here reasonable? Asking the compiler for more information on the disambiguation gives this:

##########################
# Considering function 1 #
##########################

rank this: unmanaged Parent(1) [1321487]

Comparing to function 0
-----------------------
rank this: borrowed Child(1) [1321473]

Looking at argument 0
Actual's type: _MT
Formal 1's type: _MT
Formal 2's type: _MT

Looking at argument 1
Actual's type: unmanaged Child(1)
Formal 1's type: unmanaged Parent(1)
Actual requires coercion to match formal 1
Formal 2's type: borrowed Child(1)
Actual requires coercion to match formal 2

W: Fn 1 and Fn 0 are equally specific
X: Fn 1 is a as good a match as Fn 0

Y: Fn 1 is NOT the best match.

It seems to me that the ambiguity error is coming from a combination of two things:

1. The compiler seems to be generating (instantiating?) a proc Parent.rank that has receiver type unmanaged Parent, for some reason.
2. The disambiguation rules do not have a preference between unmanaged -> borrowed and Child -> Parent implicit conversions. Perhaps in this case, preferring the Child -> Parent one would make sense; or maybe it is better to leave such cases an error. (See also functions/ferguson/hijacking/Application8.chpl if we think about changing this).

[bradcray]

Since the static type of x is Child, I would've expected it to either trivially resolve to Child.x, or for the compiler to complain that we can't override a param field in a parent class with a paren-less proc of the same name in the child class.

Checking this similar, but different, program:

class P {
  var x: int = 45;
}

class C: P {
  proc x return 33;
}

var myC = new unmanaged C();
writeln(myC.x);

it works and resolves to C.x (as does making C.x a field), suggesting that not complaining in the param case and just resolving to the child value would be consistent with what we have here.

I'm wondering whether the reason this is treated as unmanaged is so that the param resolution can be done at compile-time with no need for coercions? That said, if so, I'm not sure why the same rationale wouldn't apply to the proc param case. So it seems like those should either both be generic or both be borrowed and require the coercion.

W.r.t. whether or not override should be required, it seems like the question to ask is whether the motivations for adding override in general apply to statically-dispatched cases like they do for dynamic cases. If they do, it seems like we should require it not only for this case, but also for other cases, like where a child field overrides a parent field. On one hand, I don't think (?) it would bother me as a user if the compiler required me to write override in these cases. But on the other hand, I associate override with dynamic dispatch, and since that doesn't apply here, maybe we shouldn't require it. Specifically, I don't believe a parent class could break my program by adding a new proc param of shared name to something I have that I wasn't aware of (say) because either my static type is the child class in which case I'll still be calling my original thing; or it'll be the parent class in which case I couldn't have called it anyway. So I'd lean toward not requiring override for this case.

[mppf]

buildDefaultFunctions is making the accessor generic w.r.t. class management

https://github.com/chapel-lang/chapel/blob/e472a3245eef39f7b5b709d3e0115ae6a5f29b08/compiler/passes/buildDefaultFunctions.cpp#L472-L475

This originated in 970624c2dead0a41085c6862eb854b9383adc9c0 which the message says was trying to get test/classes/deitz/types/type_in_class3.chpl working.

[mppf]

This patch allows both the O.P. test to compile as well as test/classes/deitz/types/type_in_class3.chpl to pass.

diff --git a/compiler/passes/buildDefaultFunctions.cpp b/compiler/passes/buildDefaultFunctions.cpp
index 7597654292..f1811d7715 100644
--- a/compiler/passes/buildDefaultFunctions.cpp
+++ b/compiler/passes/buildDefaultFunctions.cpp
@@ -470,7 +470,7 @@ FnSymbol* build_accessor(AggregateType* ct, Symbol* field,
   fn->setMethod(true);

   Type* thisType = ct;
-  if (chapelClass && (typeMethod || typeOrParam))
+  if (chapelClass && typeMethod) //(typeMethod || typeOrParam))
     thisType = ct->getDecoratedClass(ClassTypeDecorator::GENERIC);
   ArgSymbol* _this = new ArgSymbol(INTENT_BLANK, "this", thisType);

[bradcray]

I was just about to ask if that would do it... :)

[mppf]

Maybe it is obvious but the change above makes this no longer compile:

class C {
  param field: int;
}

proc main() {
  var cc: C(1)? = new C(1);
  writeln(cc.field);

  cc = nil;
  writeln(cc.field);
}

and that is the cause of the failure for classes/diten/instantiatedClassname in PR #19505.

I think this is probably good (makes the language more consistent; getting these fields from the type itself is still possible).

[mppf]

This originated in 970624c2dead0a41085c6862eb854b9383adc9c0 which the message says was trying to get test/classes/deitz/types/type_in_class3.chpl working.

I think I was missing something important about the history in this area.

Issue #13617 and the related PR #14549 intentionally made it the case that it is not necessary to use a try! when accessing a type/param field of a variable with nilable class type.

[mppf]

@vasslitvinov - what do you think? Should we simplify by removing the convenience feature? Or look for a different way to fix this?

Only classes/diten/instantiatedClassname as well as the tests you added in PR #14549 are failing with the change in #19505 (stop making param/type field accessors generic).

[bradcray]

Maybe it is obvious but the change above makes this no longer compile:

Is the implication that if we were to create a user-defined array type as a record along the lines of:

record bradsArray {
  param rank: int;
  type idxType = int;
  type eltType;
}

we couldn't make a query like

var myArray = new bradsArray(rank=2, eltType=real);
writeln(myArray.rank);

and would have to type myArray.type.rank instead? (at least without having the type author rename the param field to something else and supporting a param proc on the record named rank that returned that value?). That seems a little unfortunate / roundabout to me, but that's with a minimal amount of thought.

[mppf]

@bradcray - no, that should work OK. It's just if you have a nilable class you have to use the MyClassType.type.rank or you have to add a try!.

[vasslitvinov]

I think the same way as Brad in https://github.com/chapel-lang/chapel/issues/19474#issuecomment-1072753987

• The code in the OP should compile and invoke Child.rank.
• I suggest we do not require override since there is no dynamic dispatch.
• For Michael's nilable example, I'd prefer to keep the convenience feature, i.e., enable cc.field without requiring ! as in cc!.field when field is a param or type field or method. I don't have a personal preference though. It's just whatever rationale we had for this feature back then.

I am not clear whether the accessor methods should be generic over memory management and/or nilability. Sounds like this is an question only for param accessors? If so, this aspect should not matter to the user -- they should observe the above semantics in any case.

[mppf]

@vasslitvinov do you have a suggestion for how we can do that? The convenience feature for nil-ability seems to be conflicting with the idea that the code in the OP should compile. I know I can get the OP code working by removing the convenience feature. Can you think of another way you would prefer? Or do you not want to argue that much for the convenience feature?

[vasslitvinov]

I do not see such a conflict. I suggest that preferring a method in the current class over one in a parent class should be of higher priority than genericity and even where-clauses, when determining "more specific" during resolution. For the convenience feature, the rule could be "if the method does not resolve and its non-nilable counterpart resolves to a param/type method, then use the latter".

Then again, I am OK to give up the convenience.

[mppf]

Another problem with the convenience feature is that if we do not offer it for parenless procs, then it's not really the case that you can replace a field with a parenless proc without breaking the code using it. IMO this is a pretty serious problem with it.

[vasslitvinov]

Good clarification. I think the convenience should apply to parenless procs. This way it will apply to fields too because x.myField will resolve to the accessor.

[mppf]

Yes, having the convenience feature apply to parenless procs that return type/param would presumably resolve the OP issue. I think we should choose between that option and removing the convenience feature entirely.

Another concern I have about the convenience feature is that it doesn't make sense with runtime types where something like

class C {
  type t;
  proc getType type { return this.t; }
}
var A: [1..10] int;
var cc = new C?(A.type);
cc.getType; // OK
cc = nil;
cc.getType; // uh-oh, tries to dereference nil for the runtime type

would result in core-dump if proc getType is called on a nil C because it needs information at runtime. AFAIK, #14549 tries to make the above case a compilation error. But even that might be surprising if generic code is trying to rely on the convenience feature, say.

But more broadly, when returning a runtime type, proc getType could do anything at all with this at runtime. (This is different from the compiler-generated field accessor, which is all the convenience feature handles so far). Are we now saying that certain methods can't assume that this is not nil even though this has type non-nilable C?

So, I am wondering if the convenience feature is coming at too great a cost in terms of complexity (namely that it adds new problems with runtime types that we have to deal with somehow).

[vasslitvinov]

I believe the convenience feature currently specifically excludes the case of the method returning a runtime type. I suggest to keep it this way.

[mppf]

Yes, we could do that, and keep it a compilation error, at least.

But even that might be surprising if generic code is trying to rely on the convenience feature, say.

Does the convenience feature make a type-returning methods have different capabilities depending on whether or not it returns a runtime type? Is this something that is likely to lead to confusion?

[mppf]

Also @vasslitvinov - was this convenience feature ever documented anywhere? I am not seeing a link to documentation from the issue or PR.

[vasslitvinov]

I suggest that this should be a matter of coding style -- generic code should not rely on the convenience feature if a runtime type is a possibility. I don't see this as a show-stopper. Indeed, even without it, it is the case that generic code may have different capabilities depending on what it is instantiated with.

I am not seeing any documentation of this feature either. This may well be not part of the "Chapel proper" definition.

[vasslitvinov]

The spec as it stands DOES require override even when there is no dynamic dispatch. Link. It states the following rationale, without restricting it to dynamically-dispatched methods: "This feature is designed to help avoid cases where class authors accidentally override a method without knowing it; or fail to override a method that they intended to due to not meeting the identical signature condition."

I can go either way -- upholding this or restricting override to dynamically-dispatched methods.

[vasslitvinov]

W.r.t. override: CHANGE.md has the following entry for 1.21:

compile-time (type, param) methods now require override when overridden
(see https://chapel-lang.org/docs/1.21/language/spec/classes.html#overriding-base-class-methods)

Apparently we thought about it.

[mppf]

Good point. It was discussed in issue #13154 and implemented in PR #15082.

I think the difference in this issue is that they are parenless.

[mppf]

Summarizing a discussion with @bradcray about this:

• If we use shadowing behavior for a type/param method or field or parenless method, then it is the case that I can construct a "hijacking" story where an application silently changes behavior when some depended-upon libraries are updated. For the shadowing that prefers the child type; we would imagine that the application exists and is using the parent type's method/field/parenless method. Then the child type author adds something with the same name. The application will now silently change behavior b/c it will now call the child method/field/parenless method.
• For field specifically, it's impossible for the parent class and child class to both have a field with the same name that have the same meaning (because they are different variables). So, the proposal is to make the case of a field with the same name between a parent class and child class into an error. At the very least, this design leaves room for it to not be a language change to make it no longer an error.
• For parenless procs, the proposal is to make it so that parenless procs can override fields or other parenless procs. The override keyword would be required in these cases (so that, in the story above, the author of the child class at least knows that there is an issue). The argument is that overriding a parenless proc is sometimes useful e.g. to add additional checking to accesses to a parent field.
• The proposal is also that adding to a child class a field with the same name as a parenless proc in a parent class would result in a compilation error.
• Regarding the convenience feature for nilability, it sounds like Brad would prefer that we keep this feature.

So, here are the next steps:

• think about the above proposal a bit more & see if we see any problems with it & if it makes sense with more thought.
• implement the above proposal & see if testing it provides any surprises
• implement a change to the convenience feature - it should apply to parenless type/param returning procs (rather than only to fields)
• document the convenience feature as well as the above proposal in the language specification

[vasslitvinov]

@mppf It would be nice to see the code for the hijacking scenario. It was not obvious to me from the description. Also what is "shadowing behavior"?

[bradcray]

@vasslitvinov :

Here's the case that worried me:

class P {
  var field: int;
}

class C: P {
}

var myC = new C();
writeln(P.field);

now, later, someone comes along and changes child to the following, not realizing that P had a field named field:

class C: P {
  var field: [1..100000] string;
}

Suddenly, the behavior of my code changes dramatically without any warning to me or to C's author.

[lydia-duncan]

I don't consider child class behavior changes from the parent class to be hijacking. A child class has an intentional connection to a parent class, but the child class should be free to use or discard aspects of the parent class as they see fit. I don't think we should take steps that prevent discarding of behavior from the parent type

[bradcray]

I don't think we should take steps that prevent discarding of behavior from the parent type

Assuming you're talking about the example I just added, and not something from Michael's comment:

The child class could still change the parent class's behavior in this case, they'd just have to do it by adding an override paren-less proc:

class C: P {
  override proc field { ...does something different... }
}

rather than declaring a new field that shadows the parent class field. Part of our reasons for thinking to make the "fields of same name" case an error was (1) we don't know of particularly good motivators for it, (2) it seems like it is more likely to cause confusion than benefit, (3) if we're not relying on it in compelling ways today, we can always relax it if someone has a compelling motivator for it in the future, whereas if we allow it in 2.0, we can't really go back.

[lydia-duncan]

Let's approach this from the other way. Say you have a type relationship like this:

class P {
}

class C: P {
  var field: int;
}

If the author of P later adds a field to its definition using today's rules, instances of C are not affected. If we follow what is being proposed, now instances of C get broken. A parent class change is far more likely to impact a child class than a child class change is to impact the parent and the current rules insulate a child class from that impact

[bradcray]

now instances of C get broken the current rules insulate a child class from that impact

I agree that this is true, but I'm not sure that "a child class should be unaware of what goes on in its parent's public interface" is a good design goal. For example, a similar argument holds true if you have:

class P {
}

class C: P {
  proc foo() { ... }
}

and then P's author adds its own proc foo() later on. In such a case, the child needs to either add an override or to rename their method in order to keep working with its parent class. Requiring children to have distinct type/param/field names than their parents seems like a similar sort of awareness of interface across the class hierarchy.

[mppf]

@mppf It would be nice to see the code for the hijacking scenario. It was not obvious to me from the description. Also what is "shadowing behavior"?

Sure, let me tell you a story.

We have

class P {
  var x: int = 1;
}

developed by Interval Dynamics Corp and we have

class C : P {
}

developed by Hydrologic Resonators LLC.

Then an application developer at SuperApps, Inc writes this:

proc main() {
  var obj = new C();
  writeln(obj.x); // note that this is referring to the field x in P
}

But then, Hydrologic Resonators LLC updates C like this:

class C : P {
  var x: real = -42.0;
}

When they do so, they don't get any errors so miss the fact that there are now two fields named x (one in C and one in P).

Now the application developer at SuperApps updates their program to the latest library versions and recompiles. They don't get any compilation errors but the program suddenly gets the wrong answers!

[vasslitvinov]

@mppf thank you, this clarifies. This scenario is not specific to a particular kind of field. x could have been a dynamically-dispatched function (written with parens, say) and Hydrologic Resonators LLC could have added a perfectly legitimate override for it. To me this is not grounds for disallowing it.

[bradcray]

@vasslitvinov : I think one difference is that in Michael's example, HR LLC doesn't necessarily have any idea that they're breaking or changing user code (e.g., maybe they don't even realize the parent class has the field), whereas with the method, if they add proc x inadvertently, the compiler will complain at them about the need for override, requiring them to either change the name of the method (if they didn't mean to override) or add it (if they did).

In addition, they wouldn't be permitted to override proc x such that it had a different return type (if we're doing our job right), so presumably override proc x is either something the SuperApps user wouldn't need to know about (i.e., it has the same return type and has override, so presumably provides the same capability, just in a different way), or the burden would be on HR LLC to inform their users about the change, if it did matter (in which case they could add warning to the procedure, update documentation, etc.).

[mppf]

@bradcray - yes, but there is one issue with the above

In addition, they wouldn't be permitted to override proc x such that it had a different return type (if we're doing our job right), so presumably override proc x is either something the SuperApps user wouldn't need to know about (i.e., it has the same return type and has override, so presumably provides the same capability, just in a different way), or the burden would be on HR LLC to inform their users about the change, if it did matter (in which case they could add warning to the procedure, update documentation, etc.).

In the type/param returning function case (which issue #13154 is about), is there any point in allowing one to create an override that must return the same type/param? Feels a bit like "Any color you want as long as it's black" but I guess it would allow one to add some checking / error messages.

We don't currently check the return types match for such functions, e.g. the below program compiles and runs

class P {
  proc paramReturningFn() param : int {
    return 1;
  }
}

class C : P {
  override proc paramReturningFn() param : real{
    return 42.0;
  }
}

proc main() {
  var x = new C();
  var c = x:borrowed C;
  var p = x:borrowed P;
  writeln("c access: ", c.paramReturningFn());
  writeln("p access: ", p.paramReturningFn());
}

But, for the purposes of this issue, I am happy if the compiler points out to Hydrologic Resonators LLC that overriding is occurring by insisting on the override keyword.

Nonetheless, in this issue we are talking about starting to require override for parenless methods. For now lets focus on ones that return a runtime value (i.e. not type/param). Since parenless methods are not subject to virtual dispatch, the compiler itself does not need the return types to match. But, we could require it if we wanted to. I'd like to start out by not checking the return type (since that's what we do for the other case of override with no-dynamic-dispatch - namely with the type/param returning methods) but I think we should create another issue if you want to discuss adjusting both to require the return type to match.

[vasslitvinov]

As Michael discovered in #19505, our distribution code used to replace param methods such as rank and parSafe on BaseDom with fields in some of the subclasses. Is support for this scenario on the table as future work? Obviously, this would require override annotations on the overriding fields.