Fix #1771 - inconsistencias en list.asSet() #1786

nicovio · 2019-09-11T08:07:22Z

Cambios:

asSet() deja de ser native y withoutDuplicates() ya no lo usa.
Agrego tests para asSet() y withoutDuplicates()

Después de arreglar las inconsistencias relativas al issue, note este caso que seguía pasando:

Cuando el set era de distintos tipos, cada tanto se "comía" un duplicado (siempre eran Strings y Dates).

Lo pude solucionar con un cambio en el método compare() de WollokObjectComparator. Hay que revisarlo.

coveralls · 2019-09-11T08:19:09Z

Coverage increased (+0.3%) to 89.789% when pulling 68a628f on fix-#1771-inconsistencias-en-list.asSet into 694f874 on dev.

fdodino · 2019-09-11T20:29:35Z

@nicovio en general suena bien, yo te pido que hagas un chequeo extra porque me preocupa que no se degrade la performance. Fijate este proyecto

https://github.com/wollok/test-performance-set--1370

Bajátelo y corré los tests, que generan sets y lists bastante voluminosos, los métodos no deberían tardar mucho (en general < 1seg, algunos sí tardan y están ok, fijate este PR que dice más o menos lo que se esperaría, por más que solo tengas el total de lo que corrió es fácil ver si se fue al pasto o no)

nicovio · 2019-09-11T21:52:03Z

@nicovio en general suena bien, yo te pido que hagas un chequeo extra porque me preocupa que no se degrade la performance. Fijate este proyecto

https://github.com/wollok/test-performance-set--1370

Bajátelo y corré los tests, que generan sets y lists bastante voluminosos, los métodos no deberían tardar mucho (en general < 1seg, algunos sí tardan y están ok, fijate este PR que dice más o menos lo que se esperaría, por más que solo tengas el total de lo que corrió es fácil ver si se fue al pasto o no)

Ok lo voy a ver. Gracias Dodain! ✌️

nicovio · 2019-09-12T20:49:14Z

@fdodino estuve corriendo los tests de Set que estan acá https://github.com/wollok/test-performance-set--1370 y compare los tiempos con los que pusiste en #1370.

Algunos tardaron bastante más, así que me fije como daban antes de los cambios de este PR y las diferencias fueron las mismas.

Dejo los tiempos totales que se esperaban vs los que dieron.

Pruebas hechas con Set

Números

Esperado: 4502 ms
Resultado: 4361 ms

Objetos definidos por el usuario sin == ni >

Esperado: 1740 ms
Resultado: 2697 ms

Objetos definidos por el usuario (redefiniendo == y >)

Esperado: 6692 ms
Resultado: 6812 ms

Strings

Esperado: 1548 ms
Resultado: 2328 ms

Dates

Esperado: 1036 ms
Resultado: 2209 ms

Booleans

Esperado: 53 ms
Resultado: 791 ms

fdodino · 2019-09-13T02:22:46Z

Pongo acá lo que hablamos recién, puede ser una diferencia en la potencia de las máquinas, no me preocuparía por la performance.

npasserini

Impresionante laburo!!!
Pido disculpas por los muchos comentarios pero es un lugar delicado y ya tuvimos algunas idas y vueltas, propongo que nos tomemos la paciencia de dejarlo bien de una vez por todas.

npasserini · 2019-09-13T02:56:14Z

org.uqbar.project.wollok.lib/src/wollok/lang.wlk

@@ -922,13 +922,13 @@ class Set inherits Collection {
 	}

 	/**
-	 * Returns a new copy of current Set.
+	 * Converts an object to a Set. No effect on Sets.


Es un poco quisquilloso pero deberíamos evitar la palabra effect que tiene un significado bastante específico en nuestro discurso. De paso, no creo que convierta an object to a set, vale sólo para colecciones, de hecho convierte this.

Entonces propongo "Converts this collection into a set. If this collection is already a set, it returns a copy of it."

+1 a tu propuesta.

Otra cosa: Esto estaba puesto en el asSet() de Set, pero creo que debería estar en el de Collection.

Suena lógico. @fdodino tenemos una política sobre wollokdoc en métodos que son overrides?

npasserini · 2019-09-13T02:58:24Z

org.uqbar.project.wollok.lib/src/wollok/lang.wlk

 	 */
-	override method asSet() native
+	override method asSet() = self


Acá cambió el comportamiento y no coincide con el comentario de arriba. Esta implementación no está bien en Collection sólo valdría para Set. Francamente me gustaba la idea de que el asSet devolviera una copia, pero si prefieren evitarlo no está mal, pero deberíamos:

mover esta implementación a Set

dejar este método abstracto (y chequear que otras colecciones lo implementen).

corregir el wollokdoc.

Esta implementación es la de Set y en Collection es abstracto.

Me parece que se genera una confusión porque esta mal la documentación y en el asSet() de Setdice lo que debería decir en el de Collection.

Ah, mala mía. Igualmente me gustaba de la implementación nativa que devolvía un set nuevo en lugar de self. @fdodino @PalumboN @lspigariol qué opinan?

npasserini · 2019-09-13T03:00:35Z

org.uqbar.project.wollok.lib/src/wollok/lang.wlk

@@ -1402,7 +1406,7 @@ class List inherits Collection {
 	 * [1, 3, 1, 5, 1, 3, 2, 5].withoutDuplicates() => Answers [1, 2, 3, 5]


Este ejemplo está mal, está cambiando el orden y es justamente lo que no queremos.
De hecho estaría bueno agregar ese detalle en el comentario.

Estoy de acuerdo con vos.

org.uqbar.project.wollok.lib/src/wollok/lang/WList.xtend

npasserini · 2019-09-13T03:10:43Z

org.uqbar.project.wollok.lib/src/wollok/lang/WollokObjectComparator.xtend

@@ -34,7 +34,7 @@ class WollokObjectComparator implements Comparator<WollokObject> {
 			val comparator = comparisonsStrategy.get(o1.kind.fqn) ?: new WollokObjectEqualsComparator		
 			return comparator.compare(o1, o2)
 		} catch (RuntimeException e) {
-			return o1.hashCode.compareTo(o2.hashCode)
+			return (o1.kind.hashCode).compareTo(o2.kind.hashCode)


No me convence lo de atrapar RuntimeException aca. ¿No podríamos estar capturando cualquier excepción inesperada y ocultándola?

Igual entiendo que este no es un problema de este PR, y de hecho esta línea me parece mejor que la anterior, al menos respeta la relación de orden que el comparator debería definir. Si quieren levantamos otro ticket para revisar esto.

Ahora no recuerdo por qué puse el catch, @nicovio , te acordás por qué pusiste el o1.kind.hashCode en lugar de o1.hashCode, ¿en qué casos salta el RuntimeException? Es lo único que me da cosita sacar. Y sí, si esto traba el PR generemos otro ticket.

El RuntimeException salta cuando o1 y o2 son de distintos tipos.

Puse el o1.kind.hashCode porque estaban andando mal los sets con elementos de distintos tipos, a veces aceptaba duplicados (pasaba con strings y dates).

Ah, cierto!

O sea, o1 y o2 siempre son WollokObject, pero vos me entendiste jaja (creo).

Yo entiendo dos cosas:

Que el error que se espera atrapar es algún DNU producto de que los objetos se intentan comparar entre ellos por igualdad y si son de distinto tipo y ellos mismos no lo contemplan, posiblemente uno le termina mandando un mensaje a otro que no entiende. Ejemplo:

class Pair {
var x,y
method equals(other) = x == other.x() and y == other.y()
}

Si hacés new Pair(x=1,y=2).equals( "ola") => DNU "hola no entiende x".

A mí me gusta que se intente capturar ese caso. Capturarlo con una excepción podría ser materia de discusión, pero la alternativa es preguntar por la clase y puede tener sus propios problemas, da para una discusión larga.

El tema acá es que también estamos atrapando otros errores e ignorándolos, eso me parece más peligroso. Pero no lo discutamos acá, creo un issue para ver eso.

La segunda es que usar el hash para el orden es peligroso porque no establece una relación de orden, ergo, podría pasar que a > b > c > a si b y c pueden compararse entre sí, pero no con a.

Entiendo que eso se evita si se usa el hash de la clase, asumiendo que si b y c son comparables, entonces son de la misma clase y entonces el kind.hash es el mismo.

Siendo estricto, esto funciona si

para todo b y c comparables (es decir, comparator.compare(o1, o2) no tira excepción), b.kind == c.kind

Creo que a la larga esto medio define la cuestión anterior y debemos agregar:
a. Un if o1.kind == o2.kind antes de intentar el compare, y eso nos exime de atrapar excepciones.
b. Definir la regla "si dos objetos tienen mismo kind deben ser comparables sin tirar excepción" y documentarla.
c. De yapa, no usar kind.hashCode y en cambio comparar los nombres de los kinds entre sí. Esto es un chiche pero nos daría un ordenamiento predecible entre objetos de distinto kind. Que es un ordenamiento arbitrario (alfabético, bah) pero menos caótico que ordenar por hash y que sería consistente entre diferentes ejecuciones.

Esto también obliga a tener claro cuándo es que puede pasar que dos objetos tengan el mismo kind, que asumo que no es lo mismo que tener "la misma clase", porque para eso se inventó el concepto, no?

No sé, si les parece podríamos meterle el fix que decía y dar por resuelto el #1791

npasserini · 2019-09-13T03:11:51Z

org.uqbar.project.wollok.tests/src/org/uqbar/project/wollok/tests/sdk/ListTest.xtend

+	@Test
+	def void testListOfStringAsSetConversion() {
+		'''
+		assert.equals(1, ["hola","hola"].asSet().size())


Me gustaría probar con dos formas de construir el mismo string, onda "ho" + "la" porque si lo construis las dos veces de la misma manera podría terminar siendo efectivamente "el mismo" y no estás testeando lo que queríamos verificar.

Lo mismo vale para los demás tipos de datos.

npasserini · 2019-09-13T03:12:36Z

org.uqbar.project.wollok.tests/src/org/uqbar/project/wollok/tests/sdk/ListTest.xtend

+	def void testListOfDictionaryAsSetConversion() {
+		'''
+		const dictionary = new Dictionary()
+		assert.equals(1, [dictionary,dictionary].asSet().size())


Acá efectivamente es el mismo diccionario, tenemos que probarlo creando dos dictionaries.

Hay un problema. Si creo dos dictionaries los considera distintos y esto [dictionary,otherDictionary].asSet().size() da 2.

¿Hay que redefinir el equals de Dictionary?

Es posible!
Igual antes de llegar a esa conclusión haría el test más unitario posible assert.equals(new Dictionary(), new Dictionary()) y vería qué pasa.

npasserini · 2019-09-13T03:12:46Z

org.uqbar.project.wollok.tests/src/org/uqbar/project/wollok/tests/sdk/ListTest.xtend

+	def void testListOfPairAsSetConversion() {
+		'''
+		const pair = new Pair(1,2)
+		assert.equals(1, [pair,pair].asSet().size())


Idem, hay que hacer el new dos veces.

Pasa lo mismo que con los dictionaries.

npasserini · 2019-09-13T03:13:37Z

org.uqbar.project.wollok.tests/src/org/uqbar/project/wollok/tests/sdk/ListTest.xtend

+		class MiClase {}
+		program a {
+			const miClase = new MiClase()
+			assert.equals(1, [miClase,miClase].asSet().size())


Hacer new dos veces!
Para que esto tenga sentido MiClase debería implementar equals.

Acá hay otro tema para ver.

Este test falla:

class MiClase { override method equals(other) = return true } program a { const a = new MiClase() const b = new MiClase() assert.equals(1, [a,b].asSet().size()) }

org.uqbar.project.wollok.tests.interpreter.WollokComparisonFailure: Expected [1] but found [2]

Ahora que veo todo, creo que este es el test más fácil y más representativo. Esto debería andar y si no anda es que la implementación de asSet está mal y debemos cambiarla.

Al hacerlo, deberíamos escuchar los comentarios de @fdodino sobre las cuestiones de performance en las que estuvo trabajando, pero creo que vos ya estás al tanto de eso más que yo.

npasserini · 2019-09-13T03:14:41Z

org.uqbar.project.wollok.tests/src/org/uqbar/project/wollok/tests/sdk/ListTest.xtend

+		list.add(true)
+		list.add(1)
+		list.add("hola")
+		assert.equals(3, list.withoutDuplicates().size())


Falta acá hacer el assert del orden.

De acuerdo.

nicovio · 2019-09-13T04:26:08Z

Impresionante laburo!!!
Pido disculpas por los muchos comentarios pero es un lugar delicado y ya tuvimos algunas idas y vueltas, propongo que nos tomemos la paciencia de dejarlo bien de una vez por todas.

No me molestan tantos comentarios. Al contrario, me sirven para aprender.

Yo pido disculpas si me mando muchas cagadas.

fdodino · 2019-09-14T21:09:47Z

@npasserini @nicovio

Ahí estuvimos con los muchachos cambiando la lógica, básicamente:

chequeamos que en la jerarquía del objeto no haya un equals o un == (que las definiciones sean únicamente de wollok.lang.Object). Esto está acoplado a la biblioteca, habrá que ver si el día de mañana redefinimos un == o un equals
en caso de tener un == o un equals en la jerarquía (ya sea en la clase/wko donde estoy parado o bien en alguna de las clases padres antes de Object), llamo al equals primero que a su vez por defecto delega en el == en Object. Eso nos asegura que si redefinen el equals, se use el equals y si redefinen el == llegue por rebote.

Agregué 4 tests.

Lo que sí pasa es que si definís esto:

		class Animal {
			override method ==(other) = true
		}
		
		class Perro inherits Animal {
		}
		
		class Gato inherits Animal {
			
		}

cuando tenés

#{ new Gato(), new Perro(), "hola" }.size()  // me da 2
#{ "hola", new Gato(), new Perro() }.size()  // me da 1

pero como dice @nscarcella , estamos definiendo un equals que no es conmutativo, con lo cual recontrahipersupermegaarchiexcede el alcance de este issue.

nicovio · 2019-09-14T21:42:06Z

Buenísimo, más tarde subo las correcciones del review de @npasserini.

npasserini · 2019-09-14T22:16:41Z

@npasserini @nicovio

Ahí estuvimos con los muchachos cambiando la lógica, básicamente:

chequeamos que en la jerarquía del objeto no haya un equals o un == (que las definiciones sean únicamente de wollok.lang.Object). Esto está acoplado a la biblioteca, habrá que ver si el día de mañana redefinimos un == o un equals

en caso de tener un == o un equals en la jerarquía (ya sea en la clase/wko donde estoy parado o bien en alguna de las clases padres antes de Object), llamo al equals primero que a su vez por defecto delega en el == en Object. Eso nos asegura que si redefinen el equals, se use el equals y si redefinen el == llegue por rebote.

Agregué 4 tests.

Lo que sí pasa es que si definís esto:
		class Animal {
			override method ==(other) = true
		}
		
		class Perro inherits Animal {
		}
		
		class Gato inherits Animal {
			
		}
cuando tenés
#{ new Gato(), new Perro(), "hola" }.size()  // me da 2
#{ "hola", new Gato(), new Perro() }.size()  // me da 1
pero como dice @nscarcella , estamos definiendo un equals que no es conmutativo, con lo cual recontrahipersupermegaarchiexcede el alcance de este issue.

Tenés razón, esa implementación de equals es incorrecta, al menos en una colección que tiene cosas que no son animales. Entiendo que si restringís la colección solo a animales ahí sí es conmutativa y todo debería funcionar de acuerdo a lo esperado.

Estaría bueno agregar esa regla en el comentario del equals, onda "Si redefinís esto asegurate de hacerlo conmutativo.".

Ahora... no sé cómo lo implementarías bien si no es preguntando el nombre de la clase, pero bueno, eso ya es otro tema.

fdodino · 2019-09-14T22:40:20Z

Jejeje, lo mismo dijo NicoS

fdodino · 2019-09-14T22:41:03Z

tiré kind pero es isAssignableFrom lo que necesitaríamos

clombardi · 2019-09-16T14:10:33Z

Gente, socorro. Mañana toca (en UNAHur) hablar del asSet() y justo el ejemplo es con una lista de Strings.
Y auch, en el set aparece dos veces el mismo String. Imagino que es porque no es el mismo String, obsérvese lo siguiente:

>>> var a = "hola"
>>> var strs = [a, "que", a, "tal"]
>>> strs.asSet()
#{"hola", "que", "tal"}
>>> var strs2 = ["hola", "que", "hola", "tal"]
>>> strs2.asSet()
#{"hola", "que", "hola", "tal"}

Para mayor felicidad, da la clase un docente nuevo, y yo no estoy en Buenos Aires.
Yo estoy usando Wollok 1.8.5. ¿Hay alguna versión posterior en la que esto esté arreglado?

Graciaaaas por cualquier cosa que me puedan decir.

lspigariol · 2019-09-16T14:27:09Z

(mi respuesta atrasa unos cuantos mensajes). Lo unico que puedo decir para mantener la consistencia es que en Set asSet no devuelva self sino un nuevo set. El lun., 16 sept. 2019 a las 11:10, Carlos Lombardi (< notifications@github.com>) escribió:

…

Gente, socorro. Mañana toca (en UNAHur) hablar del asSet() y justo el ejemplo es con una lista de Strings. Y auch, en el set aparece dos veces el mismo String. Imagino que es porque no es *el mismo* String, obsérvese lo siguiente: >>> var a = "hola" >>> var strs = [a, "que", a, "tal"] >>> strs.asSet() #{"hola", "que", "tal"} >>> var strs2 = ["hola", "que", "hola", "tal"] >>> strs2.asSet() #{"hola", "que", "hola", "tal"} Para mayor felicidad, da la clase un docente nuevo, y yo no estoy en Buenos Aires. Yo estoy usando Wollok 1.8.5. ¿Hay alguna versión posterior en la que esto esté arreglado? Graciaaaas por cualquier cosa que me puedan decir. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1786?email_source=notifications&email_token=ACZRXG3MYXL2CX2PUQE53BTQJ6HVVA5CNFSM4IVQURO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6ZIVBA#issuecomment-531794564>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACZRXG3BZ6UH3ZX7IM7X7S3QJ6HVVANCNFSM4IVQUROQ> .

nicovio · 2019-09-21T00:17:09Z

Hola @fdodino, tengo una mala noticia... parecía que estaba solucionado el problema con sets de listas y sets de sets, pero no.

el problema para mi está en este método:

def compareGreaterThan(WollokObject o1, WollokObject o2) {
	if (o1.hasGreaterThanMethod) {
		 if (o1.wollokGreaterThan(o2)) 1 else -1
	} else 1
}

como Collection no tiene greaterThan, acá devuelve siempre 1

Ahí cambie los tests para que fallen.

fdodino · 2019-09-21T03:13:51Z

hola @nicovio, ok, mañana lo miro. Una cosa que yo haría por si querés probar:

def compareGreaterThan(WollokObject o1, WollokObject o2) {
	if (o1.hasGreaterThanMethod) {
		 if (o1.wollokGreaterThan(o2)) 1 else -1
	} else {
                 // --->
                 if (o1.hashCode > o2.hashCode) 1 else -1
                 // <----
        }
}

nicovio · 2019-09-21T04:00:14Z

esto no anduvo:

def compareGreaterThan(WollokObject o1, WollokObject o2) {
	if (o1.hasGreaterThanMethod) {
		 if (o1.wollokGreaterThan(o2)) 1 else -1
	} else {
                 // --->
                 if (o1.hashCode > o2.hashCode) 1 else -1
                 // <----
        }
}

esto si:

def compareGreaterThan(WollokObject o1, WollokObject o2) {
	if (o1.hasGreaterThanMethod) {
		 if (o1.wollokGreaterThan(o2)) 1 else -1
	} else {
                 // --->
                 if (o1.toString > o2.toString) 1 else -1
                 // <----
        }
}

npasserini · 2019-09-21T04:18:14Z

No sé si tienen sentido ninguno de los dos. No definen relaciones de orden. Fundamentalmente si los objetos son de distinto tipo. El sáb., 21 de sep. de 2019 a la(s) 01:00, Nicolas Viotti ( notifications@github.com) escribió:

…

if (o1.hashCode > o2.hashCode) 1 else -1 no anduvo esto anda: if (o1.toString > o2.toString) 1 else -1 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1786?email_source=notifications&email_token=ABDLKOISF6IEMP53BOJMMJDQKWL47A5CNFSM4IVQURO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7IJYQA#issuecomment-533765184>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDLKONPZ75JDZNEBZHSM2LQKWL47ANCNFSM4IVQUROQ> .

fdodino · 2019-09-21T12:30:32Z

No tiene sentido, por eso mi propuesta inicial era devolver 1 como el mínimo cálculo posible. Pero seguramente se me pasó alguna letra chica del TreeSet y por eso el test que falla. Si corrige los tests, mi propuesta es pushearlo: ahora tenemos un montón de tests verdes más, solucionamos el problema original, no hay duplicados triviales para números, strings, booleanos, fechas, que es lo que el 96,89% de las veces vamos a usar, y démosle para adelante...

npasserini · 2019-09-21T13:03:29Z

Sí, tiene sentido lo que decís. Esta tarde/noche tengo un rato para Wollok, dejame ver si puedo darle la última vueltita para que quede 100%. Propongo que aguantemos el finde así metemos eso y tal vez algo más. Domingo a ultima hora o lunes a primera sale release. El sáb., 21 de sep. de 2019 a la(s) 09:30, Fernando Dodino ( notifications@github.com) escribió:

…

No tiene sentido, por eso mi propuesta inicial era devolver 1 como el mínimo cálculo posible. Pero seguramente se me pasó alguna letra chica del TreeSet y por eso el test que falla. Si corrige los tests, mi propuesta es pushearlo: ahora tenemos un montón de tests verdes más, solucionamos el problema original, no hay duplicados triviales para números, strings, booleanos, fechas, que es lo que el 96,89% de las veces vamos a usar, y démosle para adelante... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1786?email_source=notifications&email_token=ABDLKOIERL37XHOHTUAJ6NLQKYHWTA5CNFSM4IVQURO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7IQ4YQ#issuecomment-533794402>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDLKOJEQGYPWSNIL424NDTQKYHWTANCNFSM4IVQUROQ> .

fdodino · 2019-09-21T13:07:47Z

Dale, perfecto!!

npasserini · 2019-09-22T02:54:15Z

Bueno, tengo un caso que falla: class Figura { method superficie() override method equals(other) = self.superficie() == other.superficie() } class Triangulo inherits Figura { var property base var property altura override method superficie() = base * altura / 2 } class Cuadrado inherits Figura { var property lado override method superficie() = lado * lado } Wollok interactive console (type "quit" to quit):