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

Changes to rec records #160

Merged
merged 3 commits into from
Nov 26, 2024
Merged

Conversation

Foxinio
Copy link
Collaborator

@Foxinio Foxinio commented Nov 24, 2024

Changes mentioned in #149, they should make it so that most of the time accessors to record fields marked data rec... aren't unnecessarily marked as impure.

  1. It's not gonna always work, by which i mean that sometimes they will be marked as impure even though they aren't, like in this code presented in original PR:
rec
  let foo (x : T) (n : Int) =
    if n == 0 then x
    else foo (x.x n) (n - 1)

  data T = { x : Int -> T }
end
  1. It's gonna become obsolete if we decide to make non termination more precise.

Changes mentioned in fram-lang#149, they should make it so that most of the time
accessors to record fields marked `data rec...` aren't unnecessarily
marked as impure.
Copy link
Member

@ppolesiuk ppolesiuk left a comment

Choose a reason for hiding this comment

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

I think, these changes can be seen as a quick fix, and in a long run we will change them to a more systematic, but more complex solution. Therefore, I would like to see a short comment describing what's going on.

src/DblParser/Desugar.ml Show resolved Hide resolved
@ppolesiuk
Copy link
Member

Do we want to have some tests?

@Foxinio
Copy link
Collaborator Author

Foxinio commented Nov 25, 2024

It won't hurt to have test for that

@ppolesiuk ppolesiuk merged commit 67e5f34 into fram-lang:master Nov 26, 2024
1 check passed
@Foxinio Foxinio deleted the 149-post-PR-changes branch November 28, 2024 09:52
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.

2 participants