Skip to content

Signature information in Semanticdb #12885

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

Merged
merged 53 commits into from
Aug 12, 2021

Conversation

tanishiking
Copy link
Member

@tanishiking tanishiking commented Jun 21, 2021

Related: #12766

This PR enables Scala3 to generate Signature information in Semanticdb. I have some questions for model some Scala3 types into Semanticdb types:

Future work? (in other PR)

TODO (in this PR)


Overview

  • Generate Signature information for Types except MatchType and TypeLambda
    • There might be some bugs though
  • Add pretty-printer for signature and update metac.expect
    • Still, it's crappy... (might be better to just copy-and-paste from metap?)
  • Add test for Scala3 types
  • Support MatchType and TypeLambda (won't support in this PR)

Solved Questions

Can we retrieve declarations' symbols from RefinedType?

I'm trying to model RefinedType in Scala3 by StructuralType in Semanticdb. As StructuralType's declarations are modeled as Scope, I try to retrieve all the declarations' symbols of RefinedType.
For example, { def x: Int; def y: Int } in def m (x: {def x: Int; def y: Int}), we need the symbols for def x and def y.

In Scala2, RefinedTypes has their declarations as Scope directly, and therefore scalameta can convert them to Semanticdb Scope.

However, in Scala3, the information RefinedType have is only (declarations') type and names, and it doesn't have its declarations' symbols.

For example,
{ def x: Int; def y: Int } in def m (x: {def x: Int; def y: Int}) has the following type:

RefinedType(
  parent = RefinedType(
    parent = TypeRef(..., Object)
    ...
  ),
  refinedName = x,
  refinedInfo = TypeRef(..., Int)
)
  • Can we retrieve those symbols, from RefinedType?
    • (tpe.member(tpe.refinedName) didn't work).
  • If it's impossible to reconstruct those symbols from RefinedType
    • Is there a way to retrieve symbols for declarations, from the symbol (whose type is RefinedType)?
    • If it's difficult, maybe we can retrieve those symbols from AST.

Reconstruct a symbol from TypeParamRef and TermParamRef

Trying to model TypeParamRef as TypeRef, for example:

def m[T]: T = ???

// `<def m>.info` is
PolyType(
    List(Z),
    List(TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Nothing),TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Any))),
    TypeParamRef(Z)
)

// this should be modeled as
MethodSignature(
   ...
   returnType = TypeRef(..., Z)
)
  • Is there a way to retrieve the symbol (of Z in the above case) from TypeParamRef?
  • If not, I consider retrieving the symbol by sym.rawParamss and TypeParamRef#paramNum

Use metap?

  • To pretty-print the generated signature information in metac.expect which is printed by Tools#metac, we have to implement a Signature's pretty-printer in Scala3.
  • However, the pretty-printer is already implemented in scalameta's metap
  • Should we duplicate the pretty-printer in Scala3 for the testing? Or, another options: use metap for updating the metac.expect ?

@tanishiking
Copy link
Member Author

@bishabosha @tgodzik Could you take a look at the questions?

@bishabosha
Copy link
Member

However, the pretty-printer is already implemented in scalameta's metap
Should we duplicate the pretty-printer in Scala3 for the testing? Or, another options: use metap for updating the metac.expect ?

In this case we do not add dependencies to Scala compiler repos unless they are Java interfaces, so we reimplement printing in the Tools object, this can be extended for the symbol information

@tanishiking
Copy link
Member Author

tanishiking commented Jun 21, 2021

In this case we do not add dependencies to Scala compiler repos unless they are Java interfaces

I was thinking about installing metap CLI and somehow invoke it from SemanticDBTest, instead of adding a dependency to scalameta.

But I got second though it's still quite messy and we shouldn't do that, so never mind :)

@tanishiking tanishiking force-pushed the semanticdb-signature branch from a46e4c7 to 9c6015e Compare June 24, 2021 14:34
tanishiking added a commit to tanishiking/scala3 that referenced this pull request Jun 24, 2021
@tanishiking tanishiking changed the title [WIP] Signature information in Semanticdb Signature information in Semanticdb Jun 24, 2021
@tanishiking tanishiking marked this pull request as ready for review June 24, 2021 18:39
@tanishiking
Copy link
Member Author

@bishabosha @tgodzik
Could you take a look? (sorry it's gigantic...)
I think there's a lot of room for improvement, but I wanna make sure we're on the right track.

@bishabosha bishabosha self-requested a review June 25, 2021 13:21
@bishabosha bishabosha self-assigned this Jun 25, 2021
case DoubleTag => s.DoubleConstant(const.doubleValue)
case StringTag => s.StringConstant(const.stringValue)
case NullTag => s.NullConstant()
// ConstantType(_: Type, ClazzTag) should be converted as it's type
Copy link
Contributor

Choose a reason for hiding this comment

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

Let're move these comments if they are not needed.

Copy link
Member

Choose a reason for hiding this comment

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

still could remove these comments

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Great work! I think we need to figure out the issues with missing symbols for type refinements and also whether to actually copy the printer. Other than that, it all look ok and it's impossible to be sure about the tests 100%, since we got so many of those, but on first glance it looks correct.

We can always fix things later on and any changes will be much clearer now,.

@tanishiking
Copy link
Member Author

The previous approach didn't work (couldn't resolve the TypeParamRef and TermParamRef) for the types something like type X[T] = List[T] (which is equivalent with type X = [T] =>> List[T], because in this case the sym.rawparamss and sym.paramSymss are empty.

We might resolve the symbol of the TypeParamRef and TermParamRef by

  • traverse the children tree first, create symbol table Map[(LambdaType, Name), Symbol]
  • then the signatures that contain TypeParamRef and ParamRef, we can resolve those symbols by looking up the symbol table.

We could apply this approach (traverse children first and construct symbol table) to RefinedType and RecTypes.

@tanishiking
Copy link
Member Author

@bishabosha @tgodzik
Hey, I think I successfully filled the symbols for RefinedType (and did some bugfixes for LambdaTypes) 🎉 Could you take a look?
I wrote a small piece of explanation on #12885 (comment) but I think it's complicated, and if you need more information, let me know :)

@bishabosha
Copy link
Member

@bishabosha @tgodzik
Hey, I think I successfully filled the symbols for RefinedType (and did some bugfixes for LambdaTypes) 🎉 Could you take a look?
I wrote a small piece of explanation on #12885 (comment) but I think it's complicated, and if you need more information, let me know :)

Hey, I intend to take a look tomorrow

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Great work! I need to still look a bit more into TyeOps, but it looks pretty amazing!

else if sym.isTerm && sym.owner.isTerm then
SymbolInformation.Kind.LOCAL
else if sym.isInlineMethod || sym.is(Macro) then
SymbolInformation.Kind.MACRO
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have an additional kind for inline ? We might need to update the modifiers in semanticdb. Also, should we recognize inline methods as macros? @bishabosha what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it could be good to have a new Inline kind, and possibly TransparentInline TransparentMacro (due to the different semantics) but maybe they should be properties?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tanishiking maybe let's add this together with TypeLambda and MatchType PR?

@@ -21,5 +21,5 @@ object Nats/*<-recursion::Nats.*/ {
case Succ/*->recursion::Nats.Succ.*//*->recursion::Nats.Succ.unapply().*//*->local2*/(p/*<-local3*/) => toIntg/*->recursion::Nats.toIntg().*/(p/*->local3*/) +/*->scala::Int#`+`(+4).*/ 1
}

val j31/*<-recursion::Nats.j31.*/ = toIntg/*->recursion::Nats.toIntg().*/(Zero/*->recursion::Nats.Zero.*/.++.++.++/*<-local4*//*->recursion::Nats.Zero.*//*->recursion::Nats.Nat#`++`().*/ + /*<-local5*//*->recursion::Nats.Nat#`++`().*/Zer/*<-local6*//*->recursion::Nats.Nat#`++`().*//*->recursion::Nats.Nat#`+`().*/o/*->recursion::Nats.Zero.*/.++/*->recursion::Nats.Nat#`++`().*/)
val j31/*<-recursion::Nats.j31.*/ = toIntg/*->recursion::Nats.toIntg().*/(Zero/*->recursion::Nats.Zero.*/./*->recursion::Nats.Zero.*/++.++.++/*<-local4*//*->recursion::Nats.Nat#`++`().*/ + /*<-local5*//*->recursion::Nats.Nat#`++`().*/Zer/*<-local6*//*->recursion::Nats.Nat#`++`().*//*->recursion::Nats.Nat#`+`().*/o/*->recursion::Nats.Zero.*/.++/*->recursion::Nats.Nat#`++`().*/)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks weird, we get double occurrence of Zero here. Though, it's probably unrelated, but we might want to raise an issue for the future.

Copy link
Member

Choose a reason for hiding this comment

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

this looks to be because of the inlining, if I change from transparent inline to inline, then there is no repetition (also need to add "-Ystop-after:extractSemanticDB" to the compiler args in SemanticdbTests)

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.

I still have more to look over, thank you for your efforts so far!

sb.append(info.displayName).nl
end processSymbol
private def processSymbol(info: SymbolInformation, printer: SymbolInfomationPrinter)(using sb: StringBuilder): Unit =
sb.append(printer.pprintSymbolInformation(info)).nl
Copy link
Member

Choose a reason for hiding this comment

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

we can pass in sb to pprintSymbolInformation here for more efficiency

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we much priority on string concatenation efficiency?

  • I personally prefer returning String instead of passing StringBuilder (because of its code readability).
  • While I understand StringBuilder concatenation is generally faster, this pretty printer is used only for testing and the efficiency might be not a big deal?
  • btw, what do you think about this approach? Signature information in Semanticdb #12885 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Its true the efficiency is not the priority before the correctness of the actual output, so this would only be the last thing to address.

I have addressed that other comment now.

val notes = InfoNotes()
val infoPrinter = InfoPrinter(notes)

def pprintSymbolInformation(info: SymbolInformation): String =
Copy link
Member

Choose a reason for hiding this comment

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

we should try to reuse the current StringBuilder by passing it in as an argument, then either return the StringBuilder or Unit

Copy link
Contributor

Choose a reason for hiding this comment

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

@tanishiking what about this comment? We could reuse the sam StringBuilder everywhere.

class InfoPrinter(notes: InfoNotes) {
private enum SymbolStyle:
case Reference, Definition
def pprint(info: SymbolInformation): String =
Copy link
Member

Choose a reason for hiding this comment

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

again would be good to try and reuse StringBuilder where possible

@@ -21,5 +21,5 @@ object Nats/*<-recursion::Nats.*/ {
case Succ/*->recursion::Nats.Succ.*//*->recursion::Nats.Succ.unapply().*//*->local2*/(p/*<-local3*/) => toIntg/*->recursion::Nats.toIntg().*/(p/*->local3*/) +/*->scala::Int#`+`(+4).*/ 1
}

val j31/*<-recursion::Nats.j31.*/ = toIntg/*->recursion::Nats.toIntg().*/(Zero/*->recursion::Nats.Zero.*/.++.++.++/*<-local4*//*->recursion::Nats.Zero.*//*->recursion::Nats.Nat#`++`().*/ + /*<-local5*//*->recursion::Nats.Nat#`++`().*/Zer/*<-local6*//*->recursion::Nats.Nat#`++`().*//*->recursion::Nats.Nat#`+`().*/o/*->recursion::Nats.Zero.*/.++/*->recursion::Nats.Nat#`++`().*/)
val j31/*<-recursion::Nats.j31.*/ = toIntg/*->recursion::Nats.toIntg().*/(Zero/*->recursion::Nats.Zero.*/./*->recursion::Nats.Zero.*/++.++.++/*<-local4*//*->recursion::Nats.Nat#`++`().*/ + /*<-local5*//*->recursion::Nats.Nat#`++`().*/Zer/*<-local6*//*->recursion::Nats.Nat#`++`().*//*->recursion::Nats.Nat#`+`().*/o/*->recursion::Nats.Zero.*/.++/*->recursion::Nats.Nat#`++`().*/)
Copy link
Member

Choose a reason for hiding this comment

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

this looks to be because of the inlining, if I change from transparent inline to inline, then there is no repetition (also need to add "-Ystop-after:extractSemanticDB" to the compiler args in SemanticdbTests)

val sargs = args.map(loop)
loop(tycon) match
case ref: s.TypeRef => ref.copy(typeArguments = sargs)
// is there any other cases?
Copy link
Member

Choose a reason for hiding this comment

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

We can have curried type application with type lambdas so we need to support that, instead of just replacing the previously applied type arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean something like def x[T](a: T): ([X] =>> List[X])[T] = ??? ?
For type lambdas, it looks in this phase, what we will have is just an already applied type: e.g. List[T] in the above example. There might be another type than TypeRef here, but I couldn't find the example...

Copy link
Member

@bishabosha bishabosha Jul 7, 2021

Choose a reason for hiding this comment

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

here is an example I use to test the Scala 2 TASTy unpickler:

class HKClass[F <: [T] =>> [U] =>> (U, T)] {
  def foo[T,U](x: F[T][U]): String = x.toString()
}

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed 26fc7ca


case ann: AnnotatedType if ann.annot.symbol.info.isInstanceOf[ClassInfo] =>
def flatten(tpe: Type, annots: List[Annotation]): (Type, List[Annotation]) = tpe match
case AnnotatedType(parent, annot) if annot.symbol.info.isInstanceOf[ClassInfo] =>
Copy link
Member

Choose a reason for hiding this comment

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

might want to be careful of type aliases used as annotations here, which won't have ClassInfo as their info, maybe annot.symbol takes care of this

Copy link
Member Author

Choose a reason for hiding this comment

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

Judging from the Annotations.symbol https://github.com/lampepfl/dotty/blob/601184787508d8fd655f1dc4e48ed19c418fa710/compiler/src/dotty/tools/dotc/core/Annotations.scala#L13-L15 I guess we can expect there should only be ClassInfo.
Since I tried but couldn't find a counter-example for this, maybe we can go with it, and fix it in the future when we find a example here.

Copy link
Member

@bishabosha bishabosha Jul 7, 2021

Choose a reason for hiding this comment

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

I confirmed with testing it does not matter if the type is aliased, however it does not work for nested annotated types, e.g.

// tests/semanticdb/expect/InstrumentTyper.scala
type paramAlias = param
type paramRec = param @param
type AnnotatedType = Int @param
type AnnotatedType2 = Int @paramAlias
type AnnotatedType3 = Int @(param @param)
type AnnotatedType4 = Int @paramRec

In these cases AnnotatedType<3|4> info is Int @param but scala 2 would have Int @param @param

Copy link
Member

Choose a reason for hiding this comment

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

I believe this still needs resolving

enterRefined(expr.resType)
case m: MethodType =>
enterRefined(m.resType)
case _ => ()
Copy link
Member

Choose a reason for hiding this comment

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

maybe missing PolyType here?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I know, PolyType shouldn't appear here as long as they're handled by toSemanticSig (at least I confirmed there're no appearance in semanticdb/expect) (and actually I'm not sure how to convert PolyType into SemanticDB types, if there exists).

Copy link
Member

@bishabosha bishabosha Jul 7, 2021

Choose a reason for hiding this comment

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

see my comment here #12885 (comment)
Edit: it does not seem to help by adding PolyType here, but still something is up

@tanishiking tanishiking force-pushed the semanticdb-signature branch from f51da32 to 056f4a4 Compare August 10, 2021 19:09
@tanishiking
Copy link
Member Author

Rebased and then removed the merge commits 👍

@bishabosha
Copy link
Member

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/12885/ to see the changes.

Benchmarks is based on merging with master (38b983c)

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.

5 participants