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

Feature/record body declared methods #642

Merged

Conversation

JR-Mitchell
Copy link
Contributor

What?

  • Adds new Type.is_self property for identifying a function/method parameter that is self
  • Adds new string parameter name for passing the name of the body / type being parsed to:
    • the ParseBody type
    • parse_newtype
    • parse_record_body
  • Passes the record name using the name parameter for all cases where parse_record_body is called
  • Updates parse_argument_type so that if the argument is named self, the type is marked as is_self = true
  • Updates parse_function_type so that if the first argument has is_self == true, the function type is marked as is_method = true
  • Updates parse_record_body so that, before a method is added to the record's fields, it is verified that the first argument matches the type of the record, and if it does not then it is marked as is_self = false and the function type as is_method = false
  • Updates recurse_type so that the first argument is also recursed in a method if that argument is marked as is_self == true
  • Consolidates the near-indentical shallow_copy and shallow_copy_type methods into one

Why?

See #638 - this PR allows for forward declaration of methods in records, based on the first argument being named self and being correctly typed.
This allows for warnings and clearer errors when using types defined in .d.tl files, as well as allowing methods to be declared in one scope and then defined in another without losing these warnings.

Anything else?

There is an edge case which may need discussion: the code

local record MyRecord<T>
    add: function(self: MyRecord<integer>, other: MyRecord<integer>)
end
local first: MyRecord<integer> = {}
local second: MyRecord<integer> = {}
first.add(second)

does not raise a warning, since add is not treated as a method for MyRecord, even though it could be argued that it should be treated as a method of the type realisation MyRecord<integer>.
I have a vague idea of how this could be achieved if we want this behaviour, but haven't been able to properly implement it yet.
Let me know whether it is fine that there is no warning in this situation, whether this can go into a separate issue / PR, or whether I should try to resolve it in this PR.

@github-actions
Copy link

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

@hishamhm
Copy link
Member

There is an edge case which may need discussion

I think this is fine. The best we could do in that case would be to add a warning hint if the type (including type arguments) for a first argument called self is not nominally the same as the record the function is in, but for such specific scenario the programmer might want to call that self consciously and we'd be drifting into coding style, so better not make the type checker that opinionated.

Overall the PR looks good as far as I can tell! Merging, thanks!!

@hishamhm hishamhm merged commit 67ea51b into teal-language:master Apr 27, 2023
@hishamhm
Copy link
Member

also: released in Teal 0.15.2!

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

2 participants