-
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
Support MatchType for SemanticDB #14608
Conversation
Generated from tanishiking/semanticdb-for-scala3#1 See the discussion here scalameta/scalameta#2414
to merge this we would have to wait for scalameta/scalameta#2414 to be approved? |
Actually, scalameta/scalameta#2414 has already been approved and was waiting for the Scala3 implementation (to see the protobuf schema really works for Scala3). Anyway, I'll work on #14608 (comment) as they should be typed as you mentioned. |
maybe unrelated but seeing as this will only be released for 3.2.0 or beyond, it seems a shame to not be able to support the same semanticdb for older scala 3 releases - what can we do about this? @tgodzik |
I can't really think of anything :/ We would need to split it out to a plugin again like with Scala 2. |
I had a an idea that maybe there could be backports of semanticdb changes to the old release branches of previous minor versions 🤣 |
😨 |
else if tree.rhs.isEmpty then | ||
symkinds += SymbolKind.Abstract | ||
// if symbol isType, it's type variable | ||
case tree: Bind if (!tree.symbol.isType) => |
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.
Added !tree.symbol.isType
here, to misinterpret the type variable symbol as Val
This change makes type variables like local0 => case val method N$1 <: Nat
to local0 => case type N$1 <: Nat
.
Other diffs: just indented.
// prefix symbol (e.g. "scala/") + name (e.g. "Nothing") + Global symbol suffix ("#") | ||
// see: https://scalameta.org/docs/semanticdb/specification.html#symbol | ||
val ssym = s"${pre.typeSymbol.symbolName}${sym.mangledString}#" | ||
s.TypeRef(s.Type.Empty, ssym, Seq.empty) |
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.
x *: xs
part of
type Concat[Xs <: Tuple, +Ys <: Tuple] <: Tuple = Xs match
case EmptyTuple => Ys
case x *: xs => x *: Concat[xs, Ys]
have TypeRef(..., Nothing)
(where Nothing
is just a Name
instead of Symbol
), and we can't access the symbol for Nothing
here. Therefore, craft the symbol for TyperRef
for this.
Fixed #14608 (comment)
As this feature is kinda optional, I believe we don't need to backport to the older minor versions :) |
Now it's ready for review :) |
`t` of `case List[t] =>` has `CaseClass` flag. Maybe we should remove `CaseClass` flag from `t`, but as a workaround remove `Case` property from semanticdb from type variables.
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.
nice! thank you
Can you wait for merging, until scalameta merges the PR to update the schema, just in case :) ? |
ok I have added the |
@bishabosha Thanks! It's now ready for review :) |
tests/semanticdb/metac.expect
Outdated
advanced/Structural#T#[A] => typeparam A | ||
advanced/Structural#T#`<init>`(). => primary ctor <init> [typeparam A ](): T[A] | ||
advanced/Structural#T#bar(). => method bar (param b: <?>): Unit | ||
advanced/Structural#T#bar().(b) => param b: <?> |
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.
b
should still have a type here - it seems a symbol does exist for foo.B
because apparently it is local12
Edit: I have now seen your comment :) #14608 (comment)
The simplest example where I guess this is because refinement actually doesn't have a symbol (we dealt with it by fake symbol). The problem is, unlike |
what happens if you resolve the prefix to the symbol for |
Also see here for how tasty resolves selections of a type from prefix that has a refined type: |
Ok, we can access |
|
||
// see: https://github.com/lampepfl/dotty/pull/14608#discussion_r835642563 | ||
lazy val foo/*<-advanced::Test.foo.*/: (reflect.Selectable/*->scala::reflect::Selectable#*/ { type A/*<-local16*/ = Int/*->scala::Int#*/ }) &/*->scala::`&`#*/ (reflect.Selectable/*->scala::reflect::Selectable#*/ { type A/*<-local17*/ = Int/*->scala::Int#*/; val a/*<-local18*/: A/*->local17*/ }) = ???/*->scala::Predef.`???`().*/ | ||
def bar/*<-advanced::Test.bar().*/: foo/*->advanced::Test.foo.*/.A = foo/*->advanced::Test.foo.*/.a |
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.
do you have an idea why .A
and .a
do not have occurrences 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 guess this is because .A
and .a
(access the field of refinements), don't have symbols (we're crafting fake symbols for them), and ExtractSemanticDB
doesn't resolve the fake symbols by name.
We can see the same problem around here:
https://github.com/lampepfl/dotty/blob/60c336ef34fe0e77851a7652b910db13b84dfe6f/tests/semanticdb/expect/Advanced.expect.scala#L17
I'll take a look 👀
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 guess looking again registerUse
or registerUseGuarded
still only assume real symbols, for example Select
tree checks that the symbol exists, without checking for a registered fake symbol - this could be a follow up PR
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 per foo.A
, b964694 as you mentioned, you're right, we could fix the issue by looking up the fake symbols and registerUse
them.
In terms of foo.a
(field access to structural type), it is transformed to foo.selectDynamic("a")
by Typer here ↓
https://github.com/lampepfl/dotty/blob/b9646942facccdcc110753f019c14b941fae870d/compiler/src/dotty/tools/dotc/typer/Dynamic.scala#L49-L65
We have to check if qual
is something like Apply(Select(Ident(foo), name), List(Literal(Constant("a")))) if name == nme.applyDynamic || name == nme.selectDynamic || name == nme.updateDynamic || name == nme.applyDynamicNamed
, and lookup symbol for a
from foo
(This is not yet fixed, can I work on this in another PR?)
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 this is fine for another PR
b964694
to
be31d56
Compare
Realized, we just failed to register symbols to Let's see tests pass... (some failed in the previous commit) |
looks like the some new warnings are causing tests to have extra errors |
```scala val a: String = (((1: Any): b.A): Nothing): String val b: { type A >: Any <: Nothing } = loop() ``` We can't resolve a symbol of `b.A` in `(1: Any): b.A` because semanticdb generator basically resolves the symbol of refinement members (like `type A` of `b`) by - Traverse trees and collect symbol information by **depth-first** way - during the traversal, we register the symbols of refinement members to a symbol table. - Then outer part of program can resolve the symbol of refinements by looking up symbols from the symbol table. However, in this case, we failed to resolve the symbol of b.A bacause - (1) We try to lookup the symbol of `b.A` first, which has not yet registered to the symtab. - (2) And then register a symbol for A in b by traversing `b`. Maybe we could fix this issue by - (a) Generate fake symbol for `b.A` in (1), and register it to the symtab - (b) in (2), when we register the "real" symbol of `A`, it should collide with the symbol registered in step (a) - (c) if they had collision, we mark those symbols (fake one and real one) as an alias - (d) on building the semanticdb symbol, we use the same symbol for both fake one and real one
I just suppressed the warning in ba24fe1 and added a test case for semanticdb. I found a problem in dealing with the refinement symbols (like i5854.scala). In this file, we fail to look up the symbol of
Maybe we could fix this issue by
|
Thank you for all your rest work! I agree that this is basically ready to merge, would you be able to make an issue to describe the problem with traversal order? then let's merge. |
Sure! I opened an issue here #14828 |
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! thank you
/generated
sources are generated from tanishiking/semanticdb-for-scala3#50See the discussion about protobuf schema here scalameta/scalameta#2414
For example, the following program
is modeled as:
Once this PR is approved I'm going to create a PR that support MatchType extracted from scalameta/scalameta#2414 :)