Skip to content

[matching.ml cleanup] abstract default environment#8850

Merged
trefis merged 4 commits intoocaml:trunkfrom
trefis:matching-default-env
Aug 5, 2019
Merged

[matching.ml cleanup] abstract default environment#8850
trefis merged 4 commits intoocaml:trunkfrom
trefis:matching-default-env

Conversation

@trefis
Copy link
Copy Markdown
Contributor

@trefis trefis commented Aug 1, 2019

This PR:

  • regroups all the functions manipulating default environments into a module (this was a weird omission from the previous PR); this created more code move than I expected.
  • adds a type for default environments and make it abstract
  • integrates parts of Gabriel's comment.

Note: I believe the code change in the 1st commit to be correct, but the comment added in that commit should be double checked.

@trefis trefis requested review from Octachron and gasche August 1, 2019 15:17

val pop_column : t -> t

val pop_compat : pattern -> t -> t
Copy link
Copy Markdown
Member

@Octachron Octachron Aug 2, 2019

Choose a reason for hiding this comment

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

Isn't there a contradiction between the pop_column function, which remove one column from every matrix, and pop_compat, which remove one column from compatible rows but also remove incompatible rows? Maybe keeping specialize_compat as a name would be clearer here.
Similarly, pop and pop_column have a slightly different meaning for their common pop root since only pop returns the dropped leftmost element(s), but this sounds like a minor discrepancy at most.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I kept the names that were already there, but I guess just specialize would be a decent name.

There is also val pop : ... which pops the first matrix from the list... Perhaps pop_column should become pop_first_column_of_matrices?

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.

Another possibility might be specialize_for_var to describe the use case rather than the implementation?

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.

pop_compat is the composition of pop_column and filter_compat (or guard_compat). I don't think that specialize is more informative as a name, and I'm not fond of the longer names proposed (pop_first_column_of_matrices should be map_rows pop); maybe just implement filter_compat and write the composition by hand? (It's one more traversal of the whatever, but it doesn't actually matter much.)

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.

@trefis and myself have discussed better interfaces for matchers, in particular pattern -> pattern list (where NoMatch is the empty list and OrPat returns a list of explosions) and a version where OrPat is removed (replaced by generic handling of or-patterns in the specialization code), allowing pattern -> pattern option. I think that at this point it may be possible to keep only specialize in the module, and write the matchers nicely in the client code, obviating the need for those API naming discussions.

In the meantime, my inclination would be to keep the names as they currently are to keep the refactoring train going.

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.

It is most probably a good idea to let the refactoring train go on. And, maybe, or not, I will have a look at the end of the refactorization tunnel to see if those names are still here.

Copy link
Copy Markdown
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

The new comments are really nice. The new module clarifies the code structure, but the names are maybe a bit too implementation-specific.

For instance Default_env.pop_column sounds more like a description of the function's action on the underlying implementation that a meaningful action on a self-contained default environment concept. But this not may be the right PR to discuss that kind of issues.

@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 2, 2019

I don't have a definitive mind made on the question, but I think that it's good to keep "operational" names sometimes if they coincide with how the people writing the code think about the implementation. Luc really pops columns, and it's also the way most matrix-decomposition algorithms (for both compilation and analysis) are expressed and thought about.

@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 3, 2019

A default environment def is passed as input to split_or, and another is returned as an output (in top_default) along with a list of nexts (pairs of exit numbers and precompiled matrices). An important thing that I needed to re-understand to read these comments is the following: the output environment has first one entry for each exit number in nexts, and then the input default environment as a tail/suffix.

Two other ways to say similar things:

  • the nexts are purely local outputs of the compilation process (compiling a list of clauses gives a linked list of precompiled matrices), whereas def prepends local information (output of the current compilation goal) onto global information (information from the outer context of the compilation).

  • nexts is a Writer monad, def is a State monad, but in fact it is a log state; each write to nexts comes paired with a write to def.

@trefis trefis force-pushed the matching-default-env branch from f4666a5 to 01099da Compare August 5, 2019 09:46
@trefis
Copy link
Copy Markdown
Contributor Author

trefis commented Aug 5, 2019

I have updated the PR following comments (I ended up not renaming pop_compat or changing it).

I will merge once the CI is green.

@trefis trefis merged commit 301e1e6 into ocaml:trunk Aug 5, 2019
@trefis trefis deleted the matching-default-env branch August 5, 2019 11:04
trefis added a commit to trefis/ocaml that referenced this pull request Aug 12, 2019
gasche added a commit to gasche/ocaml that referenced this pull request Apr 18, 2020
The bug comes from <ocaml#8850> that we
wrote and got merged last August. It breaks specialization of default
matrices, so in theory it should not lead to wrong-code production,
but because it can create default matrices of distinct arity in the
same environment it should be possible to observe assertion failures
in code hitting the bug.

The bug was made possible and in fact easy (and also uncatchable by
human eyes, including the almost-super-human-reviewer of ocaml#8850) by
a dubious stylistic choice made in
842eb76 in 2002, where `rem`
sometimes means the not-yet-filtered remainder and, only in one
non-idiomatic case, is the filtered remainder.

I found the bug while upgrading the code to use better types. The
input of specialize_matrix has non_empty rows, but the output may be
empty -- and this code becomes ill-typed when input and output have
distinct types. Yay typing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants