Feature/warn method call for correct type

[JR-Mitchell]

What?

Adds a warning message to non-method calls on methods where the argument type is compatible with the receiver type.

Why?

Currently, an error is thrown for code like this:

record MyRecord
end
function MyRecord:read(argument: string)
end
local myRecord: MyRecord = {}
myRecord.read("hello")

telling the programmer to call :read rather than .read. This is useful, as it captures a common programmer error.

However, if the method argument type is the same as the receiver type, for example:

record MyRecord
end
function MyRecord:read(other: MyRecord)
end
local myRecord: MyRecord = {}
local otherRecord: MyRecord = {}
myRecord.read(otherRecord)

then no error is thrown, as the type of otherRecord is compatible with the receiver type.

But, in this case, it is very possible that the programmer still intended to write myRecord:read(otherRecord), and just made a typing mistake. Therefore, this PR adds a warning to any non-method call of a method where the argument type is compatible with the receiver type, so that this common mistake is identified when checking / building with teal.

Anything else?

Could someone let me know how versioning works on this project? I noticed that there is a constant local VERSION = "0.15.1+dev" at the top of tl.tl - should I be bumping this version with my PR, and if so, what to?

[github-actions[bot]]

Teal Playground URL: https://640--teal-playground-preview.netlify.app

[hishamhm]

yay, thanks for the PR! I haven't reviewed the code yet, but replying to your questions:

Could someone let me know how versioning works on this project?

That version constant is used by tl --version. On each release tag, that variable is set to the x.y.z version number, and afterwards the main branch gets updated to say x.y.z+dev so that users can easily tell whether they're running a release or unreleased version by running tl --version.

should I be bumping this version with my PR, and if so, what to?

No need to!

[hishamhm]

Getting back to the topic at hand...

then no error is thrown, as the type of otherRecord is compatible with the receiver type.

The reason why I didn't make this an error originally is because I've had to invoke methods with a different "self" in the past when coding in Lua, for example when passing methods around as function values.

This, for instance, triggers a warning in this PR's branch:

local record MyRecord
end
function MyRecord:read(other: MyRecord)
end
local myRecord: MyRecord = {}
local otherRecord: MyRecord = {}

local fn = myRecord.read

fn(myRecord, otherRecord) -- valid code, but triggers a warning

I suppose the assignment should clone* the type object for fn but "forget" the value of is_method...

(* for cloning a function Type for the purposes of unsetting is_method, I suppose a shallow_copy of the Type object, with the typeid replaced by a new_typeid(), should suffice.)

In fact, this weirdness is afflicting the master branch as well:

local record MyRecord
end
function MyRecord:read2(n: number)
end
local myRecord: MyRecord = {}

local fn2 = myRecord.read2

fn2(12) -- check out the error this causes 😂

[JR-Mitchell]

That's a good point - I will add a test case for this and see if I can remove the is_method property for such function assignments.

[JR-Mitchell]

I still plan on getting round to unsetting is_method when assigning a method, but meanwhile have added suppression of the warning in the case that was raised in #638

[JR-Mitchell]

@hishamhm 33916fb fixes the issues you flagged with assignment by cloning the type and unsetting is_method.

I have a small question here around efficiency - should I aim to reduce the number of Types created by cacheing methodless clones? Something like (roughly):

if not methodtype.nonmethod_clone then
    local functype = shallow_copy_type(methodtype)
    functype.is_method = false
    methodtype.nonmethod_clone = functype
end

Or is this not worth adding a new property to avoid instantiating a few extra type clones if this pattern is used in multiple places?

[hishamhm]

I have a small question here around efficiency - should I aim to reduce the number of Types created by cacheing methodless clones?

@JR-Mitchell good question! I usually do quick-and-dirty performance tests with hyperfine by running hyperfine "./tl check master.tl" (where master.tl is a copy of tl.tl from master — comparing tl check tl.tl in two different branches can be misleading because the amount of code in each version is different; so it's important to make sure you time runs against the same input file).

If you don't see a noticeable difference beyond the noise you get from multiple runs, then it's not worth it.

[JR-Mitchell]

I've checked tl check and tl gen for 50 runs both cacheing and non-cacheing, and they were within 0.1 standard deviations of each other, so I think it's not worth it to make this change. Can you review and let me know if this can be merged? :)

[hishamhm]

@JR-Mitchell Looking good! I squashed the test and code commit pairs together and merged manually! Thanks!!