refactor: remove [Lib_info.is_parameter]

[rgrinberg]

This function is unused. We should prefer pattern matching on the library kind to using this function anyway.

Signed-off-by: Rudi Grinberg me@rgrinberg.com

[shonfeder]

My request is that you close you, and I will leave a note in #12002 to consider whether there is still reason to keep this predicate after #12253 is addressed or if it should be removed there.

[rgrinberg]

As a general rule, we do not keep dead code around. Only reason it made it through is because I didn't notice it was unused.

Do you plan to use this function? If so, you can easily reintroduce it where you plan to use it.

[shonfeder]

Do you plan to use this function? If so, you can easily reintroduce it where you plan to use it.

This is already being used in the PR #12002 it was just introduced in #11963 specifically for this purpose. That is a PR you reviewed and approved.

So, this is not "dead code", it's an interface introduced as part of a larger arc of work that is still in progress.

If we end up deciding that revisions to #12002 -- in light of #12276 -- make this predicate useless, we will remove it there. But stepping on our toes to remove a function we just added, and are currently using in staged WIP, creates needless interference (concretely in the form of pointless merge conflicts).

So would you please close this for now and let us address it in #11963 ?

I'm happy to sync about this in case you feel that this imposition is critical for some reason, but IMO the easiest course of action is to let us focus on the core work here that we have in flight. I have noted this particular function for followup.

[shonfeder]

It turns out I have to touch these lines in #12276 . So if you insist that this must be removed now, let's at least do it on that WIP.

[rgrinberg]

We don't need to remove it now. If you want to keep it around temporarily to make it easier to make progress, go right ahead.

Just so we're on the same page, this function does need to go. Otherwise, the whole Lib_kind refactoring is pointless as this function just undermines the point of forcing callers to consider all the cases.

I'll keep this PR so that we don't forget to do this.

[shonfeder]

Thank you for being will to defer here! 🙏

To make sure that the pages are clearly enumerated

this function does need to go. Otherwise, the whole Lib_kind refactoring is pointless as this function just undermines the point of forcing callers to consider all the cases.

The fundamental problem with the previous implementation was a representation that allowed

assert (Lib_info.is_parameter x && Lib_info.virtual_ x)

even tho we know it to be an axiom that

assert (not (Lib_info.is_parameter x && Lib_info.virtual_ x))

I.e., it should be impossible for both predicates to be true.

The Lib_kind refactor has a point whether or not we also have predicate functions for convenience, because we are now enforcing this mutual exclusivity.

the point of forcing callers to consider all the cases

AFAIK, this cannot be enforced so long as we leave the definition of Lib_kind.t exposed, since callers can always just write Lib_kind.kind v = Dune_file Parameter or match Lib_kind.kind v with | Dune_file Parameter -> ... | _ -> ..., and if we had multiple occurrences of this kind of expression (as we in fact do), then it would be natural and help with clarity to abbreviate this in a named predicate. If we actually want to forbid this usage and guarantee exhaustive case analysis, I think we will need to make Lib_kind.t abstract to make pattern matching impossible, and then enforce the handling we want via a custom API. That seems like overkill here to me, but I am not opposed to it in principle.

Do you think I am missing something in my analysis of the logic of the types and the limits on the limitation of expression here?

Now, if as a matter of mere style, we want to discourage the use of predicate functions to abbreviate case analysis (even when that may be convenient), I am not opposed to that in principle either. Is this an existing convention that has already been agreed upon or adopted in the code base, or something you would move to introduce going forward? Either way, perhaps a PR into https://dune.readthedocs.io/en/stable/hacking.html#subjective-style-points would be in order?

[rgrinberg]

All of our techniques to keep the code robust rely on a certain level of co-operation and skill from the programmers. I don't know how to avoid that. I hope that all of our contributors would think twice before writing kind v = Parameter.

I think removing is_parameter already falls under:

Keep interfaces short & sweet. The less functions, types, etc., there are, the easier it is for users to understand, use, and ultimately modify the interface correctly. Instead of creating elaborate interfaces with the hope of future-proofing every use case, embrace change and make it easier to throw out or replace the interface.

Because this function is entirely redundant. You are welcome to expand on this point if you feel inspired.

and if we had multiple occurrences of this kind of expression (as we in fact do), then it would be natural and help with clarity to abbreviate this in a named predicate

That's what I thought when I introduced Lib_info.virtual_. Now we're in a situation where the rules are rather buggy about handling kind = Virtual and kind = Parameter in a consistent manner. Once you remove Lib_info.vritual_, those bugs should become evident.

Only a substantial amount of convenience offered by an abbreviation would be enough to justify its use. In conclusion, I'd rather reject the help and supposed clarify in favor of a codebase that is easy to grep and modify.

[art-w]

I'm in favor of merging this, the other PRs are not using is_parameter anymore as they pattern match on the library kind instead. I would prefer if this PR was merged after #12392 to avoid breaking the intermediate commits, but otherwise we could also rebase and squash #12392 :)

[rgrinberg]

Sure, feel free to merge this PR and rebase yours. Or, merge your PR and then close this one.