-
Notifications
You must be signed in to change notification settings - Fork 16
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
Polymorphic method type #1889
Conversation
var property x = obj1 | ||
} | ||
'''.parseAndInfer.asserting [ | ||
// FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muy bueno!!! 🥇
@fdodino me está fallando el build por algo de
|
There was a problem hiding this 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 |
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
...ypeSystem/src/org/uqbar/project/wollok/typesystem/constraints/types/CompatibilityTypes.xtend
Outdated
Show resolved
Hide resolved
…lymorphic-method-type
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