X Tutup
Skip to content

New functions#283

Merged
Perryvw merged 20 commits intoTypeScriptToLua:new-functionsfrom
tomblind:new-functions
Nov 24, 2018
Merged

New functions#283
Perryvw merged 20 commits intoTypeScriptToLua:new-functionsfrom
tomblind:new-functions

Conversation

@tomblind
Copy link
Collaborator

This PR brings everything up to what is spec'd out here: #250 (comment)

  • non-static class/interface methods and function properties (lambdas) are given a context parameter
  • static methods/lambdas and stand-alone functions have no context parameter
  • errors are thrown for attempts to assign methods to non-methods and vice-versa
  • stand-alone functions can be given a context by explicitly adding a 'this' parameter
  • methods can be declared without a context by explicitly declaring a 'this: void' parameter

A few notes:

  • NoContext decorator has been removed
  • bind/call/apply currently throw errors when called with non-methods. Are there any good reasons to support this?
  • Custom constructor functions are expected to be non-methods. If that needs to be supported, it might need to be an option in the decorator.

Current edge cases:

interface Func<T> {
    (this: T, s: string): string;
}
let fn: Func<void> = foo.method; // Should Error

Right now, Func is detected as having context when it shouldn't. I need to figure out how to resolve the generic parameter when checking assignments.

interface A {
    method(s: string): string;
}
interface B {
    method(this: void, s: string): string;
}
declare let a: A;
let b: B = a; // Should Error

Assigning to interfaces allows the check to be bypassed and calling the method on 'b' will incorrectly use the dot syntax. This should be solvable by recursively checking assignments to interface types, but this is complex and will likely add quite a lot of overhead. Need to think on this one...

@Perryvw
Copy link
Member

Perryvw commented Nov 23, 2018

Wow you've been busy, sounds very good! I'll have a look at this later today or tomorrow.

@tomblind
Copy link
Collaborator Author

No problem - whenever you have time. It's definitely not ready for integration but worth taking a look at.

One more edge case I forgot to mention:

interface F {
    (this: Foo, s: string): string;
    (s: string): string;
}
let fn: F = foo.method; // Error

Overloads that have both methods and non-methods are currently treated as non-methods when doing the assignment checks. We might want to change things to allow either in this situation.

Copy link
Member

@Perryvw Perryvw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy with these changes. Looks like this will also be a great match to the 2-phase transpilation process @lolleko is working on. I will merge this tomorrow if there are no more changes.

} else if ((fromType as ts.TypeReference).typeArguments && (toType as ts.TypeReference).typeArguments) {
// Recurse into tuples/arrays
(toType as ts.TypeReference).typeArguments.forEach((t, i) => {
this.validateAssignment(node, (fromType as ts.TypeReference).typeArguments[i], t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Would it be possible toType.typeArguments is shorter/longer than fromType.typeArguments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely possible for fromType to have more than toType (destructuring, but not catching everything in the tuple), but I haven't come up with a situation where toType could have more.

public functionBind(): void {
const source = `const abc = function (a: string, b: string) { return this.a + a + b; }
const source = `const abc = function (this: { a: number }, a: string, b: string) { return this.a + a + b; }
return abc.bind({ a: 4 }, "b")("c");`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks strange actually, should this not be abc.bind({a: 4})("b", "c")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bind() actually allows you to bind initial arguments as well. This is testing that functionality.

@Perryvw
Copy link
Member

Perryvw commented Nov 23, 2018

You should probably remove your /* NoContext */ decorators by the way, now they are no longer used. (In the lualib functions)

@tomblind
Copy link
Collaborator Author

Whoops! I'll dig around and make sure there's no others still floating about too.

@Perryvw Perryvw merged commit 515e5de into TypeScriptToLua:new-functions Nov 24, 2018
@lolleko lolleko added this to the 1.0.0 milestone Nov 27, 2018
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.

3 participants

X Tutup