-
Notifications
You must be signed in to change notification settings - Fork 99
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
Feature/warn method call for correct type #640
Feature/warn method call for correct type #640
Conversation
…ing potentially wrong use of self.
Teal Playground URL: https://640--teal-playground-preview.netlify.app |
yay, thanks for the PR! I haven't reviewed the code yet, but replying to your questions:
That version constant is used by
No need to! |
Getting back to the topic at hand...
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 (* for cloning a function 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 😂 |
That's a good point - I will add a test case for this and see if I can remove the |
…method on record typetype
…eiver is the type itself
I still plan on getting round to unsetting |
…s on aliases of methods
…opied to a newly declared variable or table entry
@hishamhm 33916fb fixes the issues you flagged with assignment by cloning the type and unsetting I have a small question here around efficiency - should I aim to reduce the number of
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? |
@JR-Mitchell good question! I usually do quick-and-dirty performance tests with hyperfine by running If you don't see a noticeable difference beyond the noise you get from multiple runs, then it's not worth it. |
I've checked |
@JR-Mitchell Looking good! I squashed the test and code commit pairs together and merged manually! Thanks!! |
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:
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:
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 oftl.tl
- should I be bumping this version with my PR, and if so, what to?