Skip to content

pattern matching refactoring: explicit types for record-expanded pattern heads#10089

Open
Octachron wants to merge 1 commit intoocaml:trunkfrom
Octachron:pattern_matching_expanded_records
Open

pattern matching refactoring: explicit types for record-expanded pattern heads#10089
Octachron wants to merge 1 commit intoocaml:trunkfrom
Octachron:pattern_matching_expanded_records

Conversation

@Octachron
Copy link
Copy Markdown
Member

During the review process of #9520, it appeared that we were repeatedly expanding record patterns of the form
{ x = _; _ } into { x = _; y = _ } because it is not-so-straightforward to track which patterns had already been expanded in such way.

This PR proposes to lift the distinction between record-expanded heads and non-expanded heads into the type system, and then fearlessly remove the redundant calls to expand_record_head in lambda/matching.ml.

@gasche
Copy link
Copy Markdown
Member

gasche commented Dec 22, 2020

I must say that I am a bit disheartened by the complexity of the result. To handle things fully statically, we have to move from a one-dimensional axis (half-simple -> simple) to two dimensions (simple or half-simple, expanded or not). The type structure is harder to read, and explicit coercions pop up in many new places.

What would other compromises look like? A first idea would be to distinguish two constructors, Record and Expanded_record, so that we dynamically track the fact that a record has been expanded, and never expand twice. Then we could try tracking expansions statically only in Simple and not in Half_simple for example, and see if that brings us more benefits than just dynamic tracking, without the complexity of your proposal. Does this make sense?

@Octachron
Copy link
Copy Markdown
Member Author

A possible simplification axis would be to inline the expanded head type into a simplified head in Matching in order to manifest the invariant that we don't need to expand heads that come from simplified patterns. This would remove the expanded/nonexpanded axis from the pattern views.
Or we could simply remove expand_record_head.

val to_omega_pattern : t -> pattern

val omega : t
val omega : expanded
Copy link
Copy Markdown
Member

@gasche gasche Jan 15, 2021

Choose a reason for hiding this comment

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

Could we use [> `Any ] here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, we cannot do this because pattern heads are not polymorphic variants, yet.

type nonexpanded_view = [
| Simple.nonexpanded_view
| `Or of pattern * pattern * row_desc option
]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could the `Or constructor have its own type declaration to not repeat it twice in view and expanded_view?

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