Skip to content

Some small patch-ups around matching on extensions #140

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

Merged
merged 8 commits into from
Mar 10, 2023

Conversation

goldfirere
Copy link
Contributor

@goldfirere goldfirere commented Mar 8, 2023

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-type branch, because I'm stacking PRs.

@goldfirere goldfirere requested a review from antalsz March 8, 2023 20:18
@goldfirere goldfirere mentioned this pull request Mar 9, 2023
Copy link
Contributor

@antalsz antalsz left a comment

Choose a reason for hiding this comment

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

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.

@goldfirere
Copy link
Contributor Author

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!

@goldfirere
Copy link
Contributor Author

(After this is reviewed, I'll rebase the rest of the chain.)

Copy link
Contributor

@antalsz antalsz left a comment

Choose a reason for hiding this comment

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

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!

@goldfirere goldfirere changed the title Use proper destructors for extension-enabled ASTs Some small patch-ups around matching on extensions Mar 10, 2023
@goldfirere goldfirere changed the base branch from rae/no-raw-type to main March 10, 2023 13:44
goldfirere and others added 8 commits March 10, 2023 09:09
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.
@goldfirere goldfirere force-pushed the rae/extension-destructors branch from f091984 to 2f9d1a3 Compare March 10, 2023 14:09
@goldfirere goldfirere merged commit 74aa974 into main Mar 10, 2023
@goldfirere goldfirere deleted the rae/extension-destructors branch March 21, 2023 21:04
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