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

Binary expressions with function calls on their RHS get incorrectly parsed #153

Open
ketkarameya opened this issue Apr 13, 2022 · 11 comments
Labels
ast-incorrect The grammar causes some code to parse without error, but generate an incorrect result

Comments

@ketkarameya
Copy link
Contributor

Hello,
Thanks a lot for making this grammar available. I was tinkering with the parser and I found that for the below scenario, the condition of the first if_statement is parsed as a conjunction_expression, while for the second if_statement it is parsed as a call_expression.

import Foundation

class SwiftExamples {

    func test() -> Bool {
      if x && y && z {
      	return true
      }
      
      if x && y && z.def() {
      	return true
      } 
    }
}

I was wondering how difficult a fix is this? I am very new to writing / updating tree-sitter grammar, and I am not sure where to start.

@ketkarameya
Copy link
Contributor Author

ketkarameya commented Apr 13, 2022

I think the problem is because call_expression and navigation_expression accept expression (for caller and target respectively), where expression could be a binary_expression. But i did not manage to fix it :(

@alex-pinkus
Copy link
Owner

I suspect you are right, and that we may need to do something similar to the ternary expr hack. There, the ambiguity is with a ? b : c() for the same reason, so to force it to be parsed the desired way, we intentionally introduce a conflict.

@alex-pinkus alex-pinkus added the ast-incorrect The grammar causes some code to parse without error, but generate an incorrect result label Apr 14, 2022
@ketkarameya
Copy link
Contributor Author

I see. Thanks a lot. I ll take this up.

@alex-pinkus
Copy link
Owner

Thank you for fixing this in #158 !

@ketkarameya
Copy link
Contributor Author

ketkarameya commented Apr 20, 2022

I reckon #158 is incomplete? I think it still needs to handle the rhs of other binary expressions. Should this same pattern be re-applied at all these site? or is there a cleaner way to do this? Let's not close this issue until we address the entire problem?

@alex-pinkus
Copy link
Owner

Yeah, I think that's fair. We probably want to incorporate that workaround into some sort of helper function if we're going to keep applying it.

Theoretically there's some precedence / associativity relationship between function calls and the various types of binary expressions that encodes the right behavior, removing the need for the workaround, but the precedence of function calls is very precariously balanced against all sorts of other things right now. I'm somewhat skeptical that there's a way to make it work other than just applying this workaround in all of those places.

We can absolutely use this to track the other places!

@alex-pinkus alex-pinkus reopened this Apr 21, 2022
@alex-pinkus alex-pinkus changed the title Incorrectly parsed if_statement's condition expression Binary expressions with function calls on their RHS get incorrectly parsed Apr 21, 2022
@nighthawk
Copy link

I think I just came across another case of this when parsing something like f() == g().

I'm still new to tree-sitter. Any suggestions for how to address this?

Screen Shot 2022-05-25 at 14 34 59

@alex-pinkus
Copy link
Owner

Thanks for pinging on this, @nighthawk. I think @ketkarameya is right and we should apply the hack elsewhere; I was just dragging my feet in the hopes that a more elegant solution would present itself.

I've published #187 to apply the hack to all binary expressions except the additive and multiplicative ones. The issue exists there too, but for some reason, the hack causes those expressions to act right-associative despite being left-associative. Maybe that's the lesser evil here, but if so, we can apply it separately.

Once that PR is merged, equality_expression will be handled properly. I'll leave this open until additive and multiplicative expressions are handled properly too.

@ketkarameya
Copy link
Contributor Author

Hey thanks a lot for #187. I was wondering what is the impact of addition/multiplication expressions being right-associative (instead of left-associative)?

@alex-pinkus
Copy link
Owner

It means that in an expression like: let a = 1 + 2 + 3, the resulting AST groups the operations as though they were let a = (1 + 2) + 3, but if we apply the expr hack, it becomes let a = 1 + (2 + 3). It doesn't matter for integer addition because they both evaluate to the same thing, but it's still not right. It probably happens with other binary operations too, but things like equality operations are unlikely to get chained since there's no reason to do a == b == true.

I want to better understand why that happens when we apply the hack before applying it to those operations where it's that much more likely. The "it's not right" argument, of course, also applies to the current interpretation of a() + b(), which is probably much more commonly a problem in the wild than a + b + c.

@ketkarameya
Copy link
Contributor Author

I see, I see. I had observed something similar, when earlier I had tried to apply the hack everywhere. I agree, let's try to investigate why this is happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast-incorrect The grammar causes some code to parse without error, but generate an incorrect result
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants