-
Notifications
You must be signed in to change notification settings - Fork 174
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
Address name shadowing #1032
Address name shadowing #1032
Conversation
where expandAllInternal xobj = | ||
do (newCtx, expansionResult) <- expand eval ctx xobj | ||
case expansionResult of | ||
Right expanded -> if expanded == xobj | ||
then pure (ctx, Right expanded) | ||
then pure (newCtx, Right expanded) |
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.
Another instance, which context should we use here?
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.
I think newCtx
is correct here
Generally, my initial idea was to return the old context if something goes wrong, so that no half-baked bindings etc find their way into there. But I’m not sure whether my approach was ever sound or solid, and I think time has washed away most of it anyway. It seems like either way hasn’t lead to problems, but I think generally being more conservative in what context to choose might be a good idea anyway. |
@hellerve Ah, that makes a lot of sense. (I think I had a similar approach in the early versions of the evaluator too, but forgot about it...) |
@jacereda Let's return the old context in places where there has been an error. Otherwise return latest. |
In any case, wouldn't it make sense to ensure a It could also make sense to, say, add some information about the error to the context (number of errors?). And if the code is refactored at some point to use an |
I'd love to move toward using the I've also considered just defining a monad implementation for |
@jacereda Yes, that sounds like a better solution. Should it be part of this PR, or another one? @scolsen Making a custom One thing that we should also plan for a little while thinking about these things is not stopping at the first error, but rather being able to continue complilation in some capacity. So maybe we should just skip ahead to whatever solution is best for doing that, while also cleaning up the code. That should definitely be its own PR though. |
@jacereda You had some comments on returning |
I still would need confirmation that #1032 (comment) is correct. Looking at |
Oh, I'll need to rebase because the PR I sent about duplicate members will probably break this. |
77acb31
to
ec90614
Compare
Rebased. |
@jacereda Alright, I think we're good now? |
I think so. |
🎉 🎉 🎉 |
There are some bits here I'm not sure about. I'll try to spot them tomorrow.l