Fix duplicate deleters in manage function - #1572
Conversation
iwami4438
commented
Jul 20, 2026
iwami4438
commented
- Filter out existing deleters with the same name before insertion.
- Added a comment suggesting exporting deleterVar in Info.hs later.
- ormolu fixed the style.
- Ref: Repl crashes on type of Map #1457
Filter out existing deleters with the same name before insertion. Added a comment suggesting exporting deleterVar in Info.hs later.
hellerve
left a comment
hellerve
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
I think this is an alright fix (I would remove the duplicated check here), but I think we could go for something more principled, and remove the root cause of the issue.
As far as I remember, the main issue was that the parser doesn’t assign identifiers (they are set in setNewIdentifiers in expansion). This is fine for all regular forms, but primitives never get there, so kind and type call qualification and annotation on uninitialized identifiers.
What I propose we do instead is to renumber before annotating in the two primitives that do this, mirroring what every other caller to annotate does. This is still brittle, because it also needs any future caller to do this, but I think it’s nicer than filtering it out later.
I’ll happily let others chime in, though.
| deleterV = \case | ||
| -- It's probably better to export deleterVar in Info.hs's. | ||
| ProperDeleter _ _ v -> v | ||
| RefDeleter v -> v | ||
| PrimDeleter v -> v | ||
| FakeDeleter v -> v |
There was a problem hiding this comment.
I’d agree to instead export deleterVar and use that.
This reverts commit b260159.
|
Thank you for the suggestion. I agree that addressing the root cause is better. I will update the implementation to renumber before annotating in the primitives as you proposed. |
- Renumber via `setNewIdentifiers` before `qualify`/`annotate` in `primitiveType` and `primitiveKind`. - Remove the now-unneeded duplicated deleter check in Memory.hs. - Export `setNewIdentifiers` from `Expand.hs`.
hellerve
left a comment
hellerve
left a comment
There was a problem hiding this comment.
Looks good to me now! If we want to go the extra mile, we can add some regression tests, but other than that, this is clean!
|
Okay, I'll add the test code. |
3 participants
