Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enumerations Sometimes Fail To Correctly Populate The Name of Values #5147

Closed
scabug opened this issue Nov 4, 2011 · 10 comments · Fixed by scala/scala#8146
Closed

Enumerations Sometimes Fail To Correctly Populate The Name of Values #5147

scabug opened this issue Nov 4, 2011 · 10 comments · Fixed by scala/scala#8146
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Nov 4, 2011

If you have a class that extends Enumeration that defines the actual Enumeration and then have a companion object extends from that class, the string values of the enumeration values is improperly handled. For example:

class Foo extends Enumeration {
  val A = Value
}
object Foo extends Foo

...and then you try to print Foo.A, it'll print "<Invalid enum: no field for #0>" rather than "A".

@scabug
Copy link
Author

scabug commented Nov 4, 2011

Imported From: https://issues.scala-lang.org/browse/SI-5147?orig=1
Reporter: Turing Eret (turingeret)
Affected Versions: 2.9.1
See #5211

@scabug
Copy link
Author

scabug commented Nov 4, 2011

@soc said:
Can't we just get rid of the whole class and think about a better way to provide this functionality?

This class has been bug-ridden since I started to work with Scala and has a highly disproportional ratio of LOC/bugs. It is not even used in Scala at all except one occurrence in Scala Swing afaik.

Most code in the wild doesn't even bother using it and takes the expensive route of

abstract class Foo
object Foo1 
object Foo2
... 

instead.

I don't have any hopes left that this functionality doesn't keep breaking like it did in the past.

Maybe it would be better to tell people to define enums in Java instead (as it is already done for runtime-visible annotations) and deprecate/remove this class?

@scabug
Copy link
Author

scabug commented Nov 4, 2011

@paulp said:
"It is not even used in Scala at all except one occurrence in Scala Swing afaik."

Without necessarily disagreeing with the idea that Enumeration should be marked for death:

  object State extends Enumeration {
object Contexts extends Enumeration {
  object InfoLevel extends Enumeration {
    object InfixMode extends Enumeration {
    object NonPublicRefs extends Enumeration {
  // object AccessMode extends Enumeration("AccessMode") {
  object severity extends Enumeration
  object Level extends Enumeration {
 *    object WeekDay extends Enumeration {
  object RoundingMode extends Enumeration(java.math.RoundingMode.values map (_.toString) : _*) with Serializable {
object Modifier extends Enumeration {
  object Category extends Enumeration {
object Alignment extends Enumeration {
  object Position extends Enumeration {
object Key extends Enumeration {
  object Location extends Enumeration {
  object Result extends Enumeration {
  object SelectionMode extends Enumeration {
  object Alignment extends Enumeration {
  object FocusLostBehavior extends Enumeration {
  object Fill extends Enumeration {
  object Anchor extends Enumeration {
  object IntervalMode extends Enumeration {
object Orientation extends Enumeration {
  object Message extends Enumeration {
  object Options extends Enumeration {
  object Result extends Enumeration {
  object BarPolicy extends Enumeration {
  object Layout extends Enumeration {
  object AutoResizeMode extends Enumeration {
  object IntervalMode extends Enumeration {
  object ElementMode extends Enumeration {
  object FocusLostBehavior extends Enumeration {

@scabug
Copy link
Author

scabug commented Nov 4, 2011

Turing Eret (turingeret) said:
So, it looks like the culprit was a fix for #4570 (specifically commit 679d803402c44c9fcb47eba732d997280345667f). In that fix, Paul made these changes to src/library/scala/Enumeration.scala:

@@ -157,10 +157,14 @@ abstract class Enumeration(initial: Int, names: String*) extends Serializable {
   protected final def Value(i: Int, name: String): Value = new Val(i, name)
 
   private def populateNameMap() {
+    val fields = getClass.getDeclaredFields
+    def isValDef(m: JMethod) = fields exists (fd => fd.getName == m.getName && fd.getType == m.getReturnType)
+
     // The list of possible Value methods: 0-args which return a conforming type
     val methods = getClass.getMethods filter (m => m.getParameterTypes.isEmpty &&
                                                    classOf[Value].isAssignableFrom(m.getReturnType) &&
-                                                   m.getDeclaringClass != classOf[Enumeration])
+                                                   m.getDeclaringClass != classOf[Enumeration] &&
+                                                   isValDef(m))

The problem is that isValDef only checks to see if it is a val defined in the current class. If you extends from a class where the actual enumeration value is defined, then it won't find it.

The following diff seems to fix this issue:

diff --git a/src/library/scala/Enumeration.scala b/src/library/scala/Enumeration.scala
index 45cb3a6..30bb274 100644
--- a/src/library/scala/Enumeration.scala
+++ b/src/library/scala/Enumeration.scala
@@ -161,7 +161,10 @@ abstract class Enumeration(initial: Int, names: String*) extends Serializable {
   protected final def Value(i: Int, name: String): Value = new Val(i, name)

   private def populateNameMap() {
-    val fields = getClass.getDeclaredFields
+    def getFields(clazz: Class[_]) : Array[JField] = {
+      clazz.getDeclaredFields ++ (if(clazz.getSuperclass != null) getFields(clazz.getSuperclass) else Nil)
+    }
+    val fields = getFields(getClass)
     def isValDef(m: JMethod) = fields exists (fd => fd.getName == m.getName && fd.getType == m.getReturnType)

     // The list of possible Value methods: 0-args which return a conforming type
diff --git a/test/files/run/t5147.check b/test/files/run/t5147.check
new file mode 100644
index 0000000..f70f10e
--- /dev/null
+++ b/test/files/run/t5147.check
@@ -0,0 +1 @@
+A
diff --git a/test/files/run/t5147.scala b/test/files/run/t5147.scala
new file mode 100644
index 0000000..6261336
--- /dev/null
+++ b/test/files/run/t5147.scala
@@ -0,0 +1,9 @@
+class Test extends Enumeration {
+  val A = Value
+}
+object Test extends Test
+object Test5147 {
+  def main(args: Array[String]): Unit = {
+    println(Test.A)
+  }
+}

@scabug
Copy link
Author

scabug commented Nov 4, 2011

Commit Message Bot (anonymous) said:
(extempore in r25943) Fix for Enumeration.

Closes #5147.

@scabug
Copy link
Author

scabug commented Nov 4, 2011

@paulp said:
Oops, I just checked in some code accidentally. Please ignore the man behind the curtain.

@scabug
Copy link
Author

scabug commented Nov 4, 2011

Commit Message Bot (anonymous) said:
(extempore in r25946) Revert "Fix for Enumeration."

Oops, didn't mean to commit that one. Opens #5147. No review.

@scabug
Copy link
Author

scabug commented Nov 5, 2011

Turing Eret (turingeret) said:
Did you mean to reopen this, Paul?

@scabug
Copy link
Author

scabug commented Nov 5, 2011

@paulp said:
Yeah, I had no idea whether the commit bot would act on "opens" but I figured I'd give it a shot.

@scabug
Copy link
Author

scabug commented Nov 7, 2011

Trond Olsen (tolsen77) said:
The Closed Collector: Bring out yer closed.
[a man puts [SI-4805] on the heap with SI-3615 SI-3815 SI-3186 SI-2214 SI-4334 SI-1505 SI-3134 SI-3687 SI-4570 SI-3616 SI-3817 SI-2827 SI-2111 SI-3719 SI-2211 SI-4333 SI-3314 SI-4045 SI-1496 SI-4571 SI-3174 SI-3950 SI-2527 SI-5117 SI-1849 SI-4292 SI-1089 SI-4236 SI-2933 SI-1550]
Large Man with Closed [SI-5147]: Here's one.
The Closed Collector: That'll be ninepence.
The Closed [SI-5147] That Claims It Isn't: I'm not closed.
The Closed Collector: What?
Large Man with Closed [SI-5147]: Nothing. There's your ninepence.

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

Successfully merging a pull request may close this issue.

4 participants