questions to hyperlink detector

[Simon Schäfer]

I just started working a bit on hyperlink detector. Two things that
would be nice if I can solve them:


Does someone know a way to create a function (String =>
scala.tools.eclipse.javaelements.ScalaSourceFile) where String contains
sources? Currently there exists such a function
(https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core.tests/src/scala/tools/eclipse/testsetup/TestProjectSetup.scala#L68)
but it expects a path to a source file and not the sources directly.

I need this for the test suite because I'm too lazy to create a file for
each simple test.



It looks like the current implementation of the hyperlink detector can't
locate symbols defined in Java
(https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core/src/scala/tools/eclipse/hyperlink/text/detector/ScalaDeclarationHyperlinkComputer.scala#L74).
If I deactivate the check for a Java symbol I do not get any result
back, why is that the case? Is it possible to fix that?


Thanks,
Simon

[iulian dragos]

On Thu, Oct 4, 2012 at 7:07 PM, Simon Schäfer <ma...@antoras.de> wrote:

I just started working a bit on hyperlink detector. Two things that would be nice if I can solve them:


Does someone know a way to create a function (String => scala.tools.eclipse.javaelements.ScalaSourceFile) where String contains sources? Currently there exists such a function (https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core.tests/src/scala/tools/eclipse/testsetup/TestProjectSetup.scala#L68) but it expects a path to a source file and not the sources directly.

I need this for the test suite because I'm too lazy to create a file for each simple test.

You could whip something up based on SDTTestUtils.addFileToProject and changeContentOfFile.

 

It looks like the current implementation of the hyperlink detector can't locate symbols defined in Java (https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core/src/scala/tools/eclipse/hyperlink/text/detector/ScalaDeclarationHyperlinkComputer.scala#L74). If I deactivate the check for a Java symbol I do not get any result back, why is that the case? Is it possible to fix that?

The current implementation of hyperlink detectors needs a bit of love. I think it's too complicated, and we should get rid of a couple of layers of indirection. Other than that, no, we can't resolve links to Java symbols because we don't type-check Java sources. The only thing the Scala compiler can do for Java symbols is to 'resolve' them to a Scala symbol, whose `pos` field is not correctly set. From there, we need the JDT to step in (based on the fully-qualified name).

It would be great if you had a look at it. My suggestion would be to break the current composite pattern, and have separate hyperlink detector classes (and remove the 'strategy' pattern as well), and pass several detectors in the SourceViewerConfiguration class (since you can pass more than one).

Let me know if you'd like to know more about it, I can talk for hours :)

thanks,

iulian

 

Thanks,
Simon

--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

[Simon Schäfer]

On Fri 05 Oct 2012 01:26:27 PM CEST, iulian dragos wrote:
> You could whip something up based on SDTTestUtils.addFileToProject and
> changeContentOfFile.

Ok, I will look at that.

> The current implementation of hyperlink detectors needs a bit of love.
> I think it's too complicated, and we should get rid of a couple of
> layers of indirection. Other than that, no, we can't resolve links to
> Java symbols because we don't type-check Java sources. The only thing
> the Scala compiler can do for Java symbols is to 'resolve' them to a
> Scala symbol, whose `pos` field is not correctly set. From there, we
> need the JDT to step in (based on the fully-qualified name).
>
> It would be great if you had a look at it. My suggestion would be to
> break the current composite pattern, and have separate hyperlink
> detector classes (and remove the 'strategy' pattern as well), and pass
> several detectors in the SourceViewerConfiguration class (since you
> can pass more than one).
>
> Let me know if you'd like to know more about it, I can talk for hours :)

Yes, I would highly appreciate any additional information. I could fix
the ticket about type aliases you commented two days ago, but currently
it works only for Scala symbols (I implemented it in
ScalaDeclarationHyperlinkComputer). Probably I need to use the way as
mentioned in
DeclarationHyperlinkDetector.javaDeclarationHyperlinkComputer. I also
looked at resolving calls to the unapply-/unapplySeq-/update-method or
to syntax sugar like `x = x+y <=> x += y` but either they result in
missing information or in incorrect positions. The only method which
seems currently to work is the apply-method. If you have the time it
would be nice if you can tell me if these things could completely solved
inside of scala-ide (and not in scalac) and where I have to look to in
order to solve these things.

[iulian dragos]

On Fri, Oct 5, 2012 at 3:36 PM, Simon Schäfer <ma...@antoras.de> wrote:

On Fri 05 Oct 2012 01:26:27 PM CEST, iulian dragos wrote:

You could whip something up based on SDTTestUtils.addFileToProject and
changeContentOfFile.

Ok, I will look at that.

The current implementation of hyperlink detectors needs a bit of love.
I think it's too complicated, and we should get rid of a couple of
layers of indirection. Other than that, no, we can't resolve links to
Java symbols because we don't type-check Java sources. The only thing
the Scala compiler can do for Java symbols is to 'resolve' them to a
Scala symbol, whose `pos` field is not correctly set. From there, we
need the JDT to step in (based on the fully-qualified name).

It would be great if you had a look at it. My suggestion would be to
break the current composite pattern, and have separate hyperlink
detector classes (and remove the 'strategy' pattern as well), and pass
several detectors in the SourceViewerConfiguration class (since you
can pass more than one).

Let me know if you'd like to know more about it, I can talk for hours :)

Yes, I would highly appreciate any additional information. I could fix the ticket about type aliases you commented two days ago, but currently it works only for Scala symbols (I implemented it in ScalaDeclarationHyperlinkComputer). Probably I need to use the way as mentioned in DeclarationHyperlinkDetector.javaDeclarationHyperlinkComputer.

Yes, and Java symbols are easy to detect using `sym.isJava` :)

 

I also looked at resolving calls to the unapply-/unapplySeq-/update-method or to syntax sugar like `x = x+y <=> x += y` but either they result in missing information or in incorrect positions. The only method which seems currently to work is the apply-method. If you have the time it would be nice if you can tell me if these things could completely solved inside of scala-ide (and not in scalac) and where I have to look to in order to solve these things.

I think that unapply/unapplySeq should definitely work. Not sure about `update` yet (how do you detect it?) Do you have the code available somewhere, so I could have a look? What exactly happens when you add the symbol to the list of 'resolved' symbols?

Regarding syntactic sugar, I'm not sure how you want it to work. I assume you see 'x += y' and you ask for a hyperlink at the `+=` position? It might help to use the new `Spy` view we added a few days ago, and check if the tree positions are what you'd expect when you place the cursor on the +=.

cheers,

iulian

[Simon Schäfer]

On Sat 06 Oct 2012 04:13:36 PM CEST, iulian dragos wrote:
>
> You could whip something up based on
> SDTTestUtils.addFileToProject and
> changeContentOfFile.
>
> Ok, I will look at that.

Got this to work and it is much easier to use now. ;)

> I think that unapply/unapplySeq should definitely work. Not sure about
> `update` yet (how do you detect it?) Do you have the code available
> somewhere, so I could have a look? What exactly happens when you add
> the symbol to the list of 'resolved' symbols?

I didn't implemented this yet, I only took a look about what
symbols/positions are available. I hoped the compiler resolves all these
methods (as it does for the apply-method) then it would be easy, to
resolve the hyperlinks. But it doesn't (or I have overseen it).

>
> Regarding syntactic sugar, I'm not sure how you want it to work. I
> assume you see 'x += y' and you ask for a hyperlink at the `+=`
> position? It might help to use the new `Spy` view we added a few days
> ago, and check if the tree positions are what you'd expect when you
> place the cursor on the +=.

Yes, that is what I intended, but neither the positions nor the resolved
symbols are correct.

I think I have to beat with the same problems as in semantic
highlighting - it seems that the compiler does not return enough
information to exhaustive solve all my ideas/tests. Is this true or is
it somehow possible to retrieve more information from the compiler (more
types/symbols)?
>
> cheers,
> iulian

[iulian dragos]

On Sat, Oct 6, 2012 at 5:08 PM, Simon Schäfer <ma...@antoras.de> wrote:
>
>
> On Sat 06 Oct 2012 04:13:36 PM CEST, iulian dragos wrote:
>>
>>
>> You could whip something up based on
>> SDTTestUtils.addFileToProject and
>> changeContentOfFile.
>>
>> Ok, I will look at that.
>
> Got this to work and it is much easier to use now. ;)

Excellent, looking forward to the pull request.

>
>> I think that unapply/unapplySeq should definitely work. Not sure about
>> `update` yet (how do you detect it?) Do you have the code available
>> somewhere, so I could have a look? What exactly happens when you add
>> the symbol to the list of 'resolved' symbols?
>
> I didn't implemented this yet, I only took a look about what
> symbols/positions are available. I hoped the compiler resolves all these
> methods (as it does for the apply-method) then it would be easy, to resolve
> the hyperlinks. But it doesn't (or I have overseen it).

I'm not sure what you mean. Here's the relevant part of the code:

https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core/src/scala/tools/eclipse/hyperlink/text/detector/ScalaDeclarationHyperlinkComputer.scala#L67

As you can see, for 'apply' we extract 2 symbols from the tree: one
for the qualifier (meaning the definition under the cursor), and
another for the `apply` method itself. If you do the same for other
standard names, like `unapply` and `unapplySeq`, the rest of the
machinery should create the hyperlinks for those symbols.

>> Regarding syntactic sugar, I'm not sure how you want it to work. I
>> assume you see 'x += y' and you ask for a hyperlink at the `+=`
>> position? It might help to use the new `Spy` view we added a few days
>> ago, and check if the tree positions are what you'd expect when you
>> place the cursor on the +=.
>
> Yes, that is what I intended, but neither the positions nor the resolved
> symbols are correct.
>
> I think I have to beat with the same problems as in semantic highlighting -
> it seems that the compiler does not return enough information to exhaustive
> solve all my ideas/tests. Is this true or is it somehow possible to retrieve
> more information from the compiler (more types/symbols)?

Unfortunately, I don't fully understand what you are doing in this
case, and in what way it fails. My gut feeling is that the compiler
provides all the required information, so it would help to have some
code to discuss. Maybe there's a simple tweak to make it work ;-)

iulian

>>
>>
>> cheers,
>> iulian

[Simon Schäfer]

On 10/06/2012 10:46 PM, iulian dragos wrote:

I'm not sure what you mean. Here's the relevant part of the code:

https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core/src/scala/tools/eclipse/hyperlink/text/detector/ScalaDeclarationHyperlinkComputer.scala#L67

As you can see, for 'apply' we extract 2 symbols from the tree: one
for the qualifier (meaning the definition under the cursor), and
another for the `apply` method itself. If you do the same for other
standard names, like `unapply` and `unapplySeq`, the rest of the
machinery should create the hyperlinks for those symbols.

Yes this works for `apply` but not for `unapply` or `unapplySeq`. This can be tested on `val Some(x) = Some(0)`. ScalaSpy shows on RHS

Tree:         Select
[...]
tree.tpe:     [A](x: A)Some[A]

symbol:         method apply
symbol.info:     [A](x: A)Some[A]

but on LHS

Tree:         Select
[...]
tree.tpe:     Some.type

symbol:         object Some
symbol.info:     Some.type

The `symbol` is not correct any more.

Unfortunately, I don't fully understand what you are doing in this
case, and in what way it fails. My gut feeling is that the compiler
provides all the required information, so it would help to have some
code to discuss. Maybe there's a simple tweak to make it work ;-)

Example:

class C {
  def +(i: Int) = this
}
object X {
  var c = new C
  c = c + 0
  c += 0
}

ScalaSpy on the `+` call:

Tree:         Select
tree.pos:     RangePosition(/home/antoras/dev/runtime-EclipseApplicationwithEquinoxWeaving/Test/src/Test.scala, 68, 70, 71)
tree.tpe:     (i: Int)C

symbol:         method +
symbol.info:     (i: Int)C

Both `symbol` and `tree.pos` are correct.

On the `+=` call:

Tree:         Apply
tree.pos:     RangePosition(/home/antoras/dev/runtime-EclipseApplicationwithEquinoxWeaving/Test/src/Test.scala, 76, 78, 82)
tree.tpe:     Unit

symbol:         method c_=
symbol.info:     (x$1: C)Unit

Here, both `symbol` and `tree.pos` are wrong.

[Simon Schäfer]

I did some refactorings on the test suite for hyperlinking. I considered
that it can be useful to test against
IHyperlinkDetector.detectHyperlinks instead of more concrete subclasses.
This will allow the test suite to test arbitrary implementations without
testing the implementations directly. The only thing I still need for
this are a ITextViewer and a ITextEditor but I have no idea where to get
them from. Is it possible to create them in the test suite and more
importantly is it even useful or too heavyweight for this tests?

[iulian dragos]

On Sun, Oct 7, 2012 at 12:11 AM, Simon Schäfer <ma...@antoras.de> wrote:

On 10/06/2012 10:46 PM, iulian dragos wrote:

I'm not sure what you mean. Here's the relevant part of the code:

https://github.com/scala-ide/scala-ide/blob/master/org.scala-ide.sdt.core/src/scala/tools/eclipse/hyperlink/text/detector/ScalaDeclarationHyperlinkComputer.scala#L67

As you can see, for 'apply' we extract 2 symbols from the tree: one
for the qualifier (meaning the definition under the cursor), and
another for the `apply` method itself. If you do the same for other
standard names, like `unapply` and `unapplySeq`, the rest of the
machinery should create the hyperlinks for those symbols.

Yes this works for `apply` but not for `unapply` or `unapplySeq`. This can be tested on `val Some(x) = Some(0)`. ScalaSpy shows on RHS

Tree:         Select
[...]
tree.tpe:     [A](x: A)Some[A]

symbol:         method apply
symbol.info:     [A](x: A)Some[A]

but on LHS

Tree:         Select
[...]
tree.tpe:     Some.type

symbol:         object Some
symbol.info:     Some.type

The `symbol` is not correct any more.

You're right.  I take back what I said about being able to hyperlink to unapply. It's not that easy. Here's what happens: the unapply looks like this in the AST (try running scalac -Ybrowse:typer on the command line -- note to self: add this to the Spy plugin):

CaseDef(Unapply(Apply(Select(Ident("MySome"), "unapply"), "<dummy arg>"), ..)

The positions for everything under `Unapply` down to `Ident` are the same. The askTypeAt method returns the innermost tree that encloses the position we ask for, meaning `Ident`. You can't get to the parent of a node, so unfortunately there's no easy way to get to the unapply.

My test program is using a custom unapply. Note that Some (any case class in fact) does not go through unapply, so there wouldn't be any hyperlinking anyway. The compiler translates patterns of case classes to instanceOf tests.

object MySome {
  def unapply(a: Int): Option[String] = Some(a.toString)
}

class TestFoo {
  val MySome(x) = 10
}

iulian