Labeled tuples

[ccasin]

This adds a new language feature: Labeled tuples, making tuple fields optionally labeled. It is joint work with @rtjoa.

Motivating example

The labeled tuples extension allows the programmer to label tuple elements. It is conceptually dual to labeled function arguments, allowing programmers to give helpful names to constructed values where labeled function arguments permit giving a helpful name to parameters.

For example, suppose we want to compute two values from a list and be careful not to mix them up:

let sum_and_product ints =
  let init = ~sum:0, ~product:1 in
  List.fold_left (fun (~sum, ~product) elem ->
    let sum = elem + sum in
    let product = elem * product in
    ~sum, ~product
  ) init ints

Because sum:int * product:int is a different type from product:int * sum:int, the use of a labeled tuple in this example prevents us from accidentally returning the pair in the wrong order, or mixing up the order of the initial values.

Like records, labeled tuples patterns may be reordered or partial. This only works when the compiler knows the type of the pattern from the context, which simplifies the implementation considerably and results in a feature that is similar in many ways to SML's flexible records (see abstract linked below for a more thorough comparison). This PR includes many tests demonstrating the reordering and partial patterns.

Background

This feature has been in Jane Street's branch of the OCaml compiler for ~8 months, and has proven useful and popular internally. I gave a talk about it at this year's ML workshop, and there has been some interest, so I am posting this PR to continue the conversation. The video of that talk is available, as is the abstract we submitted describing the idea.

The version implemented here differs in one small way from the version described in the abstract: repeated labels are not allowed. This change has been made based on feedback from the talk.

Implementation details

The implementation is fairly straightforward. The parsetree and typedtree are updated so that tuple types, expressions and patterns now have an optional label on each field. Reordered and partial patterns are resolved during typechecking and the labels are erased during translation to lambda.

The only tricky part is parsing, and I have left detailed comments in the parser explaining the complexities and my approach to resolving them. It may be that this could be simplified - I would be happy for feedback.

I have not yet updated the manual. It wasn't obvious to me whether the bits of Chapter 11 ("The OCaml language") that mention tuples should be updated to acknowledge the possibility of labels, or if they should be left alone and a new section should be added to Chapter 12 ("Language extensions"). If someone advises which to do, I will write some text.

[Octachron]

The current process for the manual is that new features are added to the language extensions chapter, with a forward reference in the "language" chapter. Then some years later when I start to worry once again about the length of the extensions chapter I try to move the new chapter outside of the "extensions" chapter.

[garrigue]

It looks like my mail did not go to the caml-devel discussion, so I write it here.

I’m sorry to be very slow to react.
While I am fine with the feature, I have some late misgivings with the syntax.
Wouldn’t it be more natural (and future proof) to use a record-like syntax, rather than just
mixing labels with tuples.
I’m thinking of something like

 let r = `{x = 1; y = 3}
 type t = `{x : int; y : int}

Combined with punning, it would not be more verbose than the current proposal.

I have no problems with having limitations at this point, but I’m quite confident
that in the future people will want these to behave more and more like real records.

This also relates to Xavier’s point that in many cases you may want to switch to
real records at some point, so that having a closer syntax would help.

[gasche]

I started looking at the code (I stopped when reaching typecore for now). Overall this looks fine, a lot of small amounts of list-shuffling code to preserve the semantics with the change of representation, but I haven't gotten to the juicy bits yet.

Overall approach

There are two ways to implement this, one (A) is to generalize the current type of tuples (Ttuple) to have optional labels on each elements, and the other (B) is to introduce a new type of labeled tuples (Tlabtuple) that has (optional) labels on each elements -- with the understanding that one is not supposed to use Tlabtuple when no labels are present, as it gives a type incompatible with the corresponding Ttuple.

The benefit of approach (A) is that it is less code, just one uniform version for both kinds of tuples, instead of duplicating logic for labeled and non-labeled tuples. The downsides of (A) is that the new/modified code will run all the time on existing tuples (so it could impact the type-checker performance or correctness, even for codebases where labeled tuples are not used), and it changes the representation of existing tuple types in .cmi files so a bootstrap is needed.

The current PR chooses (A). I think that this is reasonable -- both choices would probably have been reasonable. I'm not worried about performance impact (the manipulations that are done are not particularly expensive, just a few more allocations in an non-performance-critical codebase), and checking correctness is what we will do -- plus most mistakes would break the testsuite anyway.

Parsing code

I agree that the parsing code is tricky, and I think it would be nice if @fpottier was available to review it. I wondered if it would make things simpler to mandate parentheses when labels are present (in particular, it removes a potential conflict between labeled tuples and just expressions)?

Nitpick: helper functions

Many places in the code have a helper function that deals with lists of types (foo_list), and now there is a second helper function that deals with labelled lists of type (foo_labeled_list), which is a copy-paste of foo_list with a bit more extra code. At first I thought that it was fine, and it is mostly okay, but in fact I find it a bit unpleasant, because it means that if we want to change something in this logic, we have to make the change twice. For example, maybe we want to change the pretty-printing of lists, and now we have to change formatting boxes in two different places.

When possible, I would prefer if we could make the list version higher-order, to be parametrized over the element, in a way that also works for labeled lists (the element is a (label, type) pair). Then the labeled-list version can easily be derived as an auxiliary function from the parametrized version, and the list-traversal logic is shared.

(If there are cases where this approach would make the code harder to write or substantially more complex, then we can keep the duplicate version.)

[gasche]

Previously Ttuple was used as a hack to run deep_occur on a list of types. This should be equivalent to List.exists (deep_occur ty) l, but it will be faster if some of the elements of the list are shared or share subtypes -- they benefit from a single visited-map and will not be traversed several times.

This was inelegant before and I find it slightly worse now. I would prefer if we had a deep_occur_list version that checks for a list of types. This could possibly be implemented with something like:

type 'a inputs = One of 'a | Several of 'a list
val deep_occur_gen : type -> type inputs -> bool

(* easy wrappers over deep_occur_gen *)
val deep_occur
val deep_occur_list

[ccasin]

Yes, this is true. @rtjoa actually pointed out the same thing when he was working on this in our branch, and we merged a fix for deep_occur (oxcaml/oxcaml#1503) and he also wrote one for free_vars (oxcaml/oxcaml#1519) but we didn't merge it because it costs us something to have diffs with the main OCaml compiler.

I deleted his diff for deep_occur when backporting this just because deep_occur had changed a bit upstream and I wanted to focus on the core functionality rather than generalizing the new type marking mechanism. But I agree that what you and Ryan have suggested is cleaner and I will make an additional commit here with those changes in the next day or two.

[ccasin]

I've now pushed a commit that creates list versions of deep_occur and free_vars. I agree this is a nice improvement. I didn't find it necessary to add a type like inputs - let me know what you think.

[gasche]

Same comment here as with deep_occur above: creating a new tuple type is meh.

[t6s]

There have been ghost expressions / patterns in parser.mly that use (abuse?) Pexp_tuple / Ppat_tuple and they now need to be coupled with extra None, which looks a bit ugly. Could we perhaps add constructors for such ghost tuples?

[alainfrisch]

Wouldn’t it be more natural (and future proof) to use a record-like syntax

I tend to agree that record-like syntax might be more natural for the new feature (even if this excludes the case of mixed labeled/non-labeled fields). If one wants to keep the new notion close to tuples, implementation-wise, one could consider existing (non-labeled) tuples as syntactic sugar for `{ _1 = ...; _2 = ...; ... }.

[ccasin]

I’m sorry to be very slow to react. While I am fine with the feature, I have some late misgivings with the syntax. Wouldn’t it be more natural (and future proof) to use a record-like syntax, rather than just mixing labels with tuples.
...
I have no problems with having limitations at this point, but I’m quite confident that in the future people will want these to behave more and more like real records.

Thanks for the feedback, @garrigue and @alainfrisch! Jacques, I'm curious to know more about what you mean by "in the future people will want these to behave more and more like real records".

To provide a little background for why we went this way initially: Our design goal internally with this feature was not really to build a replacement for records. Generally I'd prefer people continue to use records in the situations where they do today. Labeled tuples as proposed here are a lightweight mechanism to give a cheap name to something that is used locally. But where many functions agree on a type, particularly if it appears in an interface, I'd prefer to encourage people to continue using a record. Record declarations provide a good place to write comments and to apply ppxes (particularly "deriving"s).

Further, because these things are structurally typed, not nominally typed, associating them with tuples rather than records feels natural to me. I think it would be a little duplicative to have yet another structurally typed product in the language (we have at least two - tuples and objects).

I agree this results in more work if you want to transition from a tuple to a record (though still less work than if the tuple didn't have names!). On the other hand, it results in less work if you want to give names to an existing tuple. And I think adding labels to tuples will be a more common mode of use than moving back and forth between records, for the reasons in the first paragraph. (And based on some experience here at Jane Street.)

I think your comment about "real records" is saying we'll eventually wish we'd done something more like a record version of polymorphic variants, with the corresponding row typing, rather than something like labeled parameters but for products. I think that's a reasonable feature to consider having, but a rather different one, and it partially already exists in the form of the object system. Personally, I prefer the version of labeled tuples we've proposed for the reasons above, and because they can be more performant than something whose type has a row variable - they are really compiled the same as regular tuples.

Anyway, that's the context for why we've proposed what we have. All that said, I really am grateful for the feedback, and of course we can change course here if there is consensus that the above reasoning is wrong.

[ccasin]

The current process for the manual is that new features are added to the language extensions chapter, with a forward reference in the "language" chapter.

Thanks @Octachron! I have added a manual chapter.

[rtjoa]

Thanks all for the feedback!

A quick note: record-like syntax makes it hard to support unlabeled components, as the most straightforward syntax (omitting lbl:) would be ambiguous with punning and partial patterns. (Does the pattern `{ x; _ } pun x or bind x to an unlabeled component? Does it ignore one unlabeled component or multiple possibly-labeled components?)

I think unlabeled components are definitely nice to have for the reasons that @ccasin outlined above on ease of transitioning from normal tuples.

[garrigue]

Further, because these things are structurally typed, not nominally typed, associating them with tuples rather than records feels natural to me. I think it would be a little duplicative to have yet another structurally typed product in the language (we have at least two - tuples and objects).

Dear @ccasin , thank you for giving more context, in particular the importance of unlabeled fields.
My impression is that, as soon as you allow fields to commute in pattern-matching, we are talking about records, not tuples (notwithstanding the syntax). That is the reason I have misgivings with this syntax.
Also, the analogy with labelled arguments is weaker than I thought first, as the separator is the comma.

For the future, it is clear to me that people will eventually want to be able to use projections in place of pattern-matching, at least. I'm less sure about row polymorphism, as the implementation cost is much higher, but there is some demand for it. Those are clearly not orthogonal to the role of labeled tuples.

So my impression is that, since the power of this feature is exactly that of SML, using the time-proved syntax of SML would be more natural and future-proof.

[goldfirere]

Regarding syntax: I think the current design is forward-compatible with a projection x.label, as long as the type of x is principally known. (We could also consider a different projection operator such as x.~label or something.)

The real syntax question to me is: do we want a feature that's like records but without requiring a type declaration? or a feature that's like tuples but allowing the possibility of labels? I think both are reasonable, but I think the latter would get more uptake in practice. (I don't have evidence of this.) The beauty of tuples today is that they are lightweight: no type declaration, minimal syntax. If you're willing to carry more weight, records are a great feature. To me, if this new feature is going to get uptake (and it certainly has within Jane Street!), it needs to have a very lightweight syntax. I think leaning on record syntax here will materially discourage users from reaching for the feature.

It's not clear to me how important reordering in patterns is. Perhaps we could drop that aspect of the feature if that is a sticking point, though it seems a little anti-user to do so when supporting it is straightforward.

[gasche]

I agree that reordering is not so important in full tuples and full record expressions, but we do probably want to keep partial patterns which are a form of reordering as well (they allow to "skip" certain fields).

[ccasin]

When possible, I would prefer if we could make the list version higher-order, to be parametrized over the element, in a way that also works for labeled lists (the element is a (label, type) pair). Then the labeled-list version can easily be derived as an auxiliary function from the parametrized version, and the list-traversal logic is shared.

@gasche Thanks for this suggestion. I've made one such change in oprint, agree it is an improvement, and have pushed it to this PR.

But the biggest offender on this front is ctype. I tried coming up with a nice higher order version of the relevant functions there, but found the result unsatisfying. I've pushed a commit here that shows the attempt for mcomp_labeled_list. I didn't find a way to include the necessary check on the labels in the same call to List.iter2 as the call to mcomp, resulting in an extra list traversal. And the abstracted version now needs an ugly type annotation to resolve the polymorphic recursion.

So, I agree this is sad, but don't quickly see a way to do better. Though, it's possible you'll find the version in that commit preferrable or see a more clever way to abstract here - let me know.

[ccasin]

Dear @ccasin , thank you for giving more context, in particular the importance of unlabeled fields.
My impression is that, as soon as you allow fields to commute in pattern-matching, we are talking about records, not tuples (notwithstanding the syntax). That is the reason I have misgivings with this syntax.
Also, the analogy with labelled arguments is weaker than I thought first, as the separator is the comma.

For the future, it is clear to me that people will eventually want to be able to use projections in place of pattern-matching, at least. I'm less sure about row polymorphism, as the implementation cost is much higher, but there is some demand for it. Those are clearly not orthogonal to the role of labeled tuples.

Thanks again for the feedback, @garrigue. I see your point that commuting in patterns feels like records, and it's a reasonable one. But I find myself still liking these as tuples:

I think, on reflection, that I don't agree that people will want these to behave "more and more like real records in the future". I certainly agree we may want projection, but as @goldfirere has pointed out we can equally well support that for tuples. And other moves in this direction, namely supporting more reordering in more places, I think will require row polymorphism. But it would not make sense to add that to labeled tuples/anonymous records because then we would just have built a feature that is exactly like one the language already has (objects), and labeled tuples/anonymous records offer some advantages over that feature.

I take your point that the record syntax works well in SML. It does! But I think it fits in less well with OCaml. We already have a notion of records, and I think adding a new structurally typed one that is separate from tuples would leave tuples themselves in a weird place. That is, it would often be a mistake for a programmer to choose a tuple rather than one of these anonymous records, because they are simply creating work for themselves if they find they want to add a label later.

So, if reordering fields of tuples is deemed too weird, I think I would argue for removing the reordering rather than changing these to be records.

[alainfrisch]

"more and more like real records in the future"

One such aspect that people might request would be the field override { r with x = ...; y = ... }, keeping the current ordering of existing fields (like records -- but with no constraint on the type of fields), and possibly extending at the end if adding more fields (not really like records). This shouldn't be more difficult to support than field projection, but I don't see a nice syntax with labeled tuples.

The beauty of tuples today is that they are lightweight: no type declaration, minimal syntax. If you're willing to carry more weight, records are a great feature.

Is the record syntax really heavier?

In terms of character count, compared to records, labels add one character per field (~), and avoid two curly braces and one disambiguation character (because one'd need one to distinguish from normal records) -- although using parentheses with tuples might be a good practice anyway so one might not even want to count these two curly braces. Anyway, even if we count two curly braces, with 3 fields or more, the record syntax is not any longer. (I agree than being "heavy" is a different, less well-defined notion.)

~foo,~bar,~baz
`{foo;bar;baz}

(~foo, ~bar, ~baz, ~qux)
`{foo; bar; baz; qux}

My intuition is that the record syntax would "work better" (in terms of readability) with long multi-line expressions. They might tend to create more indentation with usual indentation rules compared to label tuples (if people don't use parentheses), though:

let f =
  `{
    foo=(...);
    bar=(...);
    baz=(...);
  }

let f =
  ~foo:(...),
  ~bar:(...),
  ~baz:(...)

One other small syntactic advantages of the record syntax: allowing the final semi-colon, which make it look more "regular", and avoids a spurious diff line if one extra field is added.

[goldfirere]

I agree that it's hard to quantify "heavy" or make crisp arguments about it. So let me take a different tack: the goal of labeled tuples is to enhance the power of tuples, not to enhance the power of records. That is, we imagine (and have seen in practice) that programmers take existing tuples in their code, and add some labels. There are two separate interesting aspects there: 1) that folks are migrating from tuples, and 2) that only some components are labeled. (In general, it's much more important to apply labels when there are multiple values of the same type than places where the type is salient.)

Perhaps if we were starting from scratch, we would not have tuples at all and have both predefined records (as now) and not-predefined records (like the counter-proposal in this thread). But we are not starting from scratch: we want to support easy migration (for those that want to) from tuples. To me, this steers us to the tuple-based syntax.

[gasche]

We discussed this syntax at the maintainer meeting. The general consensus was to stick with a tuple-like syntax, in particular because the intended use-case is to make it easy to migrate tuple-using code to this feature, by adding more robustness with a very lightweight change. @garrigue asked that we clarify that this feature strongly resembles SML records, which it indeeds does -- despite the different surface syntax and the small difference in operations offered (no projections, reordering only in patterns and not in introduction forms).

[ccasin]

We discussed this syntax at the maintainer meeting. The general consensus was to stick with a tuple-like syntax, in particular because the intended use-case is to make it easy to migrate tuple-using code to this feature, by adding more robustness with a very lightweight change. @garrigue asked that we clarify that this feature strongly resembles SML records, which it indeeds does -- despite the different surface syntax and the small difference in operations offered (no projections, reordering only in patterns and not in introduction forms).

@gasche thanks for this update, and sorry for my slow response here. I will add a note to the manual entry mentoning the inspiration from SML flex records.

This PR is now in need of rebasing, which I am happy to do. But I think you had done half a review and I don't want to mess up your review state, so I will hold off for now in case you plan on continuing that. Let me know if you'd prefer I eagerly rebase, or if there's any way I can help move this forward.

[lpw25]

My understanding of the state of this PR is that:

• The design aspects have all been approved for merging
• It is essentially identical to the one on the Jane Street branch which has been fully reviewed for correctness and used in production for over a year.
• It needs a rebase.

Is that right @ccasin? If so then I'm happy to approve it on the basis of the design approval of the dev meeting and the previous correctness review. Then it just needs a rebase and we're good to go.

[ccasin]

My understanding of the state of this PR is that:

• The design aspects have all been approved for merging
• It is essentially identical to the one on the Jane Street branch which has been fully reviewed for correctness and used in production for over a year.
• It needs a rebase.

Is that right @ccasin? If so then I'm happy to approve it on the basis of the design approval of the dev meeting and the previous correctness review. Then it just needs a rebase and we're good to go.

That is my understanding of the state of affairs (based on @gasche's previous comments). I can rebase this next week.

[lpw25]

Approving on the basis that the design was approved at a dev meeting and the code was already reviewed for correctness at Jane Street.

[ccasin]

I have added a bit to the manual chapter about the relationship with SML's flexible records, as requested, then squashed this PR and rebased it.

The "Parsetree Updated / comment-and-label" github action is failing - any advice on how to satisfy it would be welcome. The error is "Resource not accessible by integration", and I'm not sure what it's trying to access or why it is inaccessible.

[gasche]

I'm not sure either. (Maybe @panglesd has an idea, as the author of this CI check?)
I added the parsetree-change label, as the parsetree indeed changes with this PR.

[panglesd]

I think the parsetree-change CI has been rewritten by @NathanReb. It requires to add the label (which is now done!), which triggers the addition of a comment CCing @ocaml/ppxlib-dev, so that ppxlib maintainers are aware of a potential parsetree change and can participate in the discussion.

I believe something has gone wrong with posting the comment through the API. But the CC has been done anyway now!

[NathanReb]

This looks like the action does not have write access, that might be a repository configuration issue but I'll need the help of someone with write access to the repo to debug it!

[ccasin]

@gasche @Octachron - would one of you have time to work with @NathanReb on debugging the CI check? I don't have write access, so I can not help him.

If not, perhaps we should disable it so CI can run on PRs with parsetree changes?

[Octachron]

I am not sure I follow, the CI is running just fine on this PR, isn't it?
I will try to find the time to debug what is happening with the parsetree changes check, but this should not affect this PR (nor other PRs modifying the parsetree).

[ccasin]

I am not sure I follow, the CI is running just fine on this PR, isn't it? I will try to find the time to debug what is happening with the parsetree changes check, but this should not affect this PR (nor other PRs modifying the parsetree).

Perhaps I misunderstood what has happened here. My impression from @panglesd and @NathanReb's last comments is that the parsetree changes check is broken and needs an admin to debug. If that's wrong, advice on how I can fix the current failure myself would be much appreciated!

[NathanReb]

The parsetree changes check's role is simply to label PRs that modify the parsetree and to ping ppxlib maintainers so they are aware of the change. This is a way to stay in touch with trunk's modification to the parsetree which will have to be mirrored in ppxlib.

You don't have to do anything on this PR and until it is fixed on our end, I think you can simply ignore this particular check's failure, especially since we labeled the PR and mentioned ppxlib's dev team already!

[gasche]

I started looking at the code again. Most of it is fine, but the complexity of the parser changes is giving me pause. There is a lot of very clever but fairly verbose changes (that are clearly documented, thanks) to avoid parsing conflicts. I wonder if it is really reasonable to try to support non-parenthesized forms ~x:e, ~y:e, ... (in expressions), ~x:p, ~y:p, ... (in patterns) and x:ty * y:ty * ... (in types).

Have you considered restricting labelled tuples to explicitly-parenthesized forms (~x:e, ~y:e) and (x:ty * y:ty)? I'm not sure that it would actually reduce the number of LR conflicts given the rest of the current grammar (expr_comma_list already follows this structure, and supporting extra things after LPAREN may create other conflicts), but I do have the impression that it keeps the user-facing grammar simpler and less ambiguous, with less risks of cluttering the syntax space in ways we would regret later.

[ccasin]

I started looking at the code again. Most of it is fine, but the complexity of the parser changes is giving me pause. There is a lot of very clever but fairly verbose changes (that are clearly documented, thanks) to avoid parsing conflicts. I wonder if it is really reasonable to try to support non-parenthesized forms ~x:e, ~y:e, ... (in expressions), ~x:p, ~y:p, ... (in patterns) and x:ty * y:ty * ... (in types).

Have you considered restricting labelled tuples to explicitly-parenthesized forms (~x:e, ~y:e) and (x:ty * y:ty)? I'm not sure that it would actually reduce the number of LR conflicts given the rest of the current grammar (expr_comma_list already follows this structure, and supporting extra things after LPAREN may create other conflicts), but I do have the impression that it keeps the user-facing grammar simpler and less ambiguous, with less risks of cluttering the syntax space in ways we would regret later.

These are reasoanble questions. Unfortunately, requiring parens doesn't end up simplifying parsing that much. Here is an old version (based on the Jane Street branch) where @rtjoa tried this for expressions and patterns. You'll see that (ignoring comments), labeled tuple expression parsing takes 37 lines there vs 44 lines in the current PR, and labeled tuple patterns end up the same length in both versions (46 lines). Types were tried in a different branch with similar results.

The main complication is ambiguity between labeled tuples and normal tuples. We don't want to require parens around unlabeled tuples, so we must leave them alone and add labeled tuples elsewhere in the grammar under parens. But of course wherever we do that, normal tuples are also legal. To avoid this inherent ambiguity, Ryan's approach was to make the labeled tuple parser only handle cases with at least one label, and doing this without conflicts is verbose.

I think you are right that this would result in making the user facing grammar less ambiguous. But since it doesn't result in substantial parser simplifications, and comes at the cost of requiring a lot more parens, I tend to favor the current approach. In my experience, people really like the ability to write tuples without parens! Of course, you may disagree, and I'll defer to the judgment of the maintainers.

[gasche]

My inclination would be to ask a parsing expert if they have an idea on how to do better (I mentioned @fpottier earlier and I may ask him if we meet in-person on Wednesday), and otherwise follow what you have done.

[ccasin]

My inclination would be to ask a parsing expert if they have an idea on how to do better (I mentioned @fpottier earlier and I may ask him if we meet in-person on Wednesday), and otherwise follow what you have done.

Makes sense to me!

[jberdine]

FWIW just as a data point: it would be nice to have unparenthesized labeled tuples, at least in patterns.

[fpottier]

Two cents:

• Yes, as a user, I find it nice to be able to write tuples without parentheses, both in patterns and in expressions.
• As a parser author, I believe that unparenthesized tuples are actually a good thing to have, and the right way to go, because this means that parentheses have only one role, namely serving as delimiters of the thing inside them (which can be an unparenthesized tuple). This can help simplify the grammar and make it less ambiguous.

That said, I have not looked at the grammar changes in this PR; I would probably need a significant amount of time to be able to give an informed judgement about them.

[ccasin]

Oops! I was trying to rebase this and accidentally pushed the trunk commit to this branch, causing the PR to be automatically closed. Github won't let me reopen this, so I think the only solution is to open a new PR, which I will do in a moment.

[gasche]

Can you first try to reset your branch to d47c564, force-push, and see whether Github then lets you reopen?

[ccasin]

Ah, yes, it looks like this works.

[ccasin]

Hmm, this is odd - I did force push the branch back, and posted the last comment by pressing the "reopen and comment" button, but it did not reopen the PR and now that button is greyed out (it says there are no new commits on my branch, but there are). I will give it another few minutes in case github is just taking a while to notice the updated branch. (Or if you have a way to reopen it, please go ahead)

[gasche]

I don't have a way to reopen, please feel free to send a new PR.

[ccasin]

After rebasing and pushing, I was able to reopen. It seems like it didn't want to consider the previous commit hashes to be "new".

[lpw25]

IIUC the current state is that we are good to merge other than Gabriel's comment that amounts to "Maybe we should ask a parsing expert if there is a way to tidy up the difficult parsing logic". I personally suspect that it is hard to do better than what is in here -- mostly just because I know Chris already tried fairly hard to do so -- so I propose we merge now and a parsing expert can always come and try and improve things later if they find the time. Unless anyone disagrees with that assessment in the next couple of days, I'm going to press merge.

[gasche]

I agree -- I am also guessing that it is hard to do better than what there is in this PR, and I was going to suggest essentially the same thing.