Some small patch-ups around matching on extensions#140
Conversation
antalsz
left a comment
There was a problem hiding this comment.
I have mixed feelings about this. On the one hand, I like it a lot -- saying "use get_desc instead of .foo_desc" is genuinely great. On the other hand, I worry about diffs with upstream. One careful design decision around of_ast was that we didn't need to edit the existing matches at all. We now have to change function preambles in terms of what they bring in to scope, edit what matches match on, and so forth. These may mostly be resolvable automatically, but I have some concerns about that.
|
You're right. I've mostly reverted. I didn't quite catch the main payload of the big comments as "this avoids merge conflicts". I've revised to make this clearer. Please re-review. Thanks! |
|
(After this is reviewed, I'll rebase the rest of the chain.) |
antalsz
left a comment
There was a problem hiding this comment.
Thanks, Richard, trying to make the comment highlight that more is a good plan. I think this is mostly good now, I just had a couple minor notes around comment wording. We should also rename this PR now that what it does is different.
I think you made the right call by reverting, but it's not obvious; there were wins in the get_desc design!
The goal here is to more structurally avoid the possibility of forgetting about extensions. This doesn't really actually get us there, though, because we can't change Parsetree to avoid using e.g. pexp_desc. And even if we could make that change, we might not want to, because it's just too convenient to just use pexp_desc. Until we have active patterns of some sort, we may be unable to improve upon the status quo. However, as I was working this, I identified a few spots where I think there are live bugs for lack of looking for extensions. And I think having a destructor, as this commit adds, is a positive little improvement.
f091984 to
2f9d1a3
Compare
EDIT: This was originally a much more ambitious patch, but has been scaled down because the original design would make upstreaming harder.
The goal here is to more structurally avoid the possibility of
forgetting about extensions. This doesn't really actually get us
there, though, because we can't change Parsetree to avoid using
e.g. pexp_desc. And even if we could make that change, we might
not want to, because it's just too convenient to just use pexp_desc.
Until we have active patterns of some sort, we may be unable to
improve upon the status quo.
However, as I was working this, I identified a few spots where I
think there are live bugs for lack of looking for extensions. And
I think having a destructor, as this commit adds, is a positive little
improvement.
This PR is against the
rae/no-raw-typebranch, because I'm stacking PRs.