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

Polymorphic method type #1889

Merged
merged 18 commits into from
Jun 8, 2020
Merged

Polymorphic method type #1889

merged 18 commits into from
Jun 8, 2020

Conversation

PalumboN
Copy link
Contributor

@PalumboN PalumboN commented May 8, 2020

Fix #1770
Fix #1745
Fix #1735
Fix #1526
Fix #1507
Fix #1478

Este PR soluciona problemas con la inferencia, en donde el uso de un método pesaba más que su definición y producía que se infiera un tipo más acotado para los parámetros.
Ahora los tipos de los métodos se infieren a partir de su uso, buscando el tipo más amplio.
El buscar el tipo más amplio, me llevó a inferencias del tipo (Number|String) que se esperaban que dieran error. Así que metí el concepto de tipos compatibles para ver en qué casos hacer union y en qué casos fallar.
Pueden ver los tests para más detalles

var property x = obj1
}
'''.parseAndInfer.asserting [
// FIXME
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Muy bueno!!! 🥇

@PalumboN
Copy link
Contributor Author

PalumboN commented May 8, 2020

@fdodino me está fallando el build por algo de Xpect, sabés qué onda?

[ERROR] Failed to resolve target definition /home/travis/build/uqbar-project/wollok/org.uqbar.project.wollok.targetplatform/org.uqbar.project.wollok.targetplatform.target: Failed to load p2 metadata repository from location https://ci.eclipse.org/xpect/job/Xpect/job/master/lastSuccessfulBuild/artifact/org.eclipse.xpect.releng/p2-repository/target/repository/: No repository found at https://ci.eclipse.org/xpect/job/Xpect/job/master/lastSuccessfulBuild/artifact/org.eclipse.xpect.releng/p2-repository/target/repository. -> [Help 1]

Copy link
Collaborator

@fdodino fdodino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miré principalmente los tests, me encantó lo que vi respecto a lo que detecta el type system.
El resto del código lo miré por arriba, se puede mergear una vez resueltos los conflictos.
Vamoooo' Nahue que el type system es un caño!!! 👏 👏 👏

var property x = obj1
}
'''.parseAndInfer.asserting [
// FIXME
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Muy bueno!!! 🥇

@@ -27,10 +27,10 @@ program p {
// XPECT type at c6 --> {() => Number}
const c6 = { if (bool) return 1 else return 2 }

// XPECT type at c7 --> {() => (Number|String)}
// XPECT type at c7 --> {() => Number}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acá no entendí si corregiste el type system para que tire error cuando hacés

if (bool) { return 1 }
return "2"

lo cual me parece razonable y piola.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sí, exactamente eso, porque Number y String ahora no son "compatibles". Esta es una regla media arbitraria, pero había varios tests en donde se esperaba este comportamiento, y no me parecían mal. Ahora el TS es más restrictivo, pero no creo que haya casos así en pdep/obj1 y que no impliquen un "error de diseño".

@coveralls
Copy link

coveralls commented Jun 7, 2020

Coverage Status

Coverage increased (+0.2%) to 86.151% when pulling 3170e08 on polymorphic-method-type into 8dc8bc4 on dev.

@PalumboN PalumboN merged commit f7f3f84 into dev Jun 8, 2020
@PalumboN PalumboN deleted the polymorphic-method-type branch June 8, 2020 18:27
@fdodino fdodino added this to the Wollok 2.0 I milestone Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants