Skip to content

Proper attribute propagation in presence of objects#12344

Open
lthls wants to merge 3 commits intoocaml:trunkfrom
lthls:oo-wrap-attributes
Open

Proper attribute propagation in presence of objects#12344
lthls wants to merge 3 commits intoocaml:trunkfrom
lthls:oo-wrap-attributes

Conversation

@lthls
Copy link
Copy Markdown
Contributor

@lthls lthls commented Jun 30, 2023

Fixes #12232.

@lthls lthls force-pushed the oo-wrap-attributes branch from 1162e99 to 9008a00 Compare June 30, 2023 16:11
@lthls
Copy link
Copy Markdown
Contributor Author

lthls commented Jun 30, 2023

The test failures: mixin2.ml would be fixed by #12339, and the macOS testsuite fails with an additional error that doesn't look related.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 30, 2023

The "unrelated CI failure" is for test tests/weak-ephe-final/'finaliser_handover.ml' on a macos machine:

> Commandline: [...]_ocamltest/tests/weak-ephe-final/finaliser_handover/ocamlopt.byte/finaliser_handover.opt
> ### begin stdout ###
> Fatal error: exception File "finaliser_handover.ml", line 39, characters 2-8: Assertion failed
> ### end stdout ###

@lthls
Copy link
Copy Markdown
Contributor Author

lthls commented Jul 5, 2023

Some comments to help reviewers:

What I am trying to do in this PR is to ensure that the extra let bindings introduced by oo_wrap do not interfere with the attribute transfer mechanism.

My first attempt was to make sure that all problematic calls were already under another oo_wrap call (in which case it's more or less a no-op). The easy version would be to insert a call around the whole input, but that's of course not the best solution (at least the design of oo_wrap seems to imply that it's better not to use it when it's not needed). So in the current version of this PR, I've got an extra call to oo_wrap around all value bindings. I suspect that it's making most classes use the cache mechanism, including some that wouldn't have needed it, so maybe I'll need to rework that.

I've got a slightly better solution for modules, where I only insert calls to oo_wrap when I may need to move attributes (around calls to Translattribute.add_inline_attribute in practice), but it's still not completely equivalent to the code in trunk.

To sum up, with this PR I've taken the view that it's fine to make more classes use the cache mechanism than strictly needed. If that's fine, we could keep the current version, or push this even further and wrap the whole program (that would simplify the code a lot). If that's not fine, I'll need to know it so that I can work on a proper solution (which is not going to be pretty).

@garrigue I suspect that you're the only person which may still know (or care) about that. Do you have an opinion on this issue ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interference between the object system and inlining annotations

2 participants