fsc never reuses a compiler instance

[scabug]

I was working on bug #84 and the first thing I noticed was that it doesn't do that. So I thought someone had fixed it, but investigation revealed that fsc never reused a compiler instance even on identical consecutive commands, so of course stale cache entries were not a problem.

The reason is this: before reusing a compiler instance, CompileServer verifies that the settings are identical by calling the custom equals method in Settings, which does this:

List.forall2 (this.allSettings, s.allSettings)(_==_)

This is a very interesting trap, because all the relevant fields of all the settings are indeed equal, but this still always returns false -- in fact no two settings ever compare equal. The reason is that the logic of the equals methods looks like this:

    override def equals(that: Any) = that match {
      case bs: BooleanSetting => this.name == bs.name && this.value == bs.value
      case _ => false
    }

The individual Setting (singular) objects being compared were born in two different Settings (plural) objects, so they are not the same class, and the case bs: BooleanSetting always fails. It should instead look like this:

    override def equals(that: Any) = that match {
      case bs: Settings#BooleanSetting => this.name == bs.name && this.value == bs.value
      case _ => false
    }

I have attached a patch against trunk that fixes this issue -- and with that applied I'm "happy" to say that bug #84 is still a valid bug. This took me a long time to figure out and seems like a very easy trap to fall into, so I wonder if anyone has ideas as to how to make it harder to burn yourself this way.

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-1683?orig=1
Reporter: @paulp
Attachments:

• 1683.patch (created on Jan 30, 2009 10:07:05 PM UTC, 2762 bytes)

[scabug]

@paulp said:
DRMacIver points out that isInstanceOf draws the opposite conclusion. I think either that is wrong, or the type matching behavior is wrong. Here is a demonstration.

class Settings {
  val settings: List[Setting] = List(Setting1("bob"))
  
  abstract class Setting(descr: String) {
    def name: String
  }
  
  case class Setting1(name: String) extends Setting("foo") {
    override def equals(other: Any) = other match {
      case x: Setting1 => true
      case _           => false
    }
  }
  
  case class Setting2(name: String) extends Setting("bar") {
    override def equals(other: Any) = other match {
      case x: Setting2 => true
      case _           => false
    }
  }
  
  override def equals(other: Any) = other match {
    case s: Settings  => List.forall2(settings, s.settings)(_ == _)
    case x            => false
  }
}

object go
{
  def main(args: Array[String]): Unit = {
    val set1 = new Settings
    val set2 = new Settings
    
    println("set1 == set2? " + (set1 == set2))
    println("isInstanceOf == " + set1.settings.head.isInstanceOf[set2.Setting1])
  }
}

$$ scala27 go
set1 == set2? false
isInstanceOf == true

[scabug]

@odersky said:
Good catch! I am going to apply the patch for now. I also agree that isInstanceOf and pattern matching should behave the same way. As to what is the most useful way, I am not sure. Given the trap with Settings, maybe it would be most useful to assume Outer#Inner for any match against Inner, except if an outer instance is given eplicitly.

Pro: This seems to be what one wants most of the time.
Contra: It introduces an irregularity, since normally the identifier Tpe' means this.Tpe'.

What do people think?

Thanks

-- Martin

[scabug]

@paulp said:
My impression is that using # in types is one of the least prominent areas of the language, widening this trap to infinity for any who might not yet have come across them. Between this and the fact that matching again the inner class is almost always going to be what you want (and if it wasn't what you wanted, would you really assume matching against the inner name would actually discriminate based on the outer name? I know I wouldn't...) I vote for inconsistency.

Perhaps some metatheory is out there which can capture the inconsistency in a consistent way, a la newtonian mechanics giving way to relativity.

[scabug]

@paulp said:
As this patch is now in trunk I am closing this ticket and have created #1698 to document the underlying issue.