-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Signature information in Semanticdb #12885
Conversation
@bishabosha @tgodzik Could you take a look at the questions? |
compiler/src/dotty/tools/dotc/semanticdb/SymbolInformationOps.scala
Outdated
Show resolved
Hide resolved
In this case we do not add dependencies to Scala compiler repos unless they are Java interfaces, so we reimplement printing in the |
I was thinking about installing But I got second though it's still quite messy and we shouldn't do that, so never mind :) |
a46e4c7
to
9c6015e
Compare
@bishabosha @tgodzik |
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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,.
The previous approach didn't work (couldn't resolve the We might resolve the symbol of the
We could apply this approach (traverse children first and construct symbol table) to |
@bishabosha @tgodzik |
Hey, I intend to take a look tomorrow |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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#`++`().*/) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
)
There was a problem hiding this 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!
compiler/src/dotty/tools/dotc/semanticdb/SemanticSymbolBuilder.scala
Outdated
Show resolved
Hide resolved
sb.append(info.displayName).nl | ||
end processSymbol | ||
private def processSymbol(info: SymbolInformation, printer: SymbolInfomationPrinter)(using sb: StringBuilder): Unit = | ||
sb.append(printer.pprintSymbolInformation(info)).nl |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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
compiler/src/dotty/tools/dotc/semanticdb/SemanticSymbolBuilder.scala
Outdated
Show resolved
Hide resolved
@@ -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#`++`().*/) |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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()
}
There was a problem hiding this comment.
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] => |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 _ => () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe missing PolyType
here?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
Because there could be an unexpected types for incompilable sources and if we show the warning for uncompilable sources, it could be confusing.
f51da32
to
056f4a4
Compare
Rebased and then removed the merge commits 👍 |
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/12885/ to see the changes. Benchmarks is based on merging with master (38b983c) |
Reset local symbol index for each TextDocument, updateExpect PrettyPrint signature and update metac.expect Integrate SymbolInformationOps into Scala3#SymbolOps Model WithType for AndType scala#12885 (comment) Resolve ParamRef by constructing SymbolTable To construct symbol table before referring the ParamRef it's required to traverse the child trees before registering the symbol. Lookup symbols for refinements in RefinedType and RecType Re-insert assertion to localIdx Previously, there were the risks that localIdx receives the symbol that doens't have `span` because `TypeOps` generates dummy symbols for refinements in `RefinedType` and `RecType`. However, with the following commits, scala@7dd91ad scala@226e26f now we never generate dummy symbol in `TypeOps`. So re-insert the assertion we deleted when copying this method from `ExtractSemanticDB`. see: https://github.com/lampepfl/dotty/pull/12885/files#r658150249 Convert AndType to IntersectionType Refactor enterRefined Refactor: move all extension (of Symbol) method into Scala3 Don't add type parameters as parameters to MethodSignature Refactor not to use isInstanceOf for designator of TypeRef and TermRef Use intersection type for the parent of RefinedType Convert wildcard type as ExistentialType For wildcard type `C[_ <: T]`, it's internal type representation will be `AppliedType(TypeBounds(lo = <Nothing>, hi = <T>))`. As scalameta for Scala2 does, we'll convert the wildcard type to `ExistentialType(TypeRef(NoPrefix, C, <local0>), Scope(hardlinks = List(<local0>)))` where `<local0>` has - display_name: "_" and, - signature: type_signature(..., lo = <Nothing>, hi = <T>) See: https://github.com/lampepfl/dotty/pull/12885/files#r663797616 https://scalameta.org/docs/semanticdb/specification.html#type-2 Now, when we compile the following Scala program to semanticdb ```scala class Wildcards { def e1: List[_ <: Int] = ??? } ``` The semanticdb's SymbolInformation for `e1` looks like: From scala3 ``` symbols { symbol: "advanced/Wildcards#e1()." kind: METHOD display_name: "e1" language: SCALA signature { value_signature { tpe { by_name_type { tpe { existential_type { tpe { type_ref { symbol: "scala/collection/immutable/List#" type_arguments { type_ref { symbol: "local0" } } } } declarations { hardlinks { symbol: "local0" kind: TYPE display_name: "_" language: SCALA signature { type_signature { type_parameters { } lower_bound { type_ref { symbol: "scala/Nothing#" } } upper_bound { type_ref { symbol: "scala/Int#" } } } } } } } } } } } } } ``` On the other hand, generated from scalameta's metac with scalac 2.13.6 ```sh $ metac --version Scala compiler version 2.13.6 -- Copyright 2002-2021, LAMP/EPFL and Lightbend, Inc. ``` ``` symbols { symbol: "advanced/Wildcards#e()." kind: METHOD display_name: "e" language: SCALA signature { method_signature { type_parameters { } return_type { existential_type { tpe { type_ref { symbol: "scala/package.List#" type_arguments { type_ref { symbol: "local0" } } } } declarations { hardlinks { symbol: "local0" kind: TYPE properties: 4 display_name: "_" language: SCALA signature { type_signature { type_parameters { } lower_bound { type_ref { symbol: "scala/Nothing#" } } upper_bound { type_ref { symbol: "scala/Int#" } } } } access { public_access { } } } } } } } } access { public_access { } } } ``` Use finalResultType for MethodSignature to get actual return type. Register the symbol of nested method with "actual" binder For example: `def foo(x: T)(y: T): T` and for `<y>.owner.info` would be like `MethodType(...<x>, resType = MethodType(...<y>, resType = <T>))`. (Let's say the outer `MethodType` "outer", and `MethodType` who is `resType` of outer "inner") Before this commit, we register <y> to the symbol table with `(<y>, outer)`, which should be `(<y>, inner)`. For such a nested method signature, we have to find the "actual" binder for parameters and register them to the symbol table. Remove unused parameter sym from toSemanticType Fix pprint issue for SingleType When the prefix of SingleType is Type.Emtpy, we had been printing them as `<?>.sym.type`, but it should be `sym.type` Convert symbols to local index for funParamSymbol Refactor style of if Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com> Fix printing upper type bounds Add evidence params to SemanticDB Add refinements to RefinedType generated by opaque types Fix empty refinement for RefinedType for the result of polymorphic method Print definition of ExistentialType instead of displaySymbol Update comment Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com> None for empty decls Warn if symbol lookup failed for paramRef and RefinedType Workaround symbol not found for higher kinded type param Suppress warning for known issue of exporting Polymorphic type Support multiple type param clauses for extension method By combining two type param clause into one Exclude synthetic symbols from occurrences Fallback to Type.member for refinements who can't access to the tree For example, ```scala trait SpecialRefinement { def pickOne(as: String*): Option[Any] } class PickOneRefinement_1[S <: SpecialRefinement { def pickOne(as: String*): Option[String] }] { def run(s: S, as: String*): Option[String] = s.pickOne(as:_*) } ``` In the typed AST, the SpecialRefinement is inside of the TypeTree and cannot access to the refinement (pickOne) from tree, therefore we can't register the symbol of pickOne to symbol table. In this case, fallback to `Type.member` to find the symbol of `pickOne`. Fallback to Type.member for ParamRef Workaround symbol not found for HKTypeLambda in upper bounds and fix type for curried applied type Remove unnecessary comment Do not create newSymbol for wildcard type symbol instead, create a proxy data that will be converted to semanticdb symbol who represents wildcard symbol Warn if unexpected type appear while converting type to SemanticDB type Dealias LazyRef while converting types to SemanticDB Do not emit warning for unexpected type Because there could be an unexpected types for incompilable sources and if we show the warning for uncompilable sources, it could be confusing. Do not create a newSymbol for ParamRef Make wildcard type symbol global Hardlink fake symbols Excude symbols that doesn't present in source from occurrence Make namePresenInSource safe for IndexOutOfRange Make package objects present in source Fallback to fake symbol for refinements in RefinedType of upper bound see: scala#12885 (comment) scala#12885 (comment) Add occurrences of secondary constructors Add fakeSymbols to symbol section and don't hardlink fake syms Convert tp <:< FromJavaObject to tp <:< Any Do not add wildcard symbol of existential type to symbol section Refactor styles
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)
F[T][U]
toF[T, U]
(need extending SemanticDB).TypeTree
TODO (in this PR)
Overview
MatchType
andTypeLambda
MatchType
andTypeLambda
(won't support in this PR)Solved Questions
Can we retrieve declarations' symbols from RefinedType?
I'm trying to model
RefinedType
in Scala3 byStructuralType
in Semanticdb. AsStructuralType
's declarations are modeled as Scope, I try to retrieve all the declarations' symbols ofRefinedType
.For example,
{ def x: Int; def y: Int }
indef m (x: {def x: Int; def y: Int})
, we need the symbols fordef x
anddef 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 }
indef m (x: {def x: Int; def y: Int})
has the following type:RefinedType
?tpe.member(tpe.refinedName)
didn't work).RefinedType
)?Reconstruct a symbol from
TypeParamRef
andTermParamRef
Trying to model
TypeParamRef
asTypeRef
, for example:Z
in the above case) fromTypeParamRef
?sym.rawParamss
andTypeParamRef#paramNum
Use metap?
Signature
's pretty-printer in Scala3.metap
for updating themetac.expect
?