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

[SemanticDB] Support synthetics: implicit params, context params, and implicit conversions #13288

Merged
merged 7 commits into from
Aug 30, 2021

Conversation

tanishiking
Copy link
Member

Related #13135

This PR adds initial support of synthetics to SemanticDB.

Extract synthetics for the following synthesized trees.

  • Implicit params (application)
  • Context params
    • context params application
    • Invented given name
    • Anonymous using clause

Questions

Added synthetics

implicit / context params application

def foo(x: Int)(using Int) = ???
def m(using x: Int) = foo(0)

// synthetics
def m(using x: Int) = foo(0)<<(x)>>
Synthetic(
  <foo(0)>,
  ApplyTree(
    OriginalTree(<foo(0)>),
    List(
      IdTree(<x>),
    )
  )
)

Same with Implicit parameters synthetics in Scala2 https://scalameta.org/docs/semanticdb/specification.html#synthetic-1

anonymous context params

def foo(using Int) = ???
 
// synthetic
def foo(using x$1: Int) = ???
Synthetic(
  <Int>,
  IdTree(<x$1>)
)

(Maybe range should be between using and Int, instead of overlaps with Int)?.

Invented given value

given Int = 1

// synthetic
given given_Int: Int = 1
Synthetic(
  <given Int = 1>,
  IdTree(<given_Int>)
)

Maybe the range should be between given and Int?

Implicit conversion

"fooo".stripPrefix("o")

// synthetic
Synthetic(
  <"fooo">,
  ApplyTree(
    IdTree(<Predef.augmentString>),
    List(OriginalTree(<"fooo">)))
)

[15:13..15:16):Int => x$1
[24:13..24:14):X => x$1
[34:8..34:20):given_Double => *(intValue)
[40:8..40:15):given_Y => *(given_X)
Copy link
Member Author

Choose a reason for hiding this comment

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

@bishabosha bishabosha self-requested a review August 17, 2021 11:44
@bishabosha bishabosha self-assigned this Aug 17, 2021
@tgodzik tgodzik self-requested a review August 17, 2021 14:51
@bishabosha
Copy link
Member

the work looks good so far, I do find it a bit strange to use Id trees to represent "missing identifier" in a definition - perhaps we need a new tree kind there

@bishabosha bishabosha assigned tanishiking and unassigned bishabosha Aug 17, 2021
@tanishiking tanishiking force-pushed the synthetic-implicit-conversion branch from d2fd229 to 3768c04 Compare August 23, 2021 04:29
@tanishiking
Copy link
Member Author

the work looks good so far, I do find it a bit strange to use Id trees to represent "missing identifier" in a definition - perhaps we need a new tree kind there
#13288 (comment)

Right, I got second thought we should extend SemanticDB's tree to add something like ValDef to represent a synthesized identifier.
Currently, this implementation models using x$1: Int as IdTree(x$1), but it's confusing for synthetics consumers: to know what IdTree(x$1) means, they need to know this implementation.

what do you think? @bishabosha @tgodzik

@bishabosha
Copy link
Member

bishabosha commented Aug 23, 2021

Right, I got second thought we should extend SemanticDB's tree to add something like ValDef to represent a synthesized identifier.
Currently, this implementation models using x$1: Int as IdTree(x$1), but it's confusing for synthetics consumers: to know what IdTree(x$1) means, they need to know this implementation.

I think its unclear how to process the tree because usually a synthetic has a clear "hole" representing source code which the synthetic wraps, in this case though, the source code contains a hole and the IdTree is the plug - this is an inversion of the current mental model

@tanishiking
Copy link
Member Author

I just removed synthetics for invented givens and anonymous using params for the time being.
560fc18

Let's discuss the design for them in another issue (maybe in scalameta/scalameta ?) and make this PR proceed without them?

@tanishiking tanishiking requested a review from bishabosha August 24, 2021 17:27
) =>
) &&
// exclude synthesized method parseArgument by @main
tree.symbol != defn.CLP_parseArgument =>
Copy link
Member

Choose a reason for hiding this comment

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

this isn't enough to cover all the @main boilerplate, e.g. a main method with varargs will call the parseRemainingArguments and insert implicit args e.g. :

@main def readInts(ints: Int*): Unit = println(ints.mkString(","))

this will produce the synthetic:


Synthetics:
[6:0..6:0): => *(given_FromString_Int)

Copy link
Member

Choose a reason for hiding this comment

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

A way to avoid this and more is to add a case to ExtractSemanticDB for case tree: TypeDef where we do not traverse the children if the symbol is Invisible -

case tree: TypeDef =>
  if !tree.symbol.is(Invisible) then
    traverseChildren(tree)

this seems to only affect classes generated by @main so far - will need @tgodzik to verify this is ok as it will remove all occurrences

Copy link
Member

Choose a reason for hiding this comment

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

alternatively adding a case in the Template traversal where we avoid the body if the class symbol is Invisible

Copy link
Member Author

Choose a reason for hiding this comment

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

14e9be9
So I did the latter option (adding a case for the Template whose owner is an Invisible and stop traverse for that), and excluded the synthetics of both parseArgument and parseRemainingArguments 🎉

However, it ended up removing the constructor symbols and constructor arguments' symbol of the generated class from the Symbols section as @bishabosha mentioned.
What do you think? @tgodzik

Copy link
Member

Choose a reason for hiding this comment

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

@dos65 also what do you think of the removed occurrences for the @main boilerplate - the class symbol still remains

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this synthetic is a good thing.

What about class symbols, leaving them isn't a problem. Anyway, as I see there is no occurrences that refers to these symbols, so that's ok.

@tanishiking tanishiking force-pushed the synthetic-implicit-conversion branch 3 times, most recently from 14e9be9 to edd392e Compare August 27, 2021 16:11
@bishabosha
Copy link
Member

perhaps a rebase is needed?

Support following synthetics
- implicit and context parameters application
- Anonymous context param name
- Invented given

Also convert TypeVar to stripped type, which may appear in synthetics
We have a discussion how to express the invented variable names in
SemanticDB.
We'll discuss the design of the tree in the issue and come back in the
future.
By checking if the template's owner module symbol has an Invisible flag.
@tanishiking tanishiking force-pushed the synthetic-implicit-conversion branch from edd392e to a4f1400 Compare August 27, 2021 16:55
[32:35..32:49):Array.empty[T] => *(evidence$1)
[36:22..36:27):new F => orderingToOrdered[F](*)
[36:22..36:27):new F => *(ordering)
[51:24..51:30):foo(0) => *(x$1)
Copy link
Member

@bishabosha bishabosha Aug 27, 2021

Choose a reason for hiding this comment

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

im thinking this should be more like the following (with a new tree kind, or a using flag on ApplyTree?), otherwise applying the substitution here would not compile

Suggested change
[51:24..51:30):foo(0) => *(x$1)
[51:24..51:30):foo(0) => *(using x$1)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be synthetics consumer's responsibility (e.g. when metals print context-param application, it should print using x$1 instead of x$1), and they can check the x$1 is given instance by looking up their symbol information.
↑ Nevermind, we can't necessarily look up the symbol information, for example, if the given instance is imported by wildcard import...
I agree. we should add a new tree to SemanticDB so we can tell the difference between application and context-param application. (But adding a tree just for context-param application is too scala3 specific? 🤔 )

Copy link
Member

@bishabosha bishabosha Aug 27, 2021

Choose a reason for hiding this comment

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

I'm not so sure how it works for insertion of multiple implicit parameters, but i guess those are single argument applications also, so can't be applied together (making the using distinction maybe not a huge deal)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can go with this at this moment, and discuss in separate issue (in scalameta/scalameta?)
It's definitely good to have, but as far as I know, there're no scalameta based tools to use synthetics to insert the text directly. (metals just shows the implicit / context arguments).

@tanishiking
Copy link
Member Author

scalameta/scalameta#2458 created an issue in scalameta/scalameta about missing identifier.

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

LGTM

@tanishiking
Copy link
Member Author

Thank you for the review!
I created a follow-up issue on scalameta/scalameta#2460
(and scalameta/scalameta#2458)

@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants