search

chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.78k stars 420 forks source link

Owned/Shared module - are we okay with clear being taken out of the user's namespace control? #20336

Closed lydia-duncan closed 1 year ago

lydia-duncan commented 2 years ago

I discovered this while implementing DistributedMap. maps today have a clear method and I was toying with making distributedMap a class so that there could potentially be an inheritance relationship, enabling a function to accept either a map or a distributedMap. But unfortunately, if I make it a class any attempts to call the map/distributedMap clear method will instead resolve to the owned/shared clear method, resulting in this error message if I try to clear it and then do something else with the instance:

test.chpl:13: error: Cannot transfer ownership from a non-nilable outer variable
test.chpl:14: error: Illegal use of dead value
test.chpl:13: note: 'm' is dead due to ownership transfer here
test.chpl:14: error: mention of non-nilable variable after ownership is transferred out of it

While there's probably something else that can be done to enable a function to accept both maps and distributedMaps, this begs the question: is it okay that owned and shared are using a name as common as clear? It surprised me and my first instinct when I got the message was to look if the clear method I wrote was doing something weird with its this argument - I think it will be difficult to debug for a user that happens to write their own method named clear. A quick example - if the user is writing a graphical display type, they might lean towards naming a method that cleans the display clear. And of course there's other collection types.

mppf commented 2 years ago

But unfortunately, if I make it a class any attempts to call the map/distributedMap clear method will instead resolve to the owned/shared clear method, resulting in this error message if I try to clear it and then do something else with the instance:

I think you are saying that your program is working with an owned C or shared C where C is a class type, and you called myOwnedClass.clear() trying to call C.clear() but instead you got owned.clear()?

You do have the workaround of writing myOwnedClass!.clear() to get the effect you wanted.

I think it would be reasonable to think about the owned and shared method names with a view towards this problem. Many of them can be written in a more punctuation-y way; e.g. myOwnedClass.clear() could be written as myOwnedClass = nil. I think there are other issues where we have been asking about whether we should have both ( https://github.com/Cray/chapel-private/issues/1613 / https://github.com/chapel-lang/chapel/issues/17461 ). I think this issue shows a good reason why we might want to only provide the punctuation-y way.

lydia-duncan commented 2 years ago

I think you are saying that your program is working with an owned C or shared C where C is a class type, and you called myOwnedClass.clear() trying to call C.clear() but instead you got owned.clear()?

Yes, that's right. I didn't explicitly declare the management strategy of the instance, though, I just got owned because it was the default management strategy.

I think a punctuation approach would be appropriate but would also be okay with a more unique name in this case, e.g. clearOwned or clearShared. It'll look less good in the documentation but I think would make it more explicit what was being called and make it less likely to conflict with a user's name. However, I don't think that helps with create (which was called out in the other issue) because iirc create is a type method rather than an instance method

bradcray commented 2 years ago

I think this is a good issue and don't mean to undermine it in saying this, but: I don't think distributedMap should be a class (at least, I don't think the user's view of it should be a class). That said, I fully agree that this seems likely to cause problems in other cases even if we go that route in this one and am a fan of dropping or renaming the .clear() method on owned/shared as a result.

mstrout commented 2 years ago

Related to this issue is the one about potentially deprecating .retain() and .clear() https://github.com/Cray/chapel-private/issues/1609.

bradcray commented 2 years ago

Other than .clear(), what are the other "built-in" class ownership methods that fall into this issue?

If we don't have (or can't come up with) satisfactory operator/punctuation-y ways of specifying these, it seems as though these are names we may want to put in the "special method naming" category?

I'm not swayed by Michael's workaround, but am also not understanding it:

You do have the workaround of writing myOwnedClass!.clear() to get the effect you wanted.

I think of ! as just getting rid of nilability, not of saying "drill past the owned/shared aspect of the class to get at the user's class and its methods. Am I not understanding the workaround, or something about the language and !?

mppf commented 2 years ago

.borrow()

AFAIK you already cannot define .borrow() on a class and .borrow() has some language-defined behavior on all class instances. But you are right to bring up that it is a "special method". (What to do about that, if anything, is not obvious to me).

I'm not swayed by Michael's workaround, but am also not understanding it:

You do have the workaround of writing myOwnedClass!.clear() to get the effect you wanted.

I think of ! as just getting rid of nilability, not of saying "drill past the owned/shared aspect of the class to get at the user's class and its methods. Am I not understanding the workaround, or something about the language and !?

The workaround works because ! always returns a borrow or an unmanaged (e.g. myOwnedC! is going to be borrowed C and that works whether myOwnedC is nilable or not). A probably less confusing way to write it (in that it doesn't bring up nilability) is myOwnedClass.borrow().clear().

Also, I'm happy the workaround exists, but I don't think we should use it to justify ignoring the problem. In particular, I would also rather remove clear() since it can be written in a different way.

mppf commented 2 years ago

others?

I wanted to check, so took a look at the docs.

owned has:

shared has:

lydia-duncan commented 2 years ago

I think retain and release would be good to obfuscate a bit more so that they don't interfere. borrow doesn't bother me, though, since it feels closer to a type and I'm having trouble coming up with cases where you'd want to name a method borrow otherwise

mstrout commented 2 years ago

proc type create which we might want to remove but maybe OK for this purpose because it is a type method

To check understanding, a "type method" does not take a receiver, thus would be called as create(obj) instead of obj.create()?

~A corollary of this is that I need to go edit all of the .create notes in recent github issues I have made with create.~ 😄 Thank you @lydia-duncan for clearing this up in https://github.com/chapel-lang/chapel/issues/20336#issuecomment-1212088369.

lydia-duncan commented 2 years ago

Here's a code sample:

var foo = new myType();
myType.create();          // This is a call to a type method
foo.create();             // This is a call to a regular method
bradcray commented 2 years ago

(Right, so type methods still have a receiver, but the receiver is a type rather than an object of the given type).

mppf commented 2 years ago

(Also, note that we have historically had a terminology problem, where proc type R.foo() { } and proc R.foo() type { } were both called "type methods" but the first is a method on the type (like a static method in C++) and the second is a method that returns a type. Just something to watch out for.)

mstrout commented 1 year ago

Subsumed by https://github.com/chapel-lang/chapel/issues/21372, so go there to see conclusion.