Suppressing hundreds of warnings at compile time.

aimichal commented 8 years ago

Fixing all "Multi-line if / else statements must have braces" problems because it's easy to do, and makes the code clearer. Leaving everything else alone, and writing my reasoning in build.sbt.

tests pass and compile runs without noise:

> compile
[info] Updating {file:/Users/michalg/code/openie-standalone/}openie...
[info] Resolving jline#jline;2.12 ...
[info] Done updating.
[info] Compiling 115 Scala sources to /Users/michalg/code/openie-standalone/target/scala-2.11/classes...
[success] Total time: 17 s, completed Jun 8, 2016 5:41:34 PM

danxmoran commented 8 years ago

Some comments about the general approach on my end. I'm worried about globally suppressing everything, especially if the plan is to hand the library off to somebody else at AI2 who might later be tripped up by the lack of expected warnings.

jkinkead commented 8 years ago

I'm fine turning off the style plugin. Turning off the compiler warnings is really bad, and we should not do that.

aimichal commented 8 years ago

Made some easy-ish fixes. I'm a little rusty on equals/hashCode implementations, and relied on IntelliJ to generate hashCode where I could. In SrlExtraction.scala I had to do it manually.

Remaining scalac warnings (always shown because "-nowarn" was removed):

[warn] .../chunkedextractor/Relnoun.scala:130: possible missing interpolator: detected an interpolated expression
[warn] .../chunkedextractor/Relnoun.scala:135: possible missing interpolator: detected an interpolated expression
[warn] .../chunkedextractor/Relnoun.scala:139: possible missing interpolator: detected an interpolated expression
[warn] .../chunkedextractor/Relnoun.scala:406: possible missing interpolator: detected an interpolated expression
[warn] .../chunkedextractor/Relnoun.scala:409: possible missing interpolator: detected an interpolated expression
[warn] .../chunkedextractor/Relnoun.scala:412: possible missing interpolator: detected an interpolated expression
[warn] .../chunkedextractor/Relnoun.scala:449: possible missing interpolator: detected an interpolated expression
[warn] .../chunkedextractor/Relnoun.scala:695: possible missing interpolator: detected an interpolated expression
[warn] .../collection/immutable/Interval.scala:407: Adapting argument list by creating a 2-tuple: this may not be what you want.
[warn] .../collection/immutable/Interval.scala:415: Adapting argument list by creating a 2-tuple: this may not be what you want.
[warn] .../collection/mutable/PrefixSet.scala:23: type parameter String defined in method apply shadows type String defined in object Predef. You may want to rename your type parameter, or possibly remove it.
[warn] .../common/Analysis.scala:52: Adapting argument list by creating a 2-tuple: this may not be what you want.
[warn] .../common/Analysis.scala:86: Adapting argument list by creating a 3-tuple: this may not be what you want.
[warn] .../srlie/SrlExtraction.scala:257: a type was inferred to be `Any`; this may indicate a programming error.
[warn] .../srlie/SrlExtractor.scala:123: method deserialize in object DependencyGraph is deprecated: Use StringFormat instead.
[warn] .../srlie/SrlExtractor.scala:139: method serialize in class DependencyGraph is deprecated: Use StringFormat instead.
[warn] .../srlie/confidence/AnalyzeFeatures.scala:27: method deserialize in object DependencyGraph is deprecated: Use StringFormat instead.

Remaining style warnings (when style checking not faked out in build.sbt):

[warn] .../tool/chunk/Chunker.scala:96:8: Unusual class name; consider renaming
[warn] .../tool/postag/Postagger.scala:72:8: Unusual class name; consider renaming
[error] .../tool/tokenize/ClearTokenizer.scala:5:0: illegal.imports.message
[error] .../tool/tokenize/OpenNlpTokenizer.scala:6:0: illegal.imports.message
[error] .../tool/tokenize/PennTokenizer.scala:5:0: illegal.imports.message
[warn] .../tool/tokenize/Tokenizer.scala:56:8: Unusual class name; consider renaming

... and 335 occurrences of "Line is more than 100 characters long"

danxmoran commented 8 years ago

Just a few final nitpicks :) but nothing blocking! I'll leave the magic acronym to @jkinkead.

jkinkead commented 8 years ago

A few comments. I think the canEqual check is a bug.

aimichal commented 8 years ago

EqualsVerifier is an opinionated checker for .equals and .hashCode implementations. See here: http://www.jqno.nl/equalsverifier/

I used it to test the various properties of .equals: from Effective Java chapter 3, they are reflexivity, symmetry, transitivity, consistency, and non-nullity. Some checks revealed long-standing bugs. Some couldn't be honored, and resulted in .warnings exclusions. Here's a summary of changes:

File Interval.scala:

class Interval: This is a sealed class, and most descendants are also sealed, so .equals and .hashCode were made final. (The unsealed descendants are SingletonImpl, Empty, and Closed. I'm betting no one extended those.)

File DirectEdge.scala:

class DirectEdge has no tests because:

UpEdge and DownEdge tests already exercise DirectEdge's .equals.
As an abstract class, it's hard to test (see http://www.jqno.nl/equalsverifier/2010/11/14/abstract-delegation/). I tried several variations each of which produce different equally cryptic compiler errors:

  import edu.knowitall.collection.immutable.graph.Graph.Edge
  ...
  test("DirectedEdge") {
    val e1 = new Edge("foo", "bar", "fnord")
    EqualsVerifier.forClass(classOf[DirectedEdge[String]])
        .suppress(Warning.NULL_FIELDS)
        // .withPrefabValues(classOf[Edge[_]], new UpEdge(e1), new DownEdge(e1))
        // .withPrefabValues(classOf[Edge[String]], new UpEdge(e1), new DownEdge(e1))
        // .withPrefabValues(classOf[UpEdge[_]], new UpEdge(e1), new DownEdge(e1))
        // .withPrefabValues(classOf[UpEdge[String]], new UpEdge(e1), new DownEdge(e1))
        .verify
  }

Also tried this, which compiles, but still throws the abstract delegation exception deadend:

  test("DirectedEdge") {
    val e1 : Edge[String] = new Edge[String]("source", "dest", "label1")
    val e2 : Edge[String] = new Edge[String]("source", "dest", "label2")
    val ue1 = new UpEdge[String](e1)
    val ue2 = new UpEdge[String](e2)
    val de1 = new DownEdge[String](e1)
    val de2 = new DownEdge[String](e2)
    EqualsVerifier.forClass(classOf[DirectedEdge[_]])
        .suppress(Warning.NULL_FIELDS)
        .withPrefabValues(classOf[UpEdge[_]], ue1, ue2)
        .withPrefabValues(classOf[DownEdge[_]], de1, de2)
        .suppress(Warning.STRICT_INHERITANCE)
        .verify
  }

File SrlExtraction.scala

class MultiPart has its .equals and .hashCode fully tested by way of tests for Relation and Context.

File Graph.scala

class Graph: Fixed long-standing bug where equals was not checking internal fields outgoing and incoming.

File SrlExtraction.scala

class MultiPart: Used the correct class name in canEqual class Context: Made hashCode use field values directly instead of relying on super.hashCode class Relation: fixed long-standing bug where equals did not use field sense. class Relation: defined a new hashCode in terms of class fields, not super.hashCode

jkinkead commented 8 years ago

LGTM! Merging.

allenai / openie-standalone

Suppressing hundreds of warnings at compile time. #9