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

Test and fix bad positions in conflict message #756

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

AltGr
Copy link
Contributor

@AltGr AltGr commented Dec 20, 2024

Related to #755, which this fixes for the lcalc interpreter ;
however, the real issue is the OCaml (and other) backends, where it's much harder.

Indeed, for those, the positions are passed as a flat list to the top-level default term, which has no information anymore about nested exceptions. The only reliable way I see to fix that is to switch evaluated default terms from 'a option to ('a * pos) option, and remove the the flat list of positions. It requires a change in the type of the handle_exceptions primitive, but it shouldn't be too difficult now that positions can be reified.

@AltGr
Copy link
Contributor Author

AltGr commented Dec 20, 2024

(note that, although the test triggers it with the merged same-output exceptions, the issue would exist with any nested exceptions)

Copy link
Contributor

@denismerigoux denismerigoux left a comment

Choose a reason for hiding this comment

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

I agree with this fix as well as the proposed generalized solution to change the accumulator type from 'a option to ('a, pos) option in the OCaml backend (and runtime.ml subsequently).

@denismerigoux
Copy link
Contributor

Can I merge this or do you want to put the refactoring of the runtime positions in default in this PR?

@AltGr
Copy link
Contributor Author

AltGr commented Jan 8, 2025

Hm, since ① this doesn't solve the issue where it is problematic (the backends) but only in the lcalc interpreter which the users shouldn't care about and ② the "real" fix should supersede this anyway, I don't think it's worth merging (although I'd like to keep it open for the testcase that is added)

@AltGr AltGr marked this pull request as draft January 8, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants