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

Support resolving all kinds of expressions #726

Merged
merged 12 commits into from Sep 10, 2019
Merged

Support resolving all kinds of expressions #726

merged 12 commits into from Sep 10, 2019

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jul 22, 2019

Continuation of #703

  • Add BinaryExpression
  • Add InstanceofExpression
  • Add CommaExpression
  • Add TernaryExpression
  • Add FunctionExpression
  • Add NewExpression
  • Update UnaryPrefixExpression
  • Update UnaryPostfixExpression
  • Tests
  • Document difference between instance and static overloads for prefix/postfix inc/dec

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 23, 2019

Tests are failing because overloading prefix ++ and -- no longer has any side-effects but acts like any other operator overload. Previously, the result of a ++foo was re-assigned to foo for the sake of the operator's semantics, while now it doesn't anymore. Not sure what's best there, because doing it either way or the other will prevent doing it the other way around.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 23, 2019

So I figured that there's actually a way to support both behaviors: An instance overload now re-assigns and requires a compatible return type, essentially equivalent to the old behavior, while a static overload does not attempt to re-assign and behaves like any other overload, allowing any return type.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 24, 2019

Turned out there's another issue with overloads. Let's say we have

class Foo {
  @operator("+")
  add(other: Foo): string {
    return "add";
  }
}
var foo = new Foo();
foo += foo;

then this will currently assign "add" to foo without an error, which is really bad as it breaks types. The obvious fix is to make this an error, but I also feel that we might want to have distinct overloadable + and += overloads.

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 3, 2019

Getting back to this issue I figured that half of the time we'd like to know the type of an expression, not the element. Now, while elements can be converted to types and vice-versa, that's relatively inefficient because sometimes the underlying logic yields an element and sometimes it yields a type, making it necessary to go from type to element to type for example, which quickly accumulates for sub-expressions.

Appears there are two ways to solve this: One is to have separate logic for resolving elements or types and the other is reworking this stuff towards resolving types only, with separate logic to obtain a target (Local, Global, Field or Property). The latter might also get rid of workarounds like FunctionTarget, but I still have to think this through in detail.

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 8, 2019

With the resolveXYType utility in place, much of the compiler can be refactored to make use of these exclusively as done here. That's essentially what we always needed in that we can now determine the type of any expression before actually compiling it. Makes me wonder whether the refactoring can go as far as to eliminate the element resolve stuff entirely, moving handling of different element kinds to the respective compileXY functions. Quite some work to do this for everything, though. Not sure if it makes sense to merge this, so we can at least compile every combination of expressions already, and do the refactoring later. Hmm.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 8, 2019

Yeah I guess better move iteratively and make rest refactoring wok later if everything works in this PR already. This PR pretty is important and should be landed asap I think

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 9, 2019

Ok, this should be in a merge-able state now. Some notes for follow-up PRs:

  • Resolving of element and property access expressions to elements differs from all the other expressions in that these resolve to the target, not the actual element being accessed. For element access particularly fixing that requires abstracting indexed get/set into a property-like element one can return since it's not known whether returning the getter or setter is expected. So the returns here would be such an index pair for element access, respectively an instance field, static global, instance method or static function for property access.
  • Most of the time it's more efficient to either resolve the type or the element and convert it in the respective other method. Unifying that will need additional refactoring incl. taking the former note into account.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 9, 2019

Btw I wonder is this issue #573 resolved with this PR?

@MaxGraey
Copy link
Member

MaxGraey commented Sep 9, 2019

In TypeScript such series produce catastrophic backtracking and seems to "compiler bomb". See microsoft/TypeScript#30833 (comment)

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 9, 2019

#573 appears to compile fine with this, yeah.

Regarding the compiler bomb scenario, this compiles without a noticeable delay:

class A {
  get a(): A { return this; }
}
var a = new A();
a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=(a=a.a).a).a).a).a).a).a).a).a).a).a).a).a).a).a).a).a).a).a).a

@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 9, 2019

There might still be occasions where compilation isn't as fast as it could be, though, because this is still missing some refactoring. To recap, the underlying concern for implementing this is that the compiler currently doesn't know the type of an expression it is going to compile beforehand, leading to various issues. This PR implements the utility necessary to resolve everything before compiling anything, but in its current form focuses on enabling any sort of expression, with a follow-up looking into making this more efficient, i.e. by eliminating unnecessary steps and using the new utility where it makes compilation more efficient.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 9, 2019

Happy to hear we haven't this problem) Great. I'll check everything asap

Copy link
Member

@MaxGraey MaxGraey left a comment

Choose a reason for hiding this comment

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

LGTM

@dcodeIO dcodeIO merged commit 6d628ce into master Sep 10, 2019
@dcodeIO
Copy link
Member Author

dcodeIO commented Sep 10, 2019

@dcodeIO dcodeIO deleted the rework-resolver2 branch November 8, 2019 02:00
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