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

Information lost on some Term.ApplyInfix use cases #1843

Open
RCMartins opened this issue Mar 7, 2019 · 7 comments

Comments

@RCMartins
Copy link
Contributor

@RCMartins RCMartins commented Mar 7, 2019

val tree1 = q"list foo (_ fun (_.bar))"
val tree2 = q"list foo (_ fun _.bar)"

tree1.structure == tree2.structure // true

// Both have this structure:
Term.ApplyInfix(
  Term.Name("list"),
  Term.Name("foo"),
  Nil,
  List(Term.ApplyInfix(
    Term.Placeholder(),
    Term.Name("fun"),
    Nil,
    List(Term.Select(Term.Placeholder(), Term.Name("bar")))
  ))
)

These are different trees, so the structure should have all the information to construct the original syntax.

@olafurpg

This comment has been minimized.

Copy link
Member

@olafurpg olafurpg commented Mar 7, 2019

Thanks for reporting! I can confirm the two programs compile differently but have the same AST

@ class Foo { def map[T](fn: (Int, Int) => T): Foo = this }
@ new Foo() map (_.toBinaryString + _.toBinaryString)
res9: Foo = ammonite.$sess.cmd8$Foo@40c28b0d

@ new Foo() map (_.toBinaryString + (_.toBinaryString))
cmd10.sc:1: missing parameter type for expanded function ((x$1: <error>) => x$1.toBinaryString.$plus(((x$2) => x$2.toBinaryString)))
val res10 = new Foo() map (_.toBinaryString + (_.toBinaryString))
                           ^
Compilation Failed

What is the expected parse tree for each case? I'm not sure what the best approach is to represent the differences between the two programs. We can inspect the tokens to determine if a tree is wrapped in parentheses, but tokens are lost once you do Tree.transform.

@RCMartins

This comment has been minimized.

Copy link
Contributor Author

@RCMartins RCMartins commented Mar 9, 2019

@olafurpg I'm curious to know how does the scalac (or any other scala compiler) deals with parsing this kind of programs, they should have an internal AST that can distinguish between these two problems right?
I'm not sure how to check for that.

@olafurpg

This comment has been minimized.

Copy link
Member

@olafurpg olafurpg commented Mar 9, 2019

The compiler doesn’t have infix application nodes or placeholder nodes, I think it desugars “_” directly into lambas.

@olafurpg

This comment has been minimized.

Copy link
Member

@olafurpg olafurpg commented Mar 12, 2019

One possible solution could be to introduce a new tree node Term.Parens(term: Term) that would only be used to represent significant parentheses. The parser can be updated in this case to emit the following parse tree for list foo (_ fun (_.bar))

Term.ApplyInfix(
  Term.Name("list"),
  Term.Name("foo"),
  Nil,
  List(Term.ApplyInfix(
    Term.Placeholder(),
    Term.Name("fun"),
    Nil,
-    List(Term.Select(Term.Placeholder(), Term.Name("bar")))
+    List(Term.Parens(Term.Select(Term.Placeholder(), Term.Name("bar"))))
  ))
)

The parser would still not emit Term.Parens for redundant parentheses like ((a + (b)))

While working on pretty-printing, I have sometimes wanted a custom tree node to indicate that a term should be wrapped in parentheses even the parentheses are redundant.

wdyt?

@RCMartins

This comment has been minimized.

Copy link
Contributor Author

@RCMartins RCMartins commented Mar 13, 2019

I think adding the Term.Parens it's a good solution for this problem.

While working on pretty-printing, I have sometimes wanted a custom tree node to indicate that a term should be wrapped in parentheses even the parentheses are redundant.

If I'm understanding correctly, this would be useful to avoid using Show.sequence("(", term, ")") and instead use Term.Parens(term) directly?

@olafurpg

This comment has been minimized.

Copy link
Member

@olafurpg olafurpg commented Mar 13, 2019

The primary use case would be to allow tree.transform to produce nodes that are explicitly wrapped in parentheses

q"a + b".transform {
  case q"b" => Term.Parens(q"b")
}.syntax
// a + (b)
@ghik

This comment has been minimized.

Copy link

@ghik ghik commented Nov 3, 2019

My current problem with how Scalameta represents underscore/placeholder based lambdas is that it's impossible to tell that an expression represents a lambda without looking inside it, searching for Term.Placeholder trees and manually figuring out which enclosing expression these placeholders apply to (which, as this issue shows, can be non-trivial). I would like to be able to look at a tree and immediately tell "oh, it's a lambda!". I can do that if it's represented as Term.Function but I can't do it with placeholder-based lambdas.

Therefore, I think we should introduce some wrapper node which would mark these lambda expressions explicitly. This would also resolve the ambiguity originally reported in this issue.
I'm going to call this node Term.PlaceholderFunction (as always, coming up with a good name is the hardest part).

Now, _ fun (_.bar) would be represented as

Term.PlaceholderFunction(
  Term.ApplyInfix(
    Term.Placeholder(),
    Term.Name("fun"),
    Nil,
    List(Term.PlaceholderFunction(Term.Select(Term.Placeholder(), Term.Name("bar"))))
  )
)

while _ fun _.bar would be represented as

Term.PlaceholderFunction(
  Term.ApplyInfix(
    Term.Placeholder(),
    Term.Name("fun"),
    Nil,
    List(Term.Select(Term.Placeholder(), Term.Name("bar")))
  )
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.