Fix evaluation order for toplevel bindings in classes with local opens#13179
Fix evaluation order for toplevel bindings in classes with local opens#13179gasche merged 2 commits intoocaml:trunkfrom
Conversation
| | Tcl_open (_descr, cl) -> | ||
| build_object_init_0 | ||
| ~scopes cl_table params cl copy_env subst_env top ids | ||
| | _ -> |
There was a problem hiding this comment.
Having an exhaustive list of constructor instead of _ would probably prevent future instances of the bug
There was a problem hiding this comment.
I don't agree. If we followed the specification from the manual to the letter, then only Tcl_let should trigger the lifting, and this PR is actually introducing a bug. I felt that the spirit of the specification should allow Tcl_open too, so I opened this PR, but in my mind it's clear that those two cases are exceptions. If a constructor gets added, it should fall into the catch-all, unless we specifically want to extend the toplevel optimisation to it again (and I argue in #13178 that this optimisation is not even a good idea).
There was a problem hiding this comment.
If a constructor gets added, it should fall into the catch-all, unless we specifically want to extend the toplevel optimisation to it again
Having the compiler complain gives the opportunity to think about whether to extend the optimization or not. Having a diff here when adding a constructor will make this decision explicit to other reviewers as well.
There was a problem hiding this comment.
I agree with @hhugo that it would be better to list all other constructors explicitly here.
There was a problem hiding this comment.
I still think it's a bad idea. It makes it clear that we're not handling toplevel lets behind a constraint, or in a class application, in a consistent way.
If we have a fragile pattern-matching, this can be excused: it's just a hack, and it's actually documented as such in the manual. If we introduce exhaustive pattern-matching, it looks like a conscious decision to have an inconsistent compilation scheme, and I don't want to be involved in that.
If you want exhaustive pattern-matching, you're free to introduce it in a further commit.
There was a problem hiding this comment.
Keeping an implementation fragile and not-forward-safe is not an excuse for poor specification or behavior, the argument just feels very weird to me and I am confident that you are wrong. But it's your code, I won't force you, and I certainly believe that the proposed change is an improvement with respect to trunk. Let's go ahead and merge it.
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
This looks good, but I would much prefer if you got rid of the fragile wildcard pattern that helped introduce this bug in the first place.
| | Tcl_open (_descr, cl) -> | ||
| build_object_init_0 | ||
| ~scopes cl_table params cl copy_env subst_env top ids | ||
| | _ -> |
There was a problem hiding this comment.
I agree with @hhugo that it would be better to list all other constructors explicitly here.
4 participants
This PR fixes the issue with local opens in classes, as described in #13178.
I've added the examples from that issue in this PR, with their current semantics.