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

fix(engine/importer): use correct field to import concrete implexprs #835

Closed
wants to merge 1 commit into from

Conversation

paulmure
Copy link
Contributor

@paulmure paulmure commented Aug 9, 2024

Close #834.

@W95Psp some of the existing insta tests are failing and I'm not knowledgeable enough about them to see if they need to be updated.

@W95Psp
Copy link
Collaborator

W95Psp commented Aug 13, 2024

Hi, thanks for your PR! Sorry for the delay, the week is quite busy... will try to look into those tests tomorrow!

Copy link
Collaborator

@W95Psp W95Psp left a comment

Choose a reason for hiding this comment

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

Sorry for the delay... LGTM.

@W95Psp W95Psp disabled auto-merge September 2, 2024 07:28
@W95Psp W95Psp self-requested a review September 2, 2024 07:28
@W95Psp
Copy link
Collaborator

W95Psp commented Sep 2, 2024

Ok, I'm in a rush and so I rushed the review as well... I wanted to merge too fast, and actually I think there is a problem with the PR. Let me comment on the issue.

@paulmure
Copy link
Contributor Author

paulmure commented Sep 2, 2024

@W95Psp
As discussed in the issue, I'm trying to import the trait goal alongside the information stored in ImplExprAtom.

However, this seems to break the uses of ppx_inline in many of the modules in lib/phases. I guess it's caused by incorrect inlining of the code in subtype.ml. And I'm not sure how to fix this.

Any suggestions would be greatly appreciated!

@paulmure
Copy link
Contributor Author

paulmure commented Sep 3, 2024

@W95Psp As discussed in the issue, I'm trying to import the trait goal alongside the information stored in ImplExprAtom.

However, this seems to break the uses of ppx_inline in many of the modules in lib/phases. I guess it's caused by incorrect inlining of the code in subtype.ml. And I'm not sure how to fix this.

Any suggestions would be greatly appreciated!

Forget what I said here, I think I just needed to run dune clean before building.

@paulmure
Copy link
Contributor Author

paulmure commented Sep 3, 2024

After some back and forth, I decided to only import the trait goal for the concrete case for now. This avoids storing redundant copies of this trait goal since we are not using the ImplExprAtom pattern as in the exporter.

I'm not sure if it's actually needed for the other cases. If such a need arises in the future, we can reconsider how to import this information again.

All tests pass now. The only change is that the traits test for f* added a line of implicit dependency.

@W95Psp
Copy link
Collaborator

W95Psp commented Oct 7, 2024

Hi @paulmure, I'm reviewing this PR this morning, and I think I did something similar recently... #827
Sorry for the duplicate work... (the last month has been pretty work intensive toward a verification project...)

Wdyt, shall we close this one or is it still needed?

@paulmure
Copy link
Contributor Author

That looks like the information we need. Thank you!

@paulmure paulmure closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Engine: Concrete ImplExprs are not imported correctly
2 participants