NameChoice (disambiguation): minor refactoring

[gasche]

@trefis and myself tried to start cleaning up the fairly horrible code of NameChoice.

This PR has two independent commits (better reviewed separately) that simplify one tiny part of the error logic.

[Octachron]

The second commit is really natural, to the point that I remember writing something similar for various PRs.
The first commit makes the code more locally readable with an inline record, but I think that the link between the Name_not_found and Wrong_name exception should be made clearer.

[garrigue]

As we discussed, labels are field names, not constructor names.
So this should probably be Name_kind or Choice_kind ?

By the way, does it really need to be a module?

[gasche]

It is a module because the auxiliary functions are too hard to name properly otherwise -- we know, we tried without a module first.

[gasche]

Re. label rather than name or choice: the code had a label_of_kind function before this PR which returned either "field" or "constructor", so I think that using "label" for "either a field name or a record name" is not too wrong -- not worse than before, and not worse that the two naming choices you propose, which are too generic and unclear.

[garrigue]

As I repeat, the reason it was called label_of_kind was because the functor was obtained by generalizing code specialized for field labels. But here you define a new type independent of the functor. Please think of a correct name.

[garrigue]

What about Description_kind, then.

[trefis]

FTR I agree that Label is the wrong choice for this.

[gasche]

I personally don't see anything wrong with using label to mean "a variant constructor or a record field (or a method name or a polymorphic variant constructor or an argument label)". I don't mind using a different name if you have one to suggest, but I don't think "name" or "choice" are any good. "Choice" doesn't mean anything in the context and "Name" could be just any identifier. Label may not be ideal, but again, the existing code did use label all over the place; if it was good enough then, maybe it's good enough now?

[Octachron]

After discussing with @garrigue, a possible name would be member: the module could be named Member_kind and the label_name function would become member_name. (Other propositions included Datatype_kind, Datatype_member.)

[gasche]

I addressed all review comments, and I think that this PR is ready for approval.

[garrigue]

The comments are not addressed, and I don't see how this PR (in its current status) improves the code.

[gasche]

I don't see how this PR (in its current status) improves the code.

Come on:

• The PR replaces string literals by a proper variant
• The PR replaces exception values in a partially-invalid state by a finer-grained exception that carries the right amount of information

The PR does not change a naming choice that you made in the past and preserved in the code, but now believe that it should absolutely be changed. I'm personally fine with this naming choice and I don't think it needs to be changed. If you want to change it for a better name, send your own PR! This is orthogonal to the improvements in this one.

[gasche]

If you keep adding friction to each refactoring PRs, I'm going to stop sending more of those. Please be fair : a PR that improves the code should be allowed even if the result is not perfect by your standards. If you want the code to be perfect, please do your own refactoring work instead of making mine harder.

[gasche]

I'm always interested in gathering comments and of course @garrigue's explanations have helped improve my PRs on numerous occasion. But it is also fully justified to address certain comments by saying "er no, I don't agree with this for reason X and Y", and then we should leave it at that unless there are convincing arguments otherwise. I should not have to turn it into a fight each time.

[garrigue]

@gasche I've discussed the naming problem with @Octachron, I think we can come to an agreement on a name that will clarify explanations in the future.
I'm sorry for my short previous answer: it was misled by the Github web interface, which showed me your message, but did not show that you had removed the line breaks when I moved to the "Files changed", so I thought that you had again ignored my comment. Now I understand better your message.

[gasche]

I am not convinced by member. When we say that a record field is a member of a record, we are drawing attention to the value of the field, not its name. 1, or maybe x = 1, is a member of {x = 1; y = 2}. This intuition does not translate well to variants; why would a constructor be a "member" of a variant? The only sense I can give to this idea is that a constructor declaration is a member of a variant declaration, but this is very shallow. Here we are trying to draw attention to the name, the field name or the constructor name; but we want a name that clarifies the fact that we are not talking about any kind of names, but those specific names that are used to project out of records and inject into variants -- the ones that OCaml can disambiguate. I still believe that "label" is a reasonable name for this sub-class of names.

I changed Label_kind into Name_kind, which I think is a worse name.

[Octachron]

The idea behind the member proposition is that the module Label_name is essentially building a common vocabulary between records and variants for the sake of error messages (well, for the sake of the composition of error message by the concatenation of English words). Then the word member does indeed refer to the fact that both fields and constructors are the sub-components/members of records and variants. If you don't like the name, what is your opinion on switching the focus on the datatype_kind part? Something like

module Datatype_kind = struct
  type t = Record | Variant
  ...
end

Or maybe, we should be even more explicit and tell the reader than we are defining some shared terms for disambiguated name?

module Disambiguated_name = struct
   ...
end

(Replacing the polysemous Label_kind with an even more polysemous Name_kind does not sound like a compromise to me.)

[gasche]

@trefis and myself started with a design that was Record | Variant, but then we got stuck naming the two functions which were Label_kind.label_name and Label_kind.type_name in our PR proposal.

I'm irritated by this PR and I want to get rid of it. If someone is happy enough to approve the PR (in the current state with Name_kind, or in the previous state with Label_kind; I'm happy to revert the last commit which I also dislike), let's merge it. Otherwise I will close it and let someone else's PR discuss naming to your hearth's content.

P.S.: have nice holidays, @Octachron.

[gasche]

After much grumping around, I pushed a new commit that uses the Datatype_kind suggestion. It's very mildly nicer. (The function that gives the label name is still called label_name.)

[garrigue]

Thank you for your patience.

[gasche]

Grumbl. I don't think I was very patient on this one, and I apologize for this. But I also think that there is a problem in your reviewing process that makes those refactoring PRs a test of patience, instead of a pleasant conversation to improve the code (which is how I perceived @Octachron's suggestion to factor things out in a record, which I immediately applied).

I have the impression that refactoring PRs are not exactly of the same nature as new-code PRs. In the latter, most of the code is written by the author, and they are interested in providing a new feature (so, usually, motivated to do further changes in the interest of getting the feature). In refactoring/cleanup PRs, most of the code does not come from the author, and the only thing that keeps the author motivated is their desire to makes thing slightly easier for other people in the future. (The easy, selfish way is to understand the code and leave it as-is.)

Going forward I would propose the following principle for refactoring/cleanup PRs:

"Extras are authors-decided". When further refactoring/cleanup work on the codebase is suggested by the reviewers, whether to include it in the PR or not should always be left to the PR author. It's always a good idea to suggest further changes, but the approval of the PR should not depend on whether they are performed.

(This principle does not cover new code or new design decisions made by the author of the PR, it is about improvements that are orthogonal to the cleanups in the PR itself. That said, it is probably a reasonable principle to follow more generally.)

For any PR, I would suggest the following other principle:

"Justification-strengths should match change-importance." The strength of the justification required for a code-change decision is proportional to the importance of the change. A change with a large impact should be very strongly justified in discussions with reviewers, but for a nitpick-level change the author needs few justifications (possibly just personal preference) to apply or ignore a change suggestion.

(This does not mean that whitespace or comment changes can be made completely on whims, as different whitespace/comment changes have different impact and importance. For example, an invasive whitespace change that creates conflict risks is a more impactful change, that requires stronger justification.)

[gasche]

Also: the reviewing process for refactoring/cleanup PRs should try to be fast if reasonably possible. We have a lot of code in the compiler that could benefit from more precise data structures or implementation comments, and we want to encourage people to write these PRs. If it takes more effort to get the PR reviewed than to write the cleanup commits in the first place (which is already not that easy!), it's going to be hard to motivate people to do it. The review process must of course ensure the absence of functional regressions and the fact that the cleaned-up code is actually easier to maintain, but if the answer to these two questions is "yes" there is no excellent reason to delay approving a PR -- which still lets the author iterate to take further comments into account if they wish to.