Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split FunctionValue out of Applicable #843

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

Calvin-L
Copy link
Collaborator

@Calvin-L Calvin-L commented Nov 9, 2023

This is a refactoring-only change, and therefore very low-priority. But, it fixes a longstanding and (to me) irritating lack of type safety surrounding functions and operators.


The Applicable interface is widely used, but it is "confused" because functions and operators both implement it, even though those entities have very different capabilities and cannot be interchanged in TLA+.

This commit adds a new FunctionValue interface so that TLC's code can distinguish between operators (OpValue) and functions. Despite adding a new interface, the split actually REDUCES the number of non-comment lines of code in TLC.

A few qualitative notes about why I think this is a good refactor:

  1. Operator values have to implement getDomain() even though they do not have a domain. Therefore, knowing that a value is Applicable is not enough to prove at compile-time that it supports every Applicable method.

  2. Similarly, all operator values have to implement select(), but all of them throw an exception.

  3. Code blocks that cast objects to Applicable are overly reliant on SANY's checks to ensure that the correct kind of value flows there.

  4. Operators and functions behave differently when called with more than 1 argument: operators can have arity > 1, but functions have to wrap the arguments in a tuple.

Note that Applicable has been retained because it is still widely used in the community modules. I hope that once the community modules switch to using FunctionValue and OpValue correctly, we can completely remove Applicable and the other now-deprecated methods.

I have documented the key methods of FunctionValue and OpValue as well as I can; Applicable has no documentation I could find.

The `Applicable` interface is widely used, but it is "confused" because
functions and operators both implement it, even though those entities
have very different capabilities and cannot be interchanged in TLA+.

This commit adds a new `FunctionValue` interface so that TLC's code can
distinguish between operators (`OpValue`) and functions.  Despite
adding a new interface, the split actually REDUCES the number of
non-comment lines of code in TLC.

A few qualitative notes about why I think this is a good refactor:

 1. Operator values have to implement `getDomain()` even though they
    do not have a domain.  Therefore, knowing that a value is
    `Applicable` is not enough to prove at compile-time that it
    supports every `Applicable` method.

 2. Similarly, all operator values have to implement `select()`, but
    all of them throw an exception.

 3. Code blocks that cast objects to `Applicable` are overly reliant on
    SANY's checks to ensure that the correct kind of value flows there.

 4. Operators and functions behave differently when called with
    more than 1 argument: operators can have arity > 1, but functions
    have to wrap the arguments in a tuple.

Note that `Applicable` has been retained because it is still widely
used in the community modules.  I hope that once the community modules
switch to using `FunctionValue` and `OpValue` correctly, we can
completely remove `Applicable` and the other now-deprecated methods.

I have documented the key methods of `FunctionValue` and `OpValue` as
well as I can; `Applicable` has no documentation I could find.

Signed-off-by: Calvin Loncaric <[email protected]>
@lemmy
Copy link
Member

lemmy commented Nov 9, 2023

There are other pending changes in the CommunityModules that would make CM incompatible to older versions of TLC. At some point (today?), we could and should decide that new CommunityModules releases depend on a current version of TLC.

@lemmy
Copy link
Member

lemmy commented Nov 10, 2023

LGTM

@lemmy lemmy added enhancement Lets change things for the better Tools The command line tools - TLC, SANY, ... labels Nov 10, 2023
@Calvin-L Calvin-L merged commit b125618 into master Nov 15, 2023
@Calvin-L Calvin-L deleted the split-applicable-from-opvalue branch November 15, 2023 21:00
@Calvin-L Calvin-L mentioned this pull request Aug 13, 2024
13 tasks
Calvin-L added a commit that referenced this pull request Aug 16, 2024
This finishes the work that started in #843 (commit b125618).

Signed-off-by: Calvin Loncaric <[email protected]>
@Calvin-L Calvin-L mentioned this pull request Aug 16, 2024
Calvin-L added a commit that referenced this pull request Aug 19, 2024
This finishes the work that started in #843 (commit b125618).

Signed-off-by: Calvin Loncaric <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Lets change things for the better Tools The command line tools - TLC, SANY, ...
Development

Successfully merging this pull request may close these issues.

None yet

2 participants