Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

1001621 Improved tree traversal #23

Merged
merged 1 commit into from

3 participants

@mads379
Collaborator

This fixes three issues

  • The code used super.traverse to recurse the trees. This was wrong as it would skip one 'layer' of nodes.
  • Traverse the argument list of DefDef's
  • Store occurrences in ValDefs

So it turned out that the failure that was seen with the string
interpolation test had nothing to do with string interpolation.

Originally it only found one occurrence of x in the following
example.

def foo(x: String) = s"Hi there, ${x}"

I assumed that the x that was missing was the one inside of the
string interpolation, but actually it was the argument. Ups.

Fix #1001621

@mads379
Collaborator

@dragos | @dotta This is ready for reviewing :)

@scala-jenkins

Started jenkins job scala-search-3.0.x-2.10-pr-validator at https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-3.0.x-2.10-pr-validator/23/

@scala-jenkins

jenkins job scala-search-3.0.x-2.10-pr-validator: Success - https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-3.0.x-2.10-pr-validator/23/

@scala-jenkins

Started jenkins job scala-search-master-2.10-pr-validator at https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-master-2.10-pr-validator/24/

@scala-jenkins

jenkins job scala-search-master-2.10-pr-validator: Success - https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-master-2.10-pr-validator/24/

...ols/eclipse/search/indexing/OccurrenceCollector.scala
((10 lines not shown))
occurrences += Occurrence(name.toString, file, t.pos.point, Declaration)
- super.traverse(body)
+ super.traverseTreess(args)
@dragos Owner
dragos added a note

I think it's better to remove super., just traverseTreess, in case you ever override traverseTreess in this class. Or, to be consistent, everywhere call super.traverse(tree) (the original tree that you are pattern-matching against). That would also eliminate the risk that you forget to call traverse on any sub-tree.

In this case, for instance, I am not sure it's correct to ignore the modifiers (the first argument in DefDef). Modifiers contain annotations, AFAIK. Maybe add a test that it correctly recurses in annotations (and their arguments).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mads379
Collaborator

@dragos Removed the explicit super and it now indexes annotations as well :)

@scala-jenkins

Started jenkins job scala-search-master-2.10-pr-validator at https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-master-2.10-pr-validator/26/

@scala-jenkins

Started jenkins job scala-search-3.0.x-2.10-pr-validator at https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-3.0.x-2.10-pr-validator/26/

@scala-jenkins

jenkins job scala-search-master-2.10-pr-validator: Success - https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-master-2.10-pr-validator/26/

@scala-jenkins

jenkins job scala-search-3.0.x-2.10-pr-validator: Success - https://jenkins.scala-ide.org:8496/jenkins/job/scala-search-3.0.x-2.10-pr-validator/26/

@dragos
Owner

I love it when I'm right! :sunglasses:

LGTM!

@mads379
Collaborator

It's the second time you've looked at the tree traversal and found an issue ;)

@mads379 mads379 Improved tree traversal
This fixes four issues

- The code used super.traverse to recurse the trees. This was wrong
  as it would skip one 'layer' of nodes.
- Traverse the argument list of DefDef's
- Traverse annotations on DefDef's
- Store occurrences in ValDefs

So it turned out that the failure that was seen with the string
interpolation test had nothing to do with string interpolation.

Originally it only found one occurrence of `x` in the following
example.

  def foo(x: String) = s"Hi there, ${x}"

I assumed that the `x` that was missing was the one inside of the
string interpolation, but actually it was the argument. Ups.

Fix #1001621
e9b0883
@mads379
Collaborator

Squashed commits. Should be good to merge. :+1:

@dragos
Owner

Sorry, I didn't realize I have to merge it myself! Done!

@dragos dragos merged commit 7194925 into scala-ide:master
@mads379 mads379 deleted the mads379:1001621-record-occurrences-in-string-interpolation branch
@mads379
Collaborator

Thanks! If you want to lessen @dotta's code-review burdon for tomorrow you can review #22 as well ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 9, 2013
  1. @mads379

    Improved tree traversal

    mads379 authored
    This fixes four issues
    
    - The code used super.traverse to recurse the trees. This was wrong
      as it would skip one 'layer' of nodes.
    - Traverse the argument list of DefDef's
    - Traverse annotations on DefDef's
    - Store occurrences in ValDefs
    
    So it turned out that the failure that was seen with the string
    interpolation test had nothing to do with string interpolation.
    
    Originally it only found one occurrence of `x` in the following
    example.
    
      def foo(x: String) = s"Hi there, ${x}"
    
    I assumed that the `x` that was missing was the one inside of the
    string interpolation, but actually it was the argument. Ups.
    
    Fix #1001621
This page is out of date. Refresh to see the latest.
View
13 ...ols.eclipse.search.tests/src/org/scala/tools/eclipse/search/OccurrenceCollectorTest.scala
@@ -68,5 +68,18 @@ class OccurrenceCollectorTest {
}
}
+ @Test def stringInterpolation() {
+ doWithOccurrencesInUnit("org","example","StringInterpolation.scala") { occurrences =>
+ val x = occurrenceFor("x", occurrences)
+ assertEquals("Should be 2 occurrences of x %s".format(x), 2, x.size)
+ }
+ }
+
+ @Test def annotationsOnMethods() {
+ doWithOccurrencesInUnit("org","example", "Annotations.scala") { occurrences =>
+ val x = occurrenceFor("IOException", occurrences)
+ assertEquals("Should be 1 occurrences of IOException %s".format(x), 1, x.size)
+ }
+ }
}
View
7 ...cala.tools.eclipse.search.tests/test-workspace/aProject/src/org/example/Annotations.scala
@@ -0,0 +1,7 @@
+package org.example
+
+import java.io.IOException
+
+object Test {
+ @throws(classOf[IOException]) def test() = {}
+}
View
5 ...ls.eclipse.search.tests/test-workspace/aProject/src/org/example/StringInterpolation.scala
@@ -0,0 +1,5 @@
+package org.example
+
+object StringInterpolation {
+ def foo(x: String) = s"Hi there, ${x}"
+}
View
17 ...ools.eclipse.search/src/org/scala/tools/eclipse/search/indexing/OccurrenceCollector.scala
@@ -39,14 +39,23 @@ object OccurrenceCollector extends HasLogger {
case Select(rest,name) if !isSynthetic(pc)(t, name.toString) =>
occurrences += Occurrence(name.toString, file, t.pos.point, Reference)
- super.traverse(rest) // recurse in the case of chained selects: foo.baz.bar
+ traverse(rest) // recurse in the case of chained selects: foo.baz.bar
// Method definitions
- case DefDef(_, name, _, _, _, body) if !isSynthetic(pc)(t, name.toString) =>
+ case DefDef(mods, name, _, args, _, body) if !isSynthetic(pc)(t, name.toString) =>
occurrences += Occurrence(name.toString, file, t.pos.point, Declaration)
- super.traverse(body)
+ traverseTrees(mods.annotations)
+ traverseTreess(args)
+ traverse(body)
- case _ => super.traverse(t)
+ // Val's and arguments.
+ case ValDef(_, name, tpt, rhs) =>
+ occurrences += Occurrence(name.toString, file, t.pos.point, Declaration)
+ traverse(tpt)
+ traverse(rhs)
+
+ case _ =>
+ super.traverse(t)
}
}
}
Something went wrong with that request. Please try again.