Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Support for using len(), empty() and string() with an object #13238

Closed
wants to merge 6 commits into from

Conversation

yegappan
Copy link
Member

@yegappan yegappan commented Oct 1, 2023

Add support for Vim9 object methods len(), empty() and string().

@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

Merging #13238 (be3d38c) into master (ac9c6d5) will increase coverage by 0.66%.
The diff coverage is 95.76%.

❗ Current head be3d38c differs from pull request most recent head af8df0e. Consider uploading reports for the commit af8df0e to get more accurate results

@@            Coverage Diff             @@
##           master   #13238      +/-   ##
==========================================
+ Coverage   82.16%   82.83%   +0.66%     
==========================================
  Files         160      150      -10     
  Lines      196356   183049   -13307     
  Branches    43997    41052    -2945     
==========================================
- Hits       161338   151621    -9717     
+ Misses      22131    18470    -3661     
- Partials    12887    12958      +71     
Flag Coverage Δ
huge-clang-Array 82.83% <95.76%> (+0.01%) ⬆️
linux 82.83% <95.76%> (+0.01%) ⬆️
mingw-x64-HUGE ?
mingw-x86-HUGE ?
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/eval.c 91.05% <100.00%> (-1.46%) ⬇️
src/evalfunc.c 88.97% <100.00%> (-1.29%) ⬇️
src/vim9cmds.c 84.47% <100.00%> (-1.63%) ⬇️
src/vim9expr.c 90.53% <100.00%> (-0.96%) ⬇️
src/vim9class.c 90.72% <95.28%> (-0.55%) ⬇️

... and 139 files with indirect coverage changes

@yegappan yegappan changed the title Support for using len() and empty() on an object Draft: Support for using len() and empty() on an object Oct 1, 2023
@Shane-XB-Qian
Copy link
Contributor

Shane-XB-Qian commented Oct 1, 2023 via email

@ychin
Copy link
Contributor

ychin commented Oct 1, 2023

I personally kind of dislike the double-underscore in Python as well as I find them a little unnecessarily ugly. Other languages other than Python all found ways to do this without resorting to a hammer like this, but I can see the practical aspect of this. That said I don't write a lot of vim9script.

@bfrg
Copy link
Contributor

bfrg commented Oct 1, 2023

If you're introducing dunder methods __len() and __empty(), shouldn't there also be a __new() method so that it's consistent? Currently, we have new() (without any underscores). It seems odd to need underscores for len() and empty() but not for new().

Why not just use the methods len() and empty() without the double underscores?

@yegappan
Copy link
Member Author

yegappan commented Oct 1, 2023

If you're introducing dunder methods __len() and __empty(), shouldn't there also be a __new() method so that it's consistent? Currently, we have new() (without any underscores). It seems odd to need underscores for len() and empty() but not for new().

Why not just use the methods len() and empty() without the double underscores?

The difference is that the new() method is directly invoked by the user and the number
of arguments and their types are controlled by the class and defined by the user.

On the other hand, these methods cannot be directly invoked by the user and are
used by Vim internal functions. The goal is to support the operations supported
by the built-in types (like List, Dict, Blob, etc.) for objects. The number of arguments
and the argument types for these methods cannot be changed. So we need a
mechanism to differentiate between regular methods controlled by the user and
these methods.

@errael
Copy link
Contributor

errael commented Oct 1, 2023

@yegappan since it's "draft", might be worth actually making it a draft PR so it isn't inadvertently committed before you're ready.

@errael
Copy link
Contributor

errael commented Oct 1, 2023

On the other hand, these methods cannot be directly invoked by the user and are
used by Vim internal functions. The goal is to support the operations supported
by the built-in types (like List, Dict, Blob, etc.) for objects

Have you explored doing this like it would be done in a more strongly typed
language (I actually mean Java since I'm only a simple language-lawyer).
Then the programmer could naturally detect if the object it's looking at
supports these methods.

Note, this is an approximation, would really be like implements __Container
or whatever name. The __Container would be defined internally by
vim9script and not the vim9script programmer. In this case it would require the
implementing class to define both __len() and __empty().
It's like this so it could actually be compiled and run.

You could have something like

vim9script

interface X__Container
    def X__len(): number
endinterface

class A
endclass

class B extends A implements X__Container
    this.mylen: number
    def new(n: number)
        this.mylen = n
    enddef
    def X__len(): number
        return this.mylen
    enddef
endclass

#def F(o: object): number   See https://github.com/vim/vim/issues/13120
def F(o: any): number
    if o->instanceof(X__Container)
        # return o->len()   # or some such
        return (<X__Container>o).X__len()
    endif
    return 999999
enddef

var o1 = A.new()
var o2 = B.new(3)

echo F(o1)
echo F(o2)

@errael
Copy link
Contributor

errael commented Oct 1, 2023

The spec for len() says Otherwise an error is given and returns zero
[if the specified type can not be used with len()].

There's a test label'd

" Use len() without the dunder method

Which succeeds. Why change the spec?
(or maybe I'm misinterpreting something)

@yegappan
Copy link
Member Author

yegappan commented Oct 1, 2023

The spec for len() says Otherwise an error is given and returns zero [if the specified type can not be used with len()].

There's a test label'd

" Use len() without the dunder method

Which succeeds. Why change the spec? (or maybe I'm misinterpreting something)

Makes sense. Updated the PR to return 0 if the __len() method is missing.

@yegappan
Copy link
Member Author

yegappan commented Oct 1, 2023

On the other hand, these methods cannot be directly invoked by the user and are
used by Vim internal functions. The goal is to support the operations supported
by the built-in types (like List, Dict, Blob, etc.) for objects

Have you explored doing this like it would be done in a more strongly typed language (I actually mean Java since I'm only a simple language-lawyer). Then the programmer could naturally detect if the object it's looking at supports these methods.

Note, this is an approximation, would really be like implements __Container or whatever name. The __Container would be defined internally by vim9script and not the vim9script programmer. In this case it would require the implementing class to define both __len() and __empty(). It's like this so it could actually be compiled and run.

This can be used for other operations in non-container classes. For example, currently
if you use the "string()" function it has a default implementation for converting an object
to a string. We can support the "__str()" method to get a user-defined textual
representation of the object. From your lockvar PR, we can also support "__lock()" and
"__unlock()" methods to lock and unlock an object.

@errael
Copy link
Contributor

errael commented Oct 1, 2023

The spec for len() says `Otherwise an error is given

Which succeeds. Why change the spec? (or maybe I'm misinterpreting something)

Makes sense. Updated the PR to return 0 if the __len() method is missing.

I wasn't clear. It doesn't look like an error is given. There should be an exception.

@errael
Copy link
Contributor

errael commented Oct 1, 2023

On the other hand, these methods cannot be directly invoked by the user and are
used by Vim internal functions. The goal is to support the operations supported
by the built-in types (like List, Dict, Blob, etc.) for objects

Have you explored doing this like it would be done in a more strongly typed language (I actually mean Java since I'm only a simple language-lawyer). Then the programmer could naturally detect if the object it's looking at supports these methods.
Note, this is an approximation, would really be like implements __Container or whatever name. The __Container would be defined internally by vim9script and not the vim9script programmer. In this case it would require the implementing class to define both __len() and __empty(). It's like this so it could actually be compiled and run.

This can be used for other operations in non-container classes. For example, currently if you use the "string()" function it has a default implementation for converting an object to a string. We can support the "__str()" method to get a user-defined textual representation of the object. From your lockvar PR, we can also support "__lock()" and "__unlock()" methods to lock and unlock an object.

Of course, something like

interface __Len
    def __len(): number
endinterface

interface __Empty
    def __empty(): number
endinterface

interface __Container extends __Len, __Empty
endinterface

The point is that it is useful to determine the capabilities of an object. Using an interface to tag an object is a common way to achieve that.

@yegappan
Copy link
Member Author

yegappan commented Oct 1, 2023

The spec for len() says `Otherwise an error is given

Which succeeds. Why change the spec? (or maybe I'm misinterpreting something)

Makes sense. Updated the PR to return 0 if the __len() method is missing.

I wasn't clear. It doesn't look like an error is given. There should be an exception.

If a class doesn't implement the "__len()" method, then the "E701: Invalid type for len()"
error will be given if len(obj) is used.

@errael
Copy link
Contributor

errael commented Oct 1, 2023

The spec for len() says `Otherwise an error is given

Which succeeds. Why change the spec? (or maybe I'm misinterpreting something)

Makes sense. Updated the PR to return 0 if the __len() method is missing.

I wasn't clear. It doesn't look like an error is given. There should be an exception.

If a class doesn't implement the "__len()" method, then the "E701: Invalid type for len()" error will be given if len(obj) is used.

Gotcha. I missed that when looking through the code. Sorry for the noise.

@errael
Copy link
Contributor

errael commented Oct 1, 2023

Or maybe I only saw

" Use empty() without the dunder method

and was looking at the help for len() Looks like empty() doesn't have any error conditions.

I guess empty should default to TRUE, if __empty() is not specified. Not zero.

@ychin
Copy link
Contributor

ychin commented Oct 2, 2023

I think the benefit of something like an interface, is that for things like __lock and __unlock, it will mandate compliance instead of say an object only implementing one function and not the other. So let's say you are implementing something more complicated, even just for an array, you would usually need to implement more than one function (in this case __len and __empty). I think technically you can implement dunder methods first, and then have interfaces for these capabilities retroactively though.

@errael
Copy link
Contributor

errael commented Oct 2, 2023

We can support the "__str()" method to get a user-defined textual representation of the object.

I like that; currently define ToString() methods as needed, using a builtin is easier. There's current default behavior; would that still work, but could be overriden?

we can also support "__lock()" and "__unlock()" methods
to lock and unlock an object.

Yes, the programmer could define what lock/unlock of an object means.

There's also lock/unlock of a member to consider.

Currently there's no final/const modifier allowed on a class/object variable.
In the todo, there's

"final" object members - can only be set in the constructor.

I'm not sure what that's suggesting. Does it mean that if final is implemented,
the variable can only be initialized in a constructor? And what about const? Currently a class
can do

vim9script

class A
    this.somelist: list<number>
    def new()
        this.somelist = LockIt([1, 2, 3])
    enddef
endclass

def LockIt(arg: any): any
    lockvar arg
    return arg
enddef

var o = A.new()
o.somelist[1] = 99

which from outside the class effectively makes somelist look like a const.
The above produces

E741: Value is locked: o.somelist[1] = 99

But the class could still do this.somelist = []

If you want to support locking/unlocking class/object variables, and not just what they reference, I think that would just work (with some minor adjustment) and not require any dunder support, but I haven't done any testing on it.

But if you wanted to allow the class/object to control it, there could be
__lockvar(varname) and __unlockvar(varname) and then a method that can
only be called from within a class that does the actual lock/unlock if the
object approves to lock request.

And there's other ways to look at it.

Got involved in the whole lockvar issue after seeing

how about lock/unlock?

in the todo. What about it? I still don't know what that means.

@yegappan yegappan force-pushed the lsp-async branch 4 times, most recently from 0ec5e66 to 0d05a8f Compare October 3, 2023 01:34
@yegappan yegappan changed the title Draft: Support for using len() and empty() on an object Draft: Support for using len(), empty() and string() with an object Oct 3, 2023
@errael
Copy link
Contributor

errael commented Oct 3, 2023

As mentioned by @ychin

I think technically you can implement dunder methods first, and then have interfaces for these capabilities retroactively though.

I'll take it for a test drive if it finds its way in.

I hope, at some point, that bundling dunder methods into system pre-defined interfaces happens. As noted, it helps programmers to specify objects with some well defined usage, as well as providing a way to detect if an object supports a particular behavior. In addition, common behaviors could be extended; similar to how custom collection types can be defined in Java (the only modern strongly typed language I know), and still be used as basic collection types.

@yegappan yegappan added the vim9class Vim 9 Class Functionality label Oct 3, 2023
chrisbra pushed a commit that referenced this pull request Dec 21, 2023
Problem:  Vim9: need a way to reserve future extension
Solution: reserve double underscore prefix for future use
          (Yegappan Lakshmanan)

related: #13238
closes: #13742

Signed-off-by: Yegappan Lakshmanan <yegappan@yahoo.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>
@yegappan yegappan force-pushed the lsp-async branch 2 times, most recently from 646b062 to 2a5f181 Compare January 12, 2024 16:38
@yegappan yegappan closed this Jan 12, 2024
@yegappan yegappan reopened this Jan 14, 2024
@yegappan
Copy link
Member Author

I have simplified the support for using the builtin methods like empty(), len() and string() with an object.
Now, a class can define an object method with the same as a the builtin function to support this.
Only "empty()", "len()" and "string()" functions are currently supported. These methods can be used
just like any other object method.

@errael
Copy link
Contributor

errael commented Jan 14, 2024

(I'll download and play with it, but some initial comments)

I have simplified the support for using the builtin methods like empty(), len() and string() with an object. Now, a class can define an object method with the same as a the builtin function to support this. Only "empty()", "len()" and "string()" functions are currently supported. These methods can be used just like any other object method.

There's a problem using regular names, there's no way to know that it
interoperates with vim builtins by looking at the class.

And there is also some [backwards?] compatibility issues. Consider, if I
currently have a class that defines a method empty() and this method removes
everything from something in my class then doing empty(obj) is a
catastrophe. Should obj.empty() be allowed, if the class defines empty()
as a builtin?

With this PR it's very easy to run into trouble:

  • There no way to tell by looking at a class that a method is a builtin.
  • Too easy to define methods that mean something special without knowing it.
  • Consider doing obj.empty() versus obj->empty()

I think for many, the __ was not the major problem (I agree it looks better
without the __, but...). Much of the discussion was about a way to specify
that the class is implementing some methods that interpoperate with builtin
functions and specifically which methods they are. IOW, specify a contract
that defines how the class interoperates with the rest of vim. The commonly
mentioned technique was to have some vim9script defined interface that the
class implements. I'm sure there are other ways it could be done.

Note that using __ avoids the problem, it is then clear that the method is
magic. Or requiring implementing a pre-defined system interface, as
discussed in this PR's comments, also avoids the compatibility problem. For a
magic interface, I'm not sure if I like the method declaration better with
or without the __. If the __ is part of the name, then accidentally
invoking the method directly through the object is less of a problem
obj.__empty() and obj->empty() make it clear what's going on.

@yegappan
Copy link
Member Author

(I'll download and play with it, but some initial comments)

I have simplified the support for using the builtin methods like empty(), len() and string() with an object. Now, a class can define an object method with the same as a the builtin function to support this. Only "empty()", "len()" and "string()" functions are currently supported. These methods can be used just like any other object method.

There's a problem using regular names, there's no way to know that it interoperates with vim builtins by looking at the class.

A regular object method name cannot start with a lower case letter. So these builtin
functions will not overlap with regular object methods. Also the names of these methods
is same as the Vim builtin functions. So it will be easier to recognize the mapping between them.

And there is also some [backwards?] compatibility issues. Consider, if I currently have a class that defines a method empty() and this method removes everything from something in my class then doing empty(obj) is a catastrophe. Should obj.empty() be allowed, if the class defines empty() as a builtin?

You cannot define an object method with a lower case letter currently. So the object
method empty() cannot be defined before this PR.

With this PR it's very easy to run into trouble:

  • There no way to tell by looking at a class that a method is a builtin.

If an object method name is all lower case and matches a Vim builtin function, then
it is a builtin. I think, the names "string(), len() and empty()" are easily recognizable.

  • Too easy to define methods that mean something special without knowing it.

A regular object method starting with a lower case letter cannot be defined.

  • Consider doing obj.empty() versus obj->empty()

These two invocations mean the same thing.

I think for many, the __ was not the major problem (I agree it looks better without the __, but...). Much of the discussion was about a way to specify that the class is implementing some methods that interpoperate with builtin functions and specifically which methods they are. IOW, specify a contract that defines how the class interoperates with the rest of vim. The commonly mentioned technique was to have some vim9script defined interface that the class implements. I'm sure there are other ways it could be done.

Won't it be simpler to support these optional builtin methods than adding one
or more special builtin interfaces that classes need to extend?

Note that using __ avoids the problem, it is then clear that the method is magic. Or requiring implementing a pre-defined system interface, as discussed in this PR's comments, also avoids the compatibility problem. For a magic interface, I'm not sure if I like the method declaration better with or without the __. If the __ is part of the name, then accidentally invoking the method directly through the object is less of a problem obj.__empty() and obj->empty() make it clear what's going on.

As explained above, this change will not break backward compatibility.

@errael
Copy link
Contributor

errael commented Jan 14, 2024

A regular object method name cannot start with a lower case letter. So these builtin functions will not overlap with regular object methods.

Of course, I was hallucinating, apologies for the noise.
Away from vim9script for a few days and ...

I think for many, the __ was not the major problem (I agree it looks better without the __, but...). Much of the discussion was about a way to specify that the class is implementing some methods that interpoperate with builtin functions and specifically which methods they are. IOW, specify a contract that defines how the class interoperates with the rest of vim. The commonly mentioned technique was to have some vim9script defined interface that the class implements. I'm sure there are other ways it could be done.

Won't it be simpler to support these optional builtin methods than adding one or more special builtin interfaces that classes need to extend?

I don't see how it is simpler for the vim9script programmer.

Requiring contract specification avoids bugs and is easier to understand,
IMHO. If I want to define a class that acts like a known vim container, then
stating the container type, like using implements dict

  • commonly (universally?) used in strongly typed languages
  • get compile time errors, e.g. missing method, instead of runtime errors
  • it's obvious from the implements clause how a class can be used
  • the class requirements for a type of container/object are clear

One factor is long term goals for interoperability.

Without knowing what's being implemented, what kind of confusion might arise?
While it's intriguing to use an object as both a list and a dict, I wonder if
any of the builtins require knowing what kind of object they are operating on?
Legal remove() argument and types are dependent on container type.

  • Consider doing obj.empty() versus obj->empty()

These two invocations mean the same thing.

Right. I'm not sure that's a good thing, maybe it is.

@yegappan
Copy link
Member Author

A regular object method name cannot start with a lower case letter. So these builtin functions will not overlap with regular object methods.

Of course, I was hallucinating, apologies for the noise. Away from vim9script for a few days and ...

I think for many, the __ was not the major problem (I agree it looks better without the __, but...). Much of the discussion was about a way to specify that the class is implementing some methods that interpoperate with builtin functions and specifically which methods they are. IOW, specify a contract that defines how the class interoperates with the rest of vim. The commonly mentioned technique was to have some vim9script defined interface that the class implements. I'm sure there are other ways it could be done.

Won't it be simpler to support these optional builtin methods than adding one or more special builtin interfaces that classes need to extend?

I don't see how it is simpler for the vim9script programmer.

Requiring contract specification avoids bugs and is easier to understand, IMHO. If I want to define a class that acts like a known vim container, then stating the container type, like using implements dict

  • commonly (universally?) used in strongly typed languages
  • get compile time errors, e.g. missing method, instead of runtime errors
  • it's obvious from the implements clause how a class can be used
  • the class requirements for a type of container/object are clear

One factor is long term goals for interoperability.

Without knowing what's being implemented, what kind of confusion might arise? While it's intriguing to use an object as both a list and a dict, I wonder if any of the builtins require knowing what kind of object they are operating on? Legal remove() argument and types are dependent on container type.

We could add builtin interfaces with names starting with a lower case letter (as a
user defined interface name cannot start with a lower case letter). The len() and
the empty() functions can be included in the vimContainer interface. I am not sure
about a good name for an interface to place the string() function though (vimString?
or vimObject?).

@errael
Copy link
Contributor

errael commented Jan 15, 2024

We could add builtin interfaces with names starting with a lower case letter (as a user defined interface name cannot start with a lower case letter). The len() and the empty() functions can be included in the vimContainer interface. I am not sure about a good name for an interface to place the string() function though (vimString? or vimObject?).

Here's some comments/possibilities (not an attempt at a spec). After writing
the following, I see more issues and details to consider than I'd imagined.

Preceding a builtin interface name with vim is redundant. Since it starts
with lowercase it must be builtin; but if you think a prefix is a good idea,
another possibility is __.

About string(), could say there's an implicit system base class, object,
that defines string and equals and can override them. So you could do

class C
    var v: number
    def string(): string
        return "Foo:" .. super.string()
    enddef
    def equals(other: any): bool
        return v == other.v
    enddef
endclass

But defining them is optional.

Or you could have. (But string() and equals() seem special)

class C implements string, equals

IOW, for many cases of an interface with a single method, could just use
the name of the method as the interface name.

For max flexibility, could allow stuff like

class C implements len, empty
    def len(): number
        return 4
    enddef
    def empty(): bool
        return false
    enddef
endclass

Hmm, I wonder if empty() should automatically be defined as len() != 0; at
least when len() is defined, especially for a container. Allowing empty()
to be independent of len() reminds me of == not being transitive.

For a list container, there's an implicit system definition like:

interface list extends len, empty
    def add(arg_list: list<T>, item: T): any    # Needs "any", return 1 is legal.
    # ... other list functions ...
endinterface

Run-time must check that the return value is the same as arg_list,
other-wise is that an exception? Ignored?

Or, could have, maybe better I think

def add(arg_list: list<T>, item: T): void

For this list add(), the compiler/run-time automatically returns the
argument arg_list or 1 if arg_list is not a list. I wonder where an
error comes from if item is not a subclass of T. Note that in this case
obj.add(a_list, item) is not the same as obj->add(a_list, item)

This points out something interesting. The methods may have certain
requirements that must be checked by the run-time; and some method signatures
do not match the signature seen in builtin.txt help file.

The above definition uses "T", like a generic. But this is a hidden/implicit
thing. The compiler should enforce that the :def signature of list->add()
the argument item is a subclass of the list element type.

Hmm, but I guess arg_list is actually the class, so what the compiler
needs to check is that all the list methods have a consistent item type.

I can see how the following would be nice

class C implements list<T>

then it's clear what the "constistent item type" is.

@errael
Copy link
Contributor

errael commented Jan 16, 2024

In my previous comment I screwed up with the methods by including the implicit "self" in the function signature, like for list's add() should be

def add(item: T): void

But the question, about how the compiler checks for consistent T among the list methods, remains.

runtime/doc/vim9class.txt Outdated Show resolved Hide resolved
@yegappan
Copy link
Member Author

yegappan commented Mar 9, 2024

Support for this committed (d3eae7b - patch 9.1.0148) using PR #14129.

@yegappan yegappan closed this Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vim9class Vim 9 Class Functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants