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

Fix #304 Super inside closure #310

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Conversation

fdodino
Copy link
Contributor

@fdodino fdodino commented Nov 15, 2024

El closure se define como un Method, eso causa que cuando estás en el contexto de un super si buscás el primer método de los ancestors te traiga el mismo closure y quiera ejecutar <apply> dentro del objeto receptor. Como <apply> es un pseudo método interno, filtramos los métodos construidos de esta manera y super funciona de la manera esperada.

Tiene su correspondiente sanity test: uqbar-project/wollok-language#224

@fdodino fdodino requested review from PalumboN and ivojawer November 15, 2024 22:10
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.76%. Comparing base (baa848d) to head (1eefd59).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #310   +/-   ##
=======================================
  Coverage   89.75%   89.76%           
=======================================
  Files          28       28           
  Lines        3172     3175    +3     
  Branches      574      576    +2     
=======================================
+ Hits         2847     2850    +3     
  Misses        169      169           
  Partials      156      156           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@ivojawer ivojawer left a comment

Choose a reason for hiding this comment

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

Uhh que bien este fix 💕 💕

src/helpers.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@PalumboN PalumboN left a comment

Choose a reason for hiding this comment

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

Vamoooo Dodaiinnnnn!! 🚀 🥇

src/helpers.ts Outdated
@@ -442,7 +442,10 @@ export const getNodeDefinition = (environment: Environment) => (node: Node): Nod
}

export const superMethodDefinition = (superNode: Super, methodModule: Module): Method | undefined => {
const currentMethod = superNode.ancestors.find(is(Method))!
function isValidMethod(node: Node): node is Method {
return node.is(Method) && node.name !== CLOSURE_EVALUATE_METHOD && node.parent.fullyQualifiedName !== CLOSURE_MODULE
Copy link
Contributor

Choose a reason for hiding this comment

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

Acá por qué node.parent.fullyQualifiedName !== CLOSURE_MODULE? Esto evita que se pueda usar super() dentro de la clase Closure (que por ahora no pasa).

Copy link
Contributor

@PalumboN PalumboN Nov 19, 2024

Choose a reason for hiding this comment

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

Ahí leí el comentario de @ivojawer (que se había solucionado) y estoy de acuerdo! Pero la condición había quedado mal escrita (maldito De Morgan).

Ahí delegué en una función isApplyMethodForClosures para que no haya confusión (de hecho, ya la reutilicé en otro lado).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Groso @PalumboN !! Tire magia!!!

@PalumboN PalumboN merged commit 25de9db into master Nov 19, 2024
4 checks passed
@PalumboN PalumboN deleted the fix-#304-super-inside-closure branch November 19, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants