Implement resolveBinaryExpression #689
Conversation
| let right = expression.right; | ||
| let leftElement = this.resolveExpression(left, flow, contextualType, reportMode); | ||
| if (!leftElement) return null; | ||
| let leftType = (<TypedElement>leftElement!).type; |
There was a problem hiding this comment.
If it's always a TypedElement, I think resolveExpression should have it as the return type. Unless it's not always one, ofc, which would mean it misses something here.
There was a problem hiding this comment.
That's what I thought, but what's an example of something that does not resolve to a typedElement?
There was a problem hiding this comment.
What can happen there for example is that if an expression referencing a class name is resolved, the element is a static ClassPrototype without a type. The same should be true for function names, because the function can be generic with multiple instances so a FunctionPrototype is returned. There might be others that I forgot.
There was a problem hiding this comment.
I see. Is it possible to circumvent this by using resolveType here instead of resolveExpression?
There was a problem hiding this comment.
resolveType is meant for type nodes exclusively here, so I think it isn't applicable. If I'm not mistaken the resolver should actually return the ClassPrototype respectively FunctionProtoype here (since we want the element), and whatever the caller of resolveExpression is would deal with it appropriately.
| if (!rightElement) return null; | ||
| let rightType = (<TypedElement>rightElement!).type; | ||
| let commonType: Type | null; | ||
| if (commonType = Type.commonDenominator(leftType, rightType, true)) { |
There was a problem hiding this comment.
There are some occasions where it doesn't work this way, like for shifts which always prefer the LHS. I think there should be a switch over all possible operators here that mimics what compileExpression etc. do. In this case, binaryOperatorKindFromToken can possibly be incorporated into the switch.
|
Thanks for giving this a shot, this is definitely something we have to improve. I'm also currently in the process of doing a cleanup pass over the resolver, and while doing this I figured that your idea for linking basic type classes with their respective basic types is most likely a basic building block here, because this way, every expression being resolved can resolve to a concrete |
|
Not sure if this is related, but it looks like binary expressions have a problem :). ERROR AS100: Operation not supported.
assert("ABCDE" == ("a" + "b" + "cde").toUpperCase(), "v8 test 19");
~~~~~~~~~~~~~~~~~
in std/string.ts(549,19)Do you think this is fixed here? |
|
Should be. But my fix is not proper. I'll rework it sometime soon |
|
@bowenwang1996 Thanks for PR but I guess all proposed changes already in master. So I guess we could close this PR? |
An attempt to implement
resolveBinaryExpressionto further improve the resolver and fix bugs like #625. The function is implemented by resolving to the common denominator of left expression and right expression and then checking operator overloading. However, I noticed that the helper functioncommonDenominatoris not properly implemented and does not resolve to the common superclass of two subclasses, if the two subclasses are different. This limitation can probably be addressed later. For now, expressions like(a + b).toString()can be properly resolved, even if it involves operator overloading.