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

Remove global state for typechecking patterns #12229

Merged
merged 11 commits into from
Jul 5, 2023

Conversation

ncik-roberts
Copy link
Contributor

@ncik-roberts ncik-roberts commented May 8, 2023

This is a follow-on cleanup from #12117, inspired by @garrigue's comment about global state. @lpw25 and @goldfirere reviewed a version of this patch that was merged to Jane Street internally.

This patch replaces global variables managed by type_pat with a (mutable) state value that's explicitly passed around. There are a few benefits apart from the obvious "global mutable state is hard to reason about":

  • the checking of or-patterns is a little more straightforward
  • this makes it more obviously correct to remove the unnecessary module_patterns_restriction argument from partial_pred, exposed in the mli. (And, I do remove it in this PR.) These two things don't interact in an interesting way (module patterns, exhaustivity checking), and the explicit scoping of the type_pat_state value makes it obvious that we can just ignore module patterns here.

Implementation

The previous discipline was something like:

  1. Call reset_pattern to reset the global state
  2. Call type_pat (which writes into the global state)
  3. Read out the results from the pieces of global state (the global variables pattern_variables, module_variables, and pattern_force)

The discipline is now:

  1. Call create_type_pat_state to create tps, a fresh copy of pattern-checking state
  2. Call type_pat on tps (which writes into its fields)
  3. Read out the results from the fields of tps

I add a test to ensure that pattern_forces aren't dropped when checking or-patterns, as a draft of this PR made this mistake.

@gasche
Copy link
Member

gasche commented May 8, 2023

@t6s would you maybe be able to review this?

@goldfirere
Copy link
Contributor

The Jane Street patch is at ocaml-flambda/flambda-backend#1281 if you want to see the conversations that happened during initial review.

Copy link
Contributor

@t6s t6s left a comment

Choose a reason for hiding this comment

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

The changes make the code simpler and better documented. Looks good to me.

As a side-product of my reviewing, I have done rebasing the branch to the current trunk,
with some comments added:
ncik-roberts#1

@t6s
Copy link
Contributor

t6s commented Jul 5, 2023

And, sorry for delaying my review so much.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Approving on behalf of @t6s. This is good to merge once rebased.

@ncik-roberts
Copy link
Contributor Author

Thanks @t6s for rebasing. I've updated this PR with the result of that.

@gasche gasche merged commit f9319a3 into ocaml:trunk Jul 5, 2023
9 checks passed
@gasche
Copy link
Member

gasche commented Jul 5, 2023

Merged, thanks @ncik-roberts for the nice patch and @t6s for the review.

Note: in the future (I'm noticing this as I just merged and I don't think it is important enough for this particular PR to request a change after the fact), I think it would be perfectly legitimate to credit people who reviewed on the internal flambda-backend repository (here Antal and Leo) as long as you consider their review was a substantial contribution (as far as reviews are concerned). (This is common practice in the Linux community where commit message tags are used to track review or testing work over the complex lifetime of each patchset.)

@ncik-roberts
Copy link
Contributor Author

Ah, yes, that was a mistake—I thought to myself when I first opened this PR that I wanted to credit them as reviewers. Anyway, I agree it doesn't seem worth it to fix this omission in this case, but thanks for mentioning this.

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.

None yet

4 participants