[EPIC] Mejoras al parser para los validadores

[fdodino]

En este PR y en el #196 agregamos el atributo expectedOn para los tests. Nos queda pendiente mejorar un poco el parser para ser más específico en los errores que tiramos y algunas cosas que arreglar. Lo anoto todo acá y en todo caso podemos dividir esta épica en tickets separados.

Cosas para arreglar

• shouldHaveAssertInTest.wlk: quedaron dos tests con Enters porque el método sanitizeWhitespaces no está abarcando ese nodo
• possiblyReturningBlock.wlk: también, tiene Enters de más e incluso el comment de la línea siguiente. Hay una función sourceMapForReturnValue comentada porque el nodo donde se anota el @Expected termina después que el nodo que devuelve el source map:

  1) Wollok Validations
       possiblyReturningBlock:
     AssertionError [ERR_ASSERTION]: File contains errors: possiblyReturningBlock at [12:25, 16:3]. @Expect: [12:25, 16:3] Node: Method - [12:3, 16:3]
      at /home/dodain/workspace/wollok-dev/wollok-ts/test/validator.test.ts:62:17
      at /home/dodain/workspace/wollok-dev/wollok-ts/src/model.ts:166:7

Mejoras

• lugares donde estaría bueno poder separar la parte de definición de una entidad y dejar afuera el body, por ejemplo linearizationShouldNotRepeatNamedArguments:

  @Expect(code="linearizationShouldNotRepeatNamedArguments", level="warning", expectedOn="object inherits SomeClass(someAttribute = 0, someAttribute = 1)")
  object inherits SomeClass(someAttribute = 0, someAttribute = 1) {
     // cositas
  }

Pero para eso necesitamos saber dónde arranca el body (tenemos las sentences pero qué pasa si no tenemos sentences, como es justo este caso en realidad).

Lo mismo para methodShouldHaveDifferentSignature, onlyLastParameterCanBeVarArg y shouldUseOverrideKeyword. Y en esa misma lógica:

• superclassShouldBeLastInLinearization
• shouldNotHaveLoopInHierarchy => no tengo todavía forma pero me gustaría cortar hasta la definición de la clase, antes de los {

Ej: class A inherits D <= cortar acá... y que no entre { ...

• shouldCatchUsingExceptionHierarchy => marcar el catch hasta el body
• shouldImplementAllMethodsInHierarchy, shouldImplementInheritedAbstractMethods => sacar el body, el tema es que members si es vacío no tenés nada para agarrarte. Va un ejemplo con lo que imagino para expectedOn.

@Expect(code = "shouldImplementAllMethodsInHierarchy", level = "error", expectedOn="class TomatoWithNameBelowSuper inherits Doctor")
class TomatoWithNameBelowSuper inherits Doctor {
	override method name() = "23"
}

Cosas que no se si se pueden mejorar

• shouldNotBeEmpty -> subrayar un try/catch vacío o bien un test vacío es difícil, lo dejé para que marque todo el nodo, no se qué se podría mejorar acá:

  method methodWithEmptyTryCatch() {
    try @Expect(code="shouldNotBeEmpty", level="warning") {
    } catch e @Expect(code="shouldNotBeEmpty", level="warning") {
    }
  }

• shouldNotDefineMoreThanOneSuperclass => el ejemplo sería

class A inherits B and C

pero resulta que el modelo no permite tener muchas superclases, solamente una, y cuando ves el sourceMap de la superclase apunta a donde está definido, así que a menos de que tengamos el texto no podemos jugar demasiado (yo probaría ponerle el texto y después buscar desde "inherits" hasta el "{" sacando los espacios que es donde estaría la definición de las superclases, pero es un poco tricky).

• shouldInitializeInheritedAttributes => si no hay inicializadores es muy difícil mostrar algo, al menos ahora chequea por los valores y se marca todo el nodo, se podría mostrar lo que está entre paréntesis, pero bueno, si no tiene paréntesis marcar todo el nodo del object como el primer caso:

@Expect(code="shouldInitializeInheritedAttributes", level="error", values=["w, y"])
object o1 inherits C {}
object o2 inherits C(w = 1, x = 2, y = 3, z = 4) {} // OK
@Expect(code="shouldInitializeInheritedAttributes", level="error", values=["w"])
object o3 inherits C(x = 2, y = 3, z = 4) {}

Y los últimos dos casos de shouldUseConditionalExpression marcan solo el primer if de la condición:

  method esPar6(n) {
    @Expect(code = "shouldUseConditionalExpression", level="warning", expectedOn="if (n % 2 == 0) {
      return true
    }")
    if (n % 2 == 0) {
      return true
    }
    return false
  }

El otro es similar pero con false. Acá habría que arreglarlo haciendo que el sourceMap incluya la segunda sentencia también, pero hay que manejarlo bastante caseramente.

Tip

Tener instalada la extensión show-offset de Ramya Rao A