-
Notifications
You must be signed in to change notification settings - Fork 226
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
TextDocumentOps: capture lambdas for SAM #4112
Conversation
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.
Thanks for working on this! I think we need to change it a bit, do let me know if you need any help with it!
tests-semanticdb/src/test/resources/example/SingleAbstractMethod.scala
Outdated
Show resolved
Hide resolved
...db/scalac/library/src/main/scala/scala/meta/internal/semanticdb/scalac/TextDocumentOps.scala
Outdated
Show resolved
Hide resolved
...db/scalac/library/src/main/scala/scala/meta/internal/semanticdb/scalac/TextDocumentOps.scala
Outdated
Show resolved
Hide resolved
...db/scalac/library/src/main/scala/scala/meta/internal/semanticdb/scalac/TextDocumentOps.scala
Outdated
Show resolved
Hide resolved
tests-semanticdb/src/test/resources/example/SingleAbstractMethod.scala
Outdated
Show resolved
Hide resolved
@tgodzik updated the patch, it's now very simple. also tried adding symbols, as a test, got some |
tests-semanticdb/src/test/resources/example/SingleAbstractMethod.scala
Outdated
Show resolved
Hide resolved
tests-semanticdb/src/test/resources/example/SingleAbstractMethod.scala
Outdated
Show resolved
Hide resolved
...db/scalac/library/src/main/scala/scala/meta/internal/semanticdb/scalac/TextDocumentOps.scala
Outdated
Show resolved
Hide resolved
Looks like 2.11.12 doesn't have this, we can create a trait that gets the attachment type and for 2.11 does only return the type of the tree |
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.
It seemed to work when testing today. I can take a further look tomorrow
SAM => example/SingleAbstractMethod#SAM# | ||
local0 => val local $anonfun: $anon | ||
$anon => local1 | ||
local1 => class $anon extends Object with SAM { +1 decls } |
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 all three local that are shown in the test above should have the same definition. And local1 doesn't exist in occurrences
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 add
local1
to occurrences? - why should they have the same definition? i mean, they derive from the same synthetic type (local1) but they are different functions.
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 add
local1
to occurrences?
specifically: local1
doesn't have a fixed position, so where exactly should it be defined? I can't "put" it with the definition of SAM
trait since that trait could possibly come from another file.
another option is this: instead of gsym.setInfo(sam.tpe)
and saving the sam
symbol, i can use gsym.setInfo(sam.info)
and not save any additional symbols, it will look like this:
local0 => val local $anonfun: extends Object with SAM { +1 decls }
(two spaces after :
and no $anon
name for the synthetic class). that way, no position and no local for the synthetic class, and you get your extends Object with SAM
(not AnyRef
but Object
though it's the same).
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.
Currently, local0 etc. point to functions which actually get replaced later on with SAM type, so I don't think we need to save them at all into semanticdb. Just save the anonymous class with occurrence at the function position
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 am sorry, i am not sure i understand it. could you take over the commit and adjust it yourself?
here's how i interpret what we have:
// this is the position range which defines `local0` which is...
[9:10..9:15): _ + 1 <= local0
// ... an instance of type `$anon` also known ...
local0 => val local $anonfun: $anon
$anon => local1
// ... as anonymous class deriving from SAM
local1 => class $anon extends Object with SAM { +1 decls }
Object => java/lang/Object#
SAM => example/SingleAbstractMethod#SAM#
so local0
doesn't point to a function but to a instance of a SAM type.
but i am not an expert in semanticdb, as you know, just very superficial, so if you know what you'd like to see there, to be able to use within metals, then please help me :)
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.
so going back to yesterday's state: if they won't exist in the generated code, and if you don't prefer the more verbose current solution, which is -- importantly -- not too hacky, why can't local0 simply have the type SAM
?
also, local1 is not a type, it's a label we assigned to something, so I'd volunteer a guess that, at best, it's a matter of how this is printed out.
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.
so going back to yesterday's state: if they won't exist in the generated code, and if you don't prefer the more verbose current solution, which is -- importantly -- not too hacky, why can't local0 simply have the type SAM?
The type of it is never really just sam. First it's a function then it's transformed by the compiler into the anonymous class. It would also not work currently with metals, where it's primarily used as we don't show references to type if that type doesn't exist in source code. We can only make it work with go to implementation, which this effectively is.
also, local1 is not a type, it's a label we assigned to something, so I'd volunteer a guess that, at best, it's a matter of how this is printed out.
sure that might not matter actually that much, but we need it in occurrences to actually show it somewhere. We can't use local0 since there is no relation between the two.
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.
also, local1 is not a type, it's a label we assigned to something, so I'd volunteer a guess that, at best, it's a matter of how this is printed out.
sure that might not matter actually that much, but we need it in occurrences to actually show it somewhere. We can't use local0 since there is no relation between the two.
ah, if the problem is showing local1
in occurrences, that should be fairly easy, i 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 have a fix ready, will open a separate PR in a bit.
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.
Ok, so this is what will be most of use to in metals df4b7b0
Though, the 2.13 expect hasn't updated, so maybe something is still off
do we need it for 2.11? if it doesn't have it, then is it possible that code using SAM would even exist? |
I think we don't need it for 2.11.12 yeah |
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.
Thanks!
Fixes #3394.