Permalink
Browse files

Fixes #1001268 - No more NPE in update occurrences when a source is d…

…eleted

Deleting a source file could lead to NullPointerException (NPE) in
`ScalaOccurrencesFinder`.

The issue is actually easy to reproduce. Say you have a source `A` and that
`ScalaOccurrencesFinder.findOccurrences` was just ran against it. At this
point, the value of `ScalaOccurrencesFinder.indexCache` holds a reference to
`A`.  In this scenario, deleting `A` will cause a NPE to be thrown the next
time `ScalaOccurrencesFinder.findOccurrences` is ran against any other source.

The above described scenario happens because `ScalaOccurrencesFinder` is a
singleton, and `ScalaOccurrencesFinder.getCachedIndex` is used to check for
equality of source files. In the current implementation, checking equality on
source files throws a NPE exception if the resource does no longer exist (have
a look at `EclipseResource.equals`).

The fix simply consists in linking each editor (and hence each source file) to
a *different* `ScalaOccurrencesFinder` instance. By doing so, the
`ScalaOccurrencesFinder.indexCache` no longer needs to hold a reference to the
`SourceFile`, and hence the NPE can no longer occur.

An additional positive aspect of not using a singleton `ScalaOccurrencesFinder`
is that now the occurrences index (i.e., `MarkOccurrencesIndex`) is no longer
rebuilt while switching among the active editors.

Last, there are a number of refactoring that have been carried out, I'll
mention the three I consider important:

* The Eclipse `Job` instance used to execute the "update occurrences" task,
  used to have high priority (i.e., Job.INTERACTIVE), while it is now low
  priority. The reasoning is that while the information computed by "update
  occurrences" are useful, they are not critical.

* The signature of `InteractiveCompilationUnitEditor` used to return an
  `Option[InteractiveCompilationUnit]`, ant it now returns a
  `InteractiveCompilationUnit`. The reasoning here is that an editor should
  always have a compilation unit attached to it. The only moment when this is
  not the case is during the editor initialization, but calling
  `getInteractiveCompilationUnit` on a yet not fully initialized editor doesn't
  make sense, and hence this should fail fast (i.e., throwing a NPE is a good
  way to inform the developer that is doing the wrong thing, returning None is
  not as good IMO).
  Furtheremore, if you look at the implementations of
  `getInteractiveCompilationUnit` in `ScalaSourceFileEditor` and
  `ScalaClassFileEditor`, you will see that both implementations already assumed
  that a compilation unit is already attached to the editor (the compilation unit
  is wrapped in a `Some`, not in an `Option`). Therefore, returning an `Option`
  appears like an unneeded overhead, and the current implementation should work
  no worse than the former.
* Added debug output to know how much time is spent for
    1) Building the mark occurrences index, and
    2) end-to-end update occurrences action.
  • Loading branch information...
1 parent b22ed7e commit c68b3b7d926324b02fc5826029a9741eb4d2bbc0 @dotta dotta committed Oct 12, 2012
@@ -43,7 +43,7 @@ class OccurrencesFinderTest {
val region = ScalaWordFinder.findWord(contents, pos - 1)
val word = new String(contents.slice(region.getOffset(), region.getOffset() + region.getLength()))
println("using word region: " + region)
- val occurrences = ScalaOccurrencesFinder.findOccurrences(unit, region.getOffset, region.getLength, 1)
+ val occurrences = new ScalaOccurrencesFinder(unit).findOccurrences(region, 1)
assertTrue("No occurrences of %s".format(word), occurrences.isDefined)
assertEquals("Not enough occurrences (%s): expected: %d, found: %d".format(word, count, occurrences.get.locations.size), count, occurrences.get.locations.size)
}
@@ -54,8 +54,8 @@ class ScalaClassFileEditor extends ClassFileEditor with ScalaEditor {
setAction("OpenEditor", openAction)
}
- override def getInteractiveCompilationUnit(): Option[InteractiveCompilationUnit] =
+ override def getInteractiveCompilationUnit(): InteractiveCompilationUnit = {
// getInputJavaElement always returns the right value
- Some(getInputJavaElement().asInstanceOf[InteractiveCompilationUnit])
-
+ getInputJavaElement().asInstanceOf[InteractiveCompilationUnit]
+ }
}
@@ -68,14 +68,18 @@ import org.eclipse.swt.events.VerifyEvent
import org.eclipse.jface.text.ITextViewerExtension
import scala.tools.eclipse.ui.SurroundSelectionStrategy
import org.eclipse.jface.text.IDocumentExtension4
+import scala.tools.eclipse.util.EditorUtils
+import org.eclipse.swt.widgets.Composite
-class ScalaSourceFileEditor extends CompilationUnitEditor with ScalaEditor {
+class ScalaSourceFileEditor extends CompilationUnitEditor with ScalaEditor { self =>
import ScalaSourceFileEditor._
setPartName("Scala Editor")
+ private var occurrencesFinder: ScalaOccurrencesFinder = _
+
def scalaPrefStore = ScalaPlugin.prefStore
def javaPrefStore = getPreferenceStore
@@ -156,28 +160,18 @@ class ScalaSourceFileEditor extends CompilationUnitEditor with ScalaEditor {
} yield annotationModel
private def performOccurrencesUpdate(selection: ITextSelection, documentLastModified: Long) {
-
- // TODO: find out why this code does a cast to IAdaptable before calling getAdapter
- val adaptable = getEditorInput.asInstanceOf[IAdaptable].getAdapter(classOf[IJavaElement])
- // logger.info("adaptable: " + adaptable.getClass + " : " + adaptable.toString)
-
- adaptable match {
- case scalaSourceFile: ScalaSourceFile =>
- val annotations = getAnnotations(selection, scalaSourceFile, documentLastModified)
- for (annotationModel <- getAnnotationModelOpt)
- annotationModel.withLock {
- annotationModel.replaceAnnotations(occurrenceAnnotations, annotations)
- occurrenceAnnotations = annotations.keySet
- }
- case _ =>
- // TODO: pop up a dialog explaining what needs to be fixed or fix it ourselves
- Utils tryExecute (adaptable.asInstanceOf[ScalaSourceFile], // trigger the exception, so as to get a diagnostic stack trace
- msgIfError = Some("Could not recompute occurrence annotations: configuration problem"))
+ val annotations = getAnnotations(selection, getInteractiveCompilationUnit, documentLastModified)
+ for{
+ annotationModel <- getAnnotationModelOpt
+ } annotationModel.withLock {
+ annotationModel.replaceAnnotations(occurrenceAnnotations, annotations)
+ occurrenceAnnotations = annotations.keySet
}
}
- private def getAnnotations(selection: ITextSelection, scalaSourceFile: ScalaSourceFile, documentLastModified: Long): Map[Annotation, Position] = {
- val occurrences = ScalaOccurrencesFinder.findOccurrences(scalaSourceFile, selection.getOffset, selection.getLength, documentLastModified)
+ private def getAnnotations(selection: ITextSelection, unit: InteractiveCompilationUnit, documentLastModified: Long): Map[Annotation, Position] = {
+ val region = EditorUtils.textSelection2region(selection)
+ val occurrences = occurrencesFinder.findOccurrences(region, documentLastModified)
for {
Occurrences(name, locations) <- occurrences.toList
location <- locations
@@ -206,12 +200,15 @@ class ScalaSourceFileEditor extends CompilationUnitEditor with ScalaEditor {
val job = new Job("Updating occurrence annotations") {
def run(monitor: IProgressMonitor) = {
- performOccurrencesUpdate(selection, lastModified)
+ val fileName = getInteractiveCompilationUnit.file.name
+ Utils.debugTimed("Time elapsed for \"updateOccurrences\" in source " + fileName) {
+ performOccurrencesUpdate(selection, lastModified)
+ }
Status.OK_STATUS
}
}
-
- job.setPriority(Job.INTERACTIVE)
+ // set low priority for update occurrences
+ job.setPriority(Job.DECORATE)
job.schedule()
}
@@ -323,6 +320,7 @@ class ScalaSourceFileEditor extends CompilationUnitEditor with ScalaEditor {
override def createPartControl(parent: org.eclipse.swt.widgets.Composite) {
super.createPartControl(parent)
+ occurrencesFinder = new ScalaOccurrencesFinder(getInteractiveCompilationUnit)
refactoring.RefactoringMenu.fillQuickMenu(this)
getSourceViewer match {
case sourceViewer: ITextViewerExtension =>
@@ -368,9 +366,10 @@ class ScalaSourceFileEditor extends CompilationUnitEditor with ScalaEditor {
}
- override def getInteractiveCompilationUnit(): Option[InteractiveCompilationUnit] =
+ override def getInteractiveCompilationUnit(): InteractiveCompilationUnit = {
// getInputJavaElement always returns the right value
- Some(getInputJavaElement().asInstanceOf[InteractiveCompilationUnit])
+ getInputJavaElement().asInstanceOf[InteractiveCompilationUnit]
+ }
}
@@ -1,50 +1,68 @@
-package scala.tools.eclipse
-package markoccurrences
+package scala.tools.eclipse.markoccurrences
-import org.eclipse.jface.text.Region
-
-import scala.tools.eclipse.javaelements.ScalaCompilationUnit
+import scala.tools.eclipse.InteractiveCompilationUnit
+import scala.tools.eclipse.logging.HasLogger
import scala.tools.nsc.util.SourceFile
import scala.tools.refactoring.analysis.GlobalIndexes
import scala.tools.refactoring.implementations.MarkOccurrences
+import org.eclipse.jface.text.IRegion
+import org.eclipse.jface.text.Region
+import scala.tools.eclipse.util.Utils
-case class Occurrences(name: String, locations: List[Region])
+case class Occurrences(name: String, locations: List[IRegion])
-object ScalaOccurrencesFinder {
-
- private var indexCache: Option[(SourceFile, Long, MarkOccurrences with GlobalIndexes)] = None
+/** Finds all occurrences of a binding in a Scala source file.
+ *
+ * Note that the occurrences index is re-computed only if the document's modification timestamp
+ * has changed.
+ *
+ * This class is thread-safe.
+ */
+class ScalaOccurrencesFinder(unit: InteractiveCompilationUnit) extends HasLogger {
+ import ScalaOccurrencesFinder._
+
+ /* IMPORTANT:
+ * In the current implementation, all access to `indexCache` are confined to the Presentation
+ * Compiler thread (and this is why `indexCache` requires no synchronization policy).
+ */
+ private var indexCache: Option[TimestampedIndex] = None
- private def getCachedIndex(sourceFile: SourceFile, lastModified: Long) = indexCache match {
- case Some(Triple(`sourceFile`, `lastModified`, index)) => Some(index)
+ private def getCachedIndex(lastModified: Long): Option[MarkOccurrencesIndex] = indexCache match {
+ case Some(TimestampedIndex(`lastModified`, index)) => Some(index)
case _ => None
}
- private def cacheIndex(sourceFile: SourceFile, lastModified: Long, index: MarkOccurrences with GlobalIndexes) {
- indexCache = Some(Triple(sourceFile, lastModified, index))
+ private def cacheIndex(lastModified: Long, index: MarkOccurrencesIndex): Unit = {
+ indexCache = Some(TimestampedIndex(lastModified, index))
}
-
- def findOccurrences(file: ScalaCompilationUnit, offset: Int, length: Int, lastModified: Long): Option[Occurrences] = {
- val (from, to) = (offset, offset + length)
- file.withSourceFile { (sourceFile, compiler) =>
+
+ def findOccurrences(region: IRegion, lastModified: Long): Option[Occurrences] = {
+ unit.withSourceFile { (sourceFile, compiler) =>
compiler.askOption { () =>
+ def isNotLoadedInPresentationCompiler(source: SourceFile): Boolean =
+ !compiler.unitOfFile.contains(source.file)
- val occurrencesIndex = getCachedIndex(sourceFile, lastModified) getOrElse {
- val occurrencesIndex = new MarkOccurrences with GlobalIndexes {
- val global = compiler
- lazy val index = GlobalIndex(global.body(sourceFile))
+ if(isNotLoadedInPresentationCompiler(sourceFile)) {
+ logger.info("Source %s is not loded in the presentation compiler. Aborting occurrences update."format(sourceFile.file.name))
+ None
+ } else {
+ val occurrencesIndex = getCachedIndex(lastModified) getOrElse {
+ val occurrencesIndex = new MarkOccurrencesIndex {
+ val global = compiler
+ lazy val index = Utils.debugTimed("Time elapsed for building mark occurrences index in source " + sourceFile.file.name) {
+ GlobalIndex(global.body(sourceFile))
+ }
+ }
+ cacheIndex(lastModified, occurrencesIndex)
+ occurrencesIndex
}
- cacheIndex(sourceFile, lastModified, occurrencesIndex)
- occurrencesIndex
- }
- if (!compiler.unitOfFile.contains(sourceFile.file))
- None
- else {
+ val (from, to) = (region.getOffset, region.getOffset + region.getLength)
val (selectedTree, occurrences) = occurrencesIndex.occurrencesOf(sourceFile.file, from, to)
Option(selectedTree.symbol) filter (!_.name.isOperatorName) map { sym =>
val locations = occurrences map { pos =>
- new Region(pos.start, pos.end - pos.start)
+ new Region(pos.startOrPoint, pos.endOrPoint - pos.startOrPoint)
}
Occurrences(sym.nameString, locations)
}
@@ -54,3 +72,7 @@ object ScalaOccurrencesFinder {
}
}
+object ScalaOccurrencesFinder {
+ private abstract class MarkOccurrencesIndex extends MarkOccurrences with GlobalIndexes
+ private case class TimestampedIndex(timestamp: Long, index: MarkOccurrencesIndex)
+}
@@ -3,7 +3,5 @@ package scala.tools.eclipse.ui
import scala.tools.eclipse.InteractiveCompilationUnit
trait InteractiveCompilationUnitEditor {
-
- def getInteractiveCompilationUnit(): Option[InteractiveCompilationUnit]
-
+ def getInteractiveCompilationUnit(): InteractiveCompilationUnit
}
@@ -25,7 +25,7 @@ object EditorUtils {
def getEditorCompilationUnit(editor: ITextEditor): Option[InteractiveCompilationUnit] = {
editor match {
case icuEditor: InteractiveCompilationUnitEditor =>
- icuEditor.getInteractiveCompilationUnit
+ Some(icuEditor.getInteractiveCompilationUnit)
case _ =>
None
}
@@ -12,8 +12,9 @@ import scala.tools.eclipse.InteractiveCompilationUnit
import scala.tools.eclipse.ui.InteractiveCompilationUnitEditor
import org.eclipse.swt.graphics.Font
import org.eclipse.jface.resource.JFaceResources
+import scala.tools.eclipse.logging.HasLogger
-class SpyView extends ViewPart {
+class SpyView extends ViewPart with HasLogger {
private var textArea: Text = _
def setFocus() {
@@ -38,27 +39,26 @@ class SpyView extends ViewPart {
part match {
case icuEditor: InteractiveCompilationUnitEditor =>
- for (cu <- icuEditor.getInteractiveCompilationUnit) {
- cu.doWithSourceFile { (source, compiler) =>
- import compiler._
+ val cu = icuEditor.getInteractiveCompilationUnit
+ cu.doWithSourceFile { (source, compiler) =>
+ import compiler._
- val response = new Response[Tree]
- compiler.askTypeAt(rangePos(source, offset, offset, offset + length), response)
- response.get match {
- case Left(tree) =>
- textArea.append("\n\n============\n\nTree: \t\t" + tree.productPrefix)
- textArea.append("\ntree.pos: \t%s".format(tree.pos))
- textArea.append("\ntree.tpe: \t%s".format(tree.tpe))
- textArea.append("\n\nsymbol: \t\t%s".format(tree.symbol))
- for (sym <- Option(tree.symbol) if sym ne NoSymbol)
- textArea.append("\nsymbol.info: \t%s".format(tree.symbol.info))
-
- case _ =>
-
- }
+ val response = new Response[Tree]
+ compiler.askTypeAt(rangePos(source, offset, offset, offset + length), response)
+ response.get match {
+ case Left(tree) =>
+ textArea.append("\n\n============\n\nTree: \t\t" + tree.productPrefix)
+ textArea.append("\ntree.pos: \t%s".format(tree.pos))
+ textArea.append("\ntree.tpe: \t%s".format(tree.tpe))
+ textArea.append("\n\nsymbol: \t\t%s".format(tree.symbol))
+ for (sym <- Option(tree.symbol) if sym ne NoSymbol)
+ textArea.append("\nsymbol.info: \t%s".format(tree.symbol.info))
+ case Right(ex) => logger.debug(ex)
}
}
+
+ case editor => ()
}
textArea.setSelection(0, 0)
}

0 comments on commit c68b3b7

Please sign in to comment.