Address name shadowing

[jacereda]

There are some bits here I'm not sure about. I'll try to spot them tomorrow.l

[jacereda]

Another instance, which context should we use here?

[hellerve]

I think newCtxis correct here

[hellerve]

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.

[eriksvedang]

@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...)

[eriksvedang]

@jacereda Let's return the old context in places where there has been an error. Otherwise return latest.

[jacereda]

In any case, wouldn't it make sense to ensure a Left comes with the original context instead?

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 Either monad, having Left carry the original context would make sense so you can just write foo >>= bar >>= baz.

[scolsen]

In any case, wouldn't it make sense to ensure a Left comes with the original context instead?

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 Either monad, having Left carry the original context would make sense so you can just write foo >>= bar >>= baz.

I'd love to move toward using the Either monad for this type of stuff and stick with the Maybe monad for things that can fail but shouldn't necessarily result in errors being reported (such as sequential lookup attempts).

I've also considered just defining a monad implementation for Context since in theory that seems to make a lot of sense (what is context, after all, other than some overarching context in which we are computing (evaluating in this case)?) and would likely simplify much of the context manipulations. But this would require a slight change to the type.

[eriksvedang]

In any case, wouldn't it make sense to ensure a Left comes with the original context instead?

@jacereda Yes, that sounds like a better solution. Should it be part of this PR, or another one?

@scolsen Making a custom Context mondad sounds great in the long run.

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.

[eriksvedang]

@jacereda You had some comments on returning ctx'' – those should be addressed before merge:ing, right?

[jacereda]

@jacereda You had some comments on returning ctx'' – those should be addressed before merge:ing, right?

I still would need confirmation that #1032 (comment) is correct. Looking at expand it seems it can return a mutated context, so I guess this is just a bug that should be fixed, but maybe you prefer to address that in another PR?

[jacereda]

Oh, I'll need to rebase because the PR I sent about duplicate members will probably break this.

[jacereda]

Rebased.

[eriksvedang]

@jacereda Alright, I think we're good now?

[jacereda]

I think so.

[eriksvedang]

🎉 🎉 🎉