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

Optional / required arguments for functions #366

Closed
wants to merge 13 commits into from
Closed

Conversation

hishamhm
Copy link
Member

@hishamhm hishamhm commented Jan 29, 2021

This is an experiment implementing arity checks as discussed in #71.

These are not option types, but they address one of the most common use-cases for which option types are usually wanted: catching missing arguments in function calls.

Syntax is function (r: string, o?: number, p?: boolean) where r is required and o and p are optional. Only the rightmost arguments can be optional. For type definitions, the syntax to specify optional arity in unnamed arguments is local f: function(string, ? string, ? boolean).

Behavior and syntax for return types have not changed in this PR; it only deals with input arguments.

My first impression is overall positive: it's nice to have the compiler tell you that you forgot one argument in a function!

However, there are some quirks. Assigning a function function (string, string) to a callback of type function (string...) is not valid, because the caller of the callback may not pass sufficient arguments — this is correct, strictly speaking, but it causes annoyances in things like the function callback of string.gsub and the __call metamethod... you're forced to make all arguments that match the vararg optional, which feels unnatural given that you can often be sure that the arguments are there based on context that the programmer knows but the compiler doesn't (for example, the captures in gsub). If we move forward with this, I might make an unsound exception for subtype matching of variadic functions, since those are often used in especially dynamically-typed callback APIs.

@github-actions
Copy link

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

@catwell
Copy link
Contributor

catwell commented Jan 29, 2021

Yes, callbacks and optional arguments lead to the variance rabbit hole, see #318 (also, does "optional" mean "you can pass nil" and vice-versa...)

@lenscas
Copy link
Contributor

lenscas commented Jan 29, 2021

Yes, callbacks and optional arguments lead to the variance rabbit hole, see #318 (also, does "optional" mean "you can pass nil" and vice-versa...)

Unless this PR changes this, a T is the same as T | nil, so I see no reason why you wouldn't be able to pass nil explicitly to an optional parameter.

@catwell
Copy link
Contributor

catwell commented Jan 29, 2021

Of course. But there is no type corresponding to an optional parameter (it is not T | nil) and that causes a lot of issues.

Anyway this issue is one of the reasons why TypeScript went with an unsound type system. There is an even better explanation here. I am not saying Teal needs to do this, they had to because you can't have a type system that is annoying to use with callbacks when extending JavaScript. In Teal / Lua I would say having, for instance, to cast manually with as would be fine.

@hishamhm
Copy link
Member Author

Yes, this PR does not change the other subtyping rules, only the arity rules, so yes, you can pass an explicit nil.

In fact, this PR is implemented in pieces, and one serious possibility is to apply the arity rules for function calls only, and leave the function type comparisons unchanged (keep the check in the @funcall operator, but remove the parts that change same_type and is_a).

It would be a smaller step towards correctness checking of your programs, but it would still be a step nonetheless, and would avoid all of the annoyances with variance mismatching (with the caveat of keeping them as runtime crashes in case of mistakes, just as they currently are).

It might make sense to keep arity checks for calls and returns only, and save the actual subtyping comparisons for eventual true option types. In my experience, the former caught some mistakes in my program and caused no major annoyances. The lack of arity checking in subtyping only annoyed me in assignments, because in forward function declarations you want them to match perfectly, but in callbacks you often want it to be more lenient. At one point I had an even stricter version of subtyping comparison implemented than what's on the PR right now, but then I found myself sprinkling dummy arguments all over to make the typechecker happy and that didn't feel right.

@hishamhm
Copy link
Member Author

Anyway this issue is one of the reasons why TypeScript went with an unsound type system. There is an even better explanation here.

Yeah, this is precisely the awkward behavior I hit right away. If we merge this PR only until commit 90f31f9, then we get pretty much the TypeScript behavior. I'm thinking this is the safest way to go, and then I can implement the same for return arities (which will affect return statements only).

@svermeulen
Copy link

svermeulen commented Apr 17, 2022

+1000
After using Teal a lot lately, missing a function parameter and nil reference errors are the biggest runtime errors I encounter

@euclidianAce euclidianAce mentioned this pull request Sep 22, 2022
@Nukiloco
Copy link

What is the current status of this pull request?

@hishamhm
Copy link
Member Author

hishamhm commented Jan 8, 2024

@svermeulen @Nukiloco This PR was merged into the next branch, along with a bunch of other features!

For backwards compatibility, I've added --feat-arity=off flag, which allows for the more lax argument checking to still work. But it's not a lot of work to port code to the new style, adding ? to function arguments where needed!

https://github.com/teal-language/tl/tree/next

@hishamhm hishamhm closed this Jan 8, 2024
@svermeulen
Copy link

@hishamhm Sounds awesome, looking forward to those features getting merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants