Chapel: name-based arguments in overrides and writeThis

[mppf]

This issue evolved to be about a design question for writeThis and name-based argument matching for overrides. We might consider not requiring the names match for arguments named _ or beginning with _.

---

When the override keyword is missing, the compiler currently issues a warning, but it should issue an error after 1.18. The warning is an aid to code migration.

See test/deprecated/override-warning.chpl (Resolved)

See test/classes/vass/generic-method-in-subclass-argname-1.future in addition.

[BryantLam]

Is there an implicit superclass for class.writeThis? The must-use-override warning is emitted for this example:

class C {
  proc writeThis(f) {}
}

proc main() {
  var x = new borrowed C();
  writeln(x);
}

However, if you replace class with record (and remove the borrowed), the override warning goes away.

[lydia-duncan]

Classes implicitly inherit from object, so that would make sense (though I can see that message being frustrating/confusing)

[BryantLam]

So, uh.. I guess that means I can't "always use override on read/write/This/These" because of compilation error:
error: R.writeThis override keyword present but record methods cannot override.

Edit: I don't think there's a problem with this. It's just another quirk to be aware of for class vs. record.

[bradcray]

guess that means I can't "always use override on read/write/This/These" because of compilation error:
error: R.writeThis override keyword present but record methods cannot override.

Right, this strikes me as a bit of a classic "should we simply ignore things when they don't make sense to make users' lives simpler / more consistent" vs. "don't ignore them in order to keep the code more precise." Records don't have inheritance / dynamic dispatch, so override technically doesn't make sense on records until they do. As another recent example, it's a bit like whether we should support modifiers like unmanaged or owned on types like ints or records. (In both cases, I tend to prefer the "keep the code precise" approach).

For this specific case, we could potentially remove the writeThis() method on object — I'm not sure what the implications would be... (e.g., maybe I couldn't write out an object, but that doesn't seem all that useful anyway...?) @mppf any thoughts here?

[mppf]

@bradcray - if we removed the writeThis method on object, we wouldn't be able to to dynamically dispatch when writing objects. Maybe that's what you meant by

maybe I couldn't write out an object, but that doesn't seem all that useful anyway...?

E.g. one might create an array of objects to store a variety of class instances, and then try to writeln it. Do you want that to work?

[bradcray]

I suppose I do (and, more generally, want to be able to writeln() any Chapel expression). Given that the user doesn't call writeThis() directly, part of what I was wondering was whether the writeln() (and friends) implementations could special-case object types to avoid relying on writeThis() for them. But maybe that's starting to get a bit contorted...

I haven't gone looking to figure this out for myself, but am wondering what other methods object defines that all user classes would have to use override to define. I.e., is writeThis() an outlier, or is it one of dozens of things?

[mppf]

But maybe that's starting to get a bit contorted...

I can't see how to make that work. The problem is we won't be able to dynamically dispatch anymore.

I think it would be more reasonable to simply adjust override checking to not worry about missing override on writeThis / readThis since these are always override. I'd prefer we keep the error though (in case the argument types don't match, or something).

what other methods object defines that all user classes would have to use override to define

readThis
writeThis
borrow

In the future, if we support operators-as-methods, the list would presumably be longer.

Additionally I think chpl__defaultHash would perhaps be better as a method. (and a user-facing one).

[bradcray]

The status quo is my preference as well, I was /am just trying to brainstorm about whether there's a way to reduce Bryant's frustration. I can imagine some surprise given that users don't explicitly subclass object but just get it for free.

The problem is we won't be able to dynamically dispatch anymore.

I couldn't remember whether writeln() relied on dynamic dispatch or generic instantiation and specialization...

[mppf]

I couldn't remember whether writeln() relied on dynamic dispatch or generic instantiation and specialization...

it can rely on both.

Consider this example:

class MyClass {
  override proc writeThis(f) {
    f <~> "MyClass";
  }
}
class OtherClass {
  override proc writeThis(f) {
    f <~> "OtherClass";
  }
}


proc main() {

  var A:[1..5] object;

  A[1] = new MyClass();
  A[2] = new OtherClass();

  writeln(A);
    // in the array write, object.writeThis
    // will be called
}

Also note that in the first try at writing it, I wrote

class MyClass {
  override proc writeThis(writer) {
    writer <~> "MyClass";
  }
}

and was only saved by the override keyword (otherwise, it just silently doesn't work...).

Even so, I think for a case like this, we shouldn't have to insist on the particular argument name....

[BryantLam]

Even so, I think for a case like this, we shouldn't have to insist on the particular argument name....

Yeah that's weird and I'm surprised that the argument name is part of the signature (for generics only?).

In any case, I'm okay with the override keyword for classes and not for records. However, it does become problematic later if records gain inheritance.

[bradcray]

I'm surprised that the argument name is part of the signature

This is because of the ability to match arguments by name in Chapel, like foo(i=5). If the names didn't have to match in an override, it leads to questions about what would happen for the call above in a case like the following (assuming override was not necessary as in 1.17):

class C {
  proc foo(x: real = 3.14, i: int) { writeln("In C.foo()"); }
}

class D: C {
  proc foo(x: real = 3.14, y: int) { writeln("In D.foo()"); }
}

Presumably the only viable candidate for myD.foo(i=5) would be C.foo(), so did D's author intend to override foo() and fail to get it correct, or did they want to create a new overload successfully?

[BryantLam]

Wow I did not know you could do that. That makes sense as to why a different argument name would get flagged by override.

[bradcray]

Even so, I think for a case like this, we shouldn't have to insist on the particular argument name....

If we were going to special-case something for methods like writeThis() on user base classes, maybe it should be that you don't need the override keyword at all for such methods rather than that the argument could be named whatever you want (since that would presumably give you both conveniences). For a user subclass that specialized a user parent class's writeThis(), I think both would/should be required again.

[mppf]

Even so, I think for a case like this, we shouldn't have to insist on the particular argument name....

I was thinking more along the lines of making some arguments have ignored names. This is in an attempt to address a more general problem that writeThis and override. One straw-man there would be to not allow named argument passing for argument names beginning with _. (In the case of writeThis, it would work to simply not allow named argument passing for arguments named _).

For example, it could be, in object, proc writeThis(_) or proc writeThis(_f).
Likewise if a base class declares a method and wants to disable named arguments to allow other arguments, it could do so. E.g. for a sorting Comparator subclass:

class BaseComparator {
  proc compare(_a, _b) { }
}
class MyComparator : BaseComparator {
  override proc compare(a, b) { } // different name available here and it still overrides
}

In such an event:

• named argument passing would not be allowed for BaseComparator
• it would be allowed for MyComparator
• a subclass of MyComparator would still need named argument passing

Back to your response:

If we were going to special-case something for methods like writeThis() on user base classes, maybe it should be that you don't need the override keyword at all for such methods

That would be fine with me.