Prevent assigning class name to variable.

[errael]

@lifepillar @yegappan @dkearns please review.
Code reviewers, please consider where the errors are generated. Is there a better place to check? Are all the cases caught?

Error is E1393: Cannot use a class as a variable or value

notes on changes

Prevent assignment

• from non-value items, e.g. class or typealias.
• of non-value items as function argument or return.
• to dict/list: var l = [1, C], var d = {x: C}.

NOTE: currently non-value items can be added to a list (while #13421 is being sorted out)
NOTE: a builtin must not return a non-value:, bare, in list, in dict

Here's an overview of the code changes made (so far).

simple assignments
    scriptlevel assignment  ex_let_one in evalvars.c
    :def assignment         compile_assignment in vim9compile.c

Function related checks
    scriptlevel argument    get_func_arguments in userfunc.c
    argument, call in def   check_ufunc_arg_types in vim9execute.c
    return value            ISN_RETURN (but gives wrong line number)
    defer/funcref           check_func_args_from_type in vim9instr.c

dict/list checks
    script list         eval_list in list.c
    script dict         eval_dict in dict.c
    compile list/dict   get_member_type_from_stack in vim9instr.c

There is a separate commit for instanceof() experimentation.
There is a separate commit for uniform builtin argument checking.

TODO

• More tests.
  Fixed tests as needed, but have around a dozen or so tests to write based on debug scripts. Plan to add tests after initial review (particularly of the error message).

What now?

This PR is very conservative. Some of the checks could be backed out.
The advantage of aggressive checks is uniformity, which should be less
confusing.

• No extra builtin checks, builtins have their own checks.
• Allow non-values in list/dict.
  This would ease some problems with builtins like instanceof()
  This still wouldn't allow the use of the non-value.
  Need to prevent non-value from being the result of an expression;
  protects against usage like aList[0].new().

---

Notes on why the change

With this PR, Test_call_constructor_from_legacy() failed, the test has been
replaced with Test_construct_object_from_legacy() which shows an alternate
way to do the same thing.

There's the following TODO item:

When "Meta" is a class, is "const MetaAlias = Meta" allowed?  It should
either work or given an error. Possibly give an error now and implement it
later (using a typedef).  #12006

This PR "give[s] an error now".
The important question: does preventing this take away an important capability?

When I first saw this TODO, I wondered why not just let it be, and take care
of it later. I played with it, #12961, and noted that it was very limited with
common stuff not working. I think I didn't read the original issue, #12006,
carefully; just reading it now, Bram's argument is

• it should be handled by typedef whenever something like that becomes available
• there is no class type.

If it's left in, it leaves a lot of technical debt to get it fully working.

Some uses for this, can be achieved with something like

class C
    static def CreateC(): any ... enddef
endclass
var CreateC: func = C.Create

[errael]

Add additional test, it does: call(A.new, args)

[errael]

Transitioning this to Ready for review. Not Ready for commit, although it is and I'd like to see it committed at some point.

[dkearns]

This is just a quick general comment for the moment, not addressing the specifics of the PR.

It seems to me, from the existing implementation, that classes were intended to to be first-class objects. This is in keeping with the language in general.

there is no class type.

:help type() lists v:t_class and it is implemented.

[errael]

a quick general comment

Appreciated. Main question is whether or not var c = SomeClass, which I think is accidentally implemented, should be allowed in v9.1; assuming vim9.1 is relatively soon and that there's no work to make class a full blown variable type or that there's not going to be an alternate way (like typedef) to do this. Is there a downside to disallowing the assignment for now, until a decision is made?

there is no class type.

:help type() lists v:t_class and it is implemented.

Interesting point, that statement is based on Bram's comment "It's not an object and we don't have a type that is a class" in #12006 (comment); Bram closed that issue with "Once something like a typedef has been implemented please check this again. Closing for now."

Maybe the distinction is that it's not a type because you can't do

var someClass: class

although internally there is a vim object of type VAR_CLASS. And you can do

vim9script
class C
endclass
echo type(C)
echo typename(C)

12
class<C>

But there are several breadcrumbs about a class type.

[lifepillar]

I am actually assigning classes to a const variables in my code. This is my use case:

import `mylibrary.vim`
const SomeClass = mylibrary.SomeClass

As you mention, that might be:

typedef SomeClass mylibrary.SomeClass

but that is not implemented yet, afaik. If the class assignment is removed, one is forced to use imported classes with a prefix, I think. Or is there another way?

I personally find it very convenient to be able to assign non-prefixed aliases to imported names; it makes my code much easier to read and write.

[lifepillar]

@errael Can you elaborate on your proposed workaround? I have tried this:

vim9script
# lib.vim

export class Context
  this.text: string
  public this.index: number = 0

  static def MakeContext(text: string, index = 0): any
    return Context.new(text, index)
  enddef

  def new(this.text, this.index = v:none)
  enddef
endclass

vim9script

import './lib.vim'
const MakeContext = lib.Context.MakeContext

def Test_MakeContext()
  var ctx = MakeContext("X", 42)
  assert_equal(v:t_object, type(ctx))
  assert_equal("X", ctx.text)  # <== E715
  assert_equal(42, ctx.index)  # <== E715
enddef

Test_MakeContext()

Sourcing the latter script results in E715: Dictionary required any time a member is accessed.

[lifepillar]

Ok, I can make it work like this:

vim9script
# lib.vim

export class Context
  this.text: string
  public this.index: number = 0

  def new(this.text, this.index = v:none)
  enddef
endclass

export def NewContext(text: string, index = 0): Context
  return Context.new(text, index)
enddef

vim9script

import './lib.vim'
const NewContext = lib.NewContext

def Test_MakeContext()
  var ctx = NewContext("X", 42)
  assert_equal(v:t_object, type(ctx))
  assert_equal("X", ctx.text)
  assert_equal(42, ctx.index)
enddef

Test_MakeContext()

It is still the case, though, that if I want to define a function that takes a Context object as an argument, I need to qualify it:

def F(context: lib.Context)
...

A bit cumbersome, but maybe not too bad, after all. Please consider the ergonomics of removing the assignment without providing a replacement. It feels a bit unnatural to me that I can assign anything but classes.

[errael]

import `mylibrary.vim`
const SomeClass = mylibrary.SomeClass

I personally find it very convenient to be able to assign non-prefixed aliases to imported names; it makes my code much easier to read and write.

Oh crap, how could I forget this important case, which I use all the time with functions. Without :type this assignment is needed.

[errael]

vim9script
# lib.vim

export class Context
  static def MakeContext(text: string, index = 0): any
endclass

Right, MakeContext() can't be properly declared because of #12369.

vim9script
def Test_MakeContext()
  var ctx = MakeContext("X", 42)
  assert_equal("X", ctx.text)  # <== E715
enddef

Sourcing the latter script results in E715: Dictionary required any time a member is accessed.

If object was a type, #13404, then could do static def MakeContext(): object, that should provided sufficient information to the compiler to avoid the all to familiar E715. But that's a workaround in this case, (and it's not implemented anyway). In other cases it can be useful.

It is still the case, though, that if I want to define a function that takes a Context object as an argument, I need to qualify it:

def F(context: lib.Context)
...

A bit cumbersome, but maybe not too bad, after all.

You're right, I don't think that's acceptable. But maybe :type.

Please consider the ergonomics of removing the assignment without providing a replacement. It feels a bit unnatural to me that I can assign anything but classes.

class C defines a type, you can do var o: C.
If var x = C works, would you expect var x = number to work?

But it's beginning to look like :type might be an option, #13407.
Would type Context lib.Context be OK?

[lifepillar]

I see. Clearly, something like var x = C where C is a class (that is, a type) makes sense if classes are themselves values (as in languages like Ruby, where each class is an instance of Class, iirc). But the Ruby way doesn't fit Vim 9 script.

Would type Context lib.Context be OK?

It's the proper way to do it, imo. If you are going to implement :type then I have no objections against merging this PR. :type is the only feature I miss in Vim 9 script. I'm looking forward to replacing things like func(func(list<dict<any>>)): dict<any> with a readable name :)

[errael]

If you are going to implement :type then I have no objections against merging this PR.

Cool. I'll put it back to "Draft PR", and reopen when :type is available. That should allow the smoothest transition after breaking your code.

[dkearns]

Main question is whether or not var c = SomeClass, which I think is accidentally implemented, should be allowed in v9.1; assuming vim9.1 is relatively soon and that there's no work to make class a full blown variable type or that there's not going to be an alternate way (like typedef) to do this. Is there a downside to disallowing the assignment for now, until a decision is made?

Probably not now that :type is available to take care of the aliasing cases.

I think you've missed assignments to parameters.

vim9script

class A
endclass

def Foo(c: any)
  echomsg typename(c)
enddef

Foo(A)

there is no class type.

:help type() lists v:t_class and it is implemented.

Interesting point, that statement is based on Bram's comment "It's not an object and we don't have a type that is a class" in #12006 (comment); Bram closed that issue with "Once something like a typedef has been implemented please check this again. Closing for now."

I can't understand the comment there, given that the help entry was added only a month earlier.

But there are several breadcrumbs about a class type.

Indeed. I wouldn't like to see it sent out into the world in its current state even with assignment disabled without some sort of reasonable narrative.

It looks a bit like class/singleton-metaclass pairs. I suppose something like that could fall out of the implementation but class<Foo> looks quite specific.

[errael]

I think you've missed assignments to parameters.

vim9script

class A
endclass

def Foo(c: any)
  echomsg typename(c)
enddef

Foo(A)

Ah, of course. Thanks. Need a function like check_is_value(typval_T*)

there is no class type.

:help type() lists v:t_class and it is implemented.

Interesting point, that statement is based on Bram's comment "It's not an object and we don't have a type that is a class" in #12006 (comment); Bram closed that issue with "Once something like a typedef has been implemented please check this again. Closing for now."

I can't understand the comment there, given that the help entry was added only a month earlier.

But there are several breadcrumbs about a class type.

Indeed. I wouldn't like to see it sent out into the world in its current state even with assignment disabled without some sort of reasonable narrative.

I found @lifepillar's distinction between type and value clarifying. #13404 (comment)

It looks a bit like class/singleton-metaclass pairs. I suppose something like that could fall out of the implementation but class<Foo> looks quite specific.

class<Foo> might be an artifact of how typename() displays non-value items. And now there's

vim9script
type LN list<number>
echo typename(LN)

which outputs typealias<list<number>>

[errael]

Implementation note: From what I've seen there's a dictionary of accessible names.

dictitem_T *v = find_var(name, ...)
typval_T *tv = &v->di_tv;

Maybe pre vim9class, tv was always a value. Now it can also be a type.

[yegappan]

You can use list assignment to assign multiple variables (B is a class in the statement below):

    [a, b] = [10, B]

To handle this case, this check should be moved to the ex_let_one() function.

[errael]

To handle this case, this check should be moved to the ex_let_one() function

Thanks, it was still on my list.

[errael]

Pushed a tweak to allow non-value items in list while instanceof() issues are sorted out.

[errael]

Can you elaborate on your proposed workaround? I have tried this:
[snip]

vim9script

import './lib.vim'
const MakeContext = lib.Context.MakeContext

def Test_MakeContext()
  var ctx = MakeContext("X", 42)
  assert_equal(v:t_object, type(ctx))
  assert_equal("X", ctx.text)  # <== E715
  assert_equal(42, ctx.index)  # <== E715
enddef

Test_MakeContext()

Sourcing the latter script results in E715: Dictionary required any time a member is accessed.

@lifepillar With :type (now merged) you can do the following to easily specify the type and avoid E715.

vim9script

import '/tmp/xlib.vim'

type Context = xlib.Context
const MakeContext = xlib.Context.MakeContext

def Test_MakeContext()
  var ctx: Context = MakeContext("X", 42)
  echo ctx.text
  echo ctx.index
enddef

Test_MakeContext()

There are still things that are a hassle or unexpected. Which make this a workaround.

• If Cannot use class type of itself in the method #12369 is fixed, the type could be automatically assigned.
• The typealias can not be used as a class type in all situations
  Context.new('X', 42) does not work, necessitating the MakeContext class function.
  const MakeContext = Context.MakeContext does not work

[errael]

Rebase with :type. Without design review/feedback (see description #13404 (comment)) this PR is on hold.

[yegappan]

Why was this PR closed? Is this not needed anymore?

[errael]

Why was this PR closed? Is this not needed anymore?

I closed it in favor of yegappan#2 which is identical to this PR.

[yegappan]

• The typealias can not be used as a class type in all situations

I have updated PR #13441 to address this issue.

[chrisbra]

@yegappan can you please check , if Commit feaccd2 includes your latest change?

[yegappan]

@yegappan can you please check , if Commit feaccd2 includes your latest change?

Yes. Your commit includes the latest change. Thanks.

[chrisbra]

Thanks. Glad I didn't merge 5 minutes earlier :)