Filter out step into forwarder and default getter methods. #224

Merged
merged 2 commits into from Nov 7, 2012

Conversation

Projects
None yet
3 participants
Owner

dragos commented Nov 4, 2012

A general reorganization of step filters was necessary:

  • made it a class instead of an object
  • split functionality between the filter class and a method classifier
  • split tests in unit tests (using the method classifier) and integration tests (testing the step-into
    inside a debugger instance)

A separate commit makes the Scala launch delegate behave according to the spec, throwing
CoreException on failure. Failure to do so was making tests hang indefinitely when a debug-test
project had errors, and therefore could not be launched.

Fixed #1001288.

Throw an exception if the launcher fails.
The `MainMethodVerifier` may stop the Scala launcher if there is no main type,
or the project has errors. The Eclipse API requires that we throw `CoreException`,
but we used to fail silently (leading to the debug tests hanging indefinitely).
...tests/src/scala/tools/eclipse/debug/model/MethodClassifierUnitTest.scala
+ @Test
+ def cp_testConcreteClassExtendsTrait() {
+ val resource = SDTTestUtils.workspace.getRoot().getFile("/constant-pool/bin/stepping/ConcreteClass.class")
+ val pool = ConstantPool.fromFile(resource.getLocation().toFile())
@dotta

dotta Nov 5, 2012

Owner

What are the tests checking? That the ConstantPool can parse the binary without throwing exception? Is the pool local value useful?

@dragos

dragos Nov 5, 2012

Owner

They check that there are no crashes. Yeap, I can remove the pool value.

@dotta

dotta Nov 6, 2012

Owner

I see. Would it make sense to add some assert using the pool? Or this is already covered by some other test?

@dragos

dragos Nov 6, 2012

Owner

Not in this test, as long as there is no exception I am happy. The other tests will exercise methods on the pool when parsing full classfiles.

+ @Test
+ def testForwarder_neg_Java() {
+ // this is a call to a Java static method with the same name (but it's not inside an Impl class)
+ assertForwarder("console", false)
@dotta

dotta Nov 5, 2012

Owner

IMO, it would be easier to read without having to pass the boolean argument. What do you think about having instead assertNoForwarder and assertHasForwarder

@dragos

dragos Nov 5, 2012

Owner

I'm not sure the readability is that bad right now, and adding two more methods seems overkill.

@dotta

dotta Nov 6, 2012

Owner

Fair enough.

+ inv.getArguments()(0) match {
+ case name: String =>
+ val field = mock(classOf[Field])
+ parser.fields.get(name).map(_ => field).getOrElse(null)
@dotta

dotta Nov 5, 2012

Owner

Let's make sure I understand what's happening here: you are returning a mocked field if a field with the passed name exists in parser.fields, null otherwise. Is that it?

@dragos

dragos Nov 5, 2012

Owner

Exactly. :)

...ide.sdt.debug/src/scala/tools/eclipse/debug/classfile/ConstantPool.scala
+ idx += 9
+ i += 1 // long and double constants occupy *two* slots
+ case _ =>
+ assert(false, "Unknown entry at index %d: %d ".format(i, bytes(idx)))
@dotta

dotta Nov 5, 2012

Owner

Considering assert can be elided, maybe it's better to throw a "real" exception, e.g., IllegalStateException.

@dragos

dragos Nov 5, 2012

Owner

It's not an expected outcome, that should really not happen. I don't think we elide assertions even in compiler release builds.

I am not sure what to do: throwing an exception would not convey the underlying assumption of the code. On the other hand, maybe the calling code wants to be able to recover... hard choice.

@dotta

dotta Nov 6, 2012

Owner

It's not an expected outcome, that should really not happen.

I know. And that's why you are using an assertion. I just think that an exception would be better in this case. And, I checked the doc for IllegalStateException, and figured it is not the right exception to throw in this case. I believe UnsupportedOperationException is the one.

I don't think we elide assertions even in compiler release builds.

What is true today may very well change tomorrow, and the less thing you need to remember the better.

I am not sure what to do: throwing an exception would not convey the underlying assumption of the code. On the other hand, maybe the calling code wants to be able to recover... hard choice.

What do you think of UnsupportedOperationException?

@dragos

dragos Nov 6, 2012

Owner

I will throw a custom exception, I think it makes more sense.

...ebug/src/scala/tools/eclipse/debug/preferences/DebuggerPreferences.scala
@@ -29,6 +30,8 @@ Configured step filters:
addField(new BooleanFieldEditor(FILTER_SYNTHETIC, "Filter SYNTHETIC methods", parent))
addField(new BooleanFieldEditor(FILTER_GETTER, "Filter Scala getters", getFieldEditorParent))
addField(new BooleanFieldEditor(FILTER_SETTER, "Filter Scala setters", getFieldEditorParent))
+ addField(new BooleanFieldEditor(FILTER_DEFAULT_GETTER, "Filter default getters", getFieldEditorParent))
@dotta

dotta Nov 5, 2012

Owner

I wonder if "Filter compiler generated getters" would be a better description.

@dragos

dragos Nov 5, 2012

Owner

Default getters are getters for default parameters. Good point, I'll update the description accordingly.

+
+ /** Is the given method of the `kind` type? */
+ def is(kind: MethodClassifier.Value, method: Method): Boolean = {
+ kind match {
@dotta

dotta Nov 5, 2012

Owner

Instead of using pattern matching, why not having proper (case) classes for the different MethodClassifiers. I find it would be easier to read.

@dragos

dragos Nov 5, 2012

Owner

A method may have more than one classifier.. I am not proud of this code, but I spent some time thinking of better designs and didn't find something that I like. Any suggestions are welcome.

@dotta

dotta Nov 6, 2012

Owner

The following is what I had in mind:

object MethodClassifier extends Enumeration {
  // This is defined in Enumeration but it cannot be accessed!?!
  private def nextNameOrNull = if (nextName != null && nextName.hasNext) nextName.next else null

  protected abstract class Classifier extends super.Val(nextId, nextNameOrNull) {
    def is(method: Method): Boolean
  }
  case object Synthetic extends Classifier {
    def is(method: Method): Boolean = method.isSynthetic()
  }
  case object Getter extends Classifier {
    def is(method: Method): Boolean = method.declaringType().fieldByName(method.name()) ne null
  }
  ...

  def allKindsOf(method: Method): Set[MethodClassifier.Value] = values.filter(_.is(method))
}

I know it's more verbose, but I like that it keeps data and behavior close, particularly for the Forwarder and DefaultGetter case.

@dragos

dragos Nov 6, 2012

Owner

I like it (except the part where we depend on internals of Enumeration). Now what would be a better name for def is?

@dragos

dragos Nov 6, 2012

Owner

Actually, it's not so good. Moving all the logic in the enumeration object makes it static and would prevent caching in the future (that is important for constant pool parsing in case of Forwarders)

@dragos

dragos Nov 6, 2012

Owner

I'll add a comment about these alternatives and leave it as such for the moment. We can revisit this when we work more on step filters, like adding caching. Does that sound good?

+ final val CONSTANT_Utf8 = 1
+ final val CONSTANT_MethodHandle = 15
+ final val CONSTANT_MethodType = 16
+ final val CONSTANT_InvokeDynamic = 18
@dotta

dotta Nov 5, 2012

Owner

Should be moved to the companion object.

@dragos

dragos Nov 6, 2012

Owner

Good catch!

Filter out step into forwarder and default getter methods.
A general reorganization of step filters was necessary:

- made it a class instead of an object
- split functionality between the filter class and a method classifier
- split tests in unit tests (using the method classifier) and integration tests (testing the step-into
  inside a debugger instance)

A separate commit makes the Scala launch delegate behave according to the spec, throwing
`CoreException` on failure. Failure to do so was making tests hang indefinitely when a debug-test 
project had errors, and therefore could not be launched.

Fixed #1001288.
Owner

dragos commented Nov 6, 2012

I forced-push fixes for the comments you had. If the build goes through, I'll merge it.

Owner

dragos commented Nov 6, 2012

PLS REBUILD ALL

dragos added a commit that referenced this pull request Nov 7, 2012

Merge pull request #224 from dragos/issue/more-step-filters-1001288
Filter out step into forwarder and default getter methods.

@dragos dragos merged commit cc81f37 into scala-ide:master Nov 7, 2012

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