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

Emit KoreJson \right-assoc for collections #3651

Closed
wants to merge 13 commits into from

Conversation

goodlyrottenapple
Copy link
Contributor

This PR modifies the encoding of lists, maps and sets to use \right-assoc{concatSymbol}(...) to remove differences in bracketing. THis should also make it easier to write a diffing application which ignores the order of/sorts the list of children.

@goodlyrottenapple goodlyrottenapple marked this pull request as ready for review September 11, 2023 08:08
@goodlyrottenapple goodlyrottenapple self-assigned this Sep 11, 2023
Comment on lines -103 to +104
. fromPattern
. from @_ @(Kore.Pattern _ (TermLike.TermAttributes VariableName))
. Kore.Syntax.Json.Internal.fromTermLike
Copy link
Member

Choose a reason for hiding this comment

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

We should completely delete Internal.fromPattern and rewrite the unit tests to use the new fromTermLike to retain the test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what the best way to go about this is tbh. fromPattern is basically unused, but we use the inverse toParsedPattern in a lot of places. The problem is we go from KorePattern to ParsedPattern which at some point gets turned into a TermLike which we then turn back into a KorePattern. Perhaps that path should be added and we keep the other unit tests??

-- If the structure has exactly one child, we just get the child.
-- This child will be an application of internalElement symbol to its arguments
-- For multiple children we will get a \right-assoc{concatSymbol}(x1,...x_n)
foldApplication Kore.Symbol{symbolConstructor = unitSymbolConstructor, symbolParams = unitSymbolParams} Kore.Symbol{symbolConstructor = concatSymbolConstructor, symbolParams = concatSymbolParams} = foldAux
Copy link
Member

Choose a reason for hiding this comment

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

For reference: https://github.com/runtimeverification/haskell-backend/blob/master/kore/src/Kore/Internal/TermLike/TermLike.hs#L1385-L1440 differs in foldApplication only.
Unfortunately there is no good way to avoid the duplication.

test/rpc-server/runTests.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@geo2a geo2a left a comment

Choose a reason for hiding this comment

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

The integration test looks good to me!

I've skimmed through the implementation, and it aligns with my intuition. Though I have to admit that my expertise here is limited.

I agree with Jost's comment about unit-testing the new fromTermLike function

Copy link
Member

@jberthold jberthold left a comment

Choose a reason for hiding this comment

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

Would still be good to come up with unit tests for Internal.fromTermLike but pre-approving

@goodlyrottenapple goodlyrottenapple removed their assignment Mar 21, 2024
@jberthold jberthold closed this Apr 15, 2024
@jberthold jberthold deleted the sam/right-assoc-collections branch April 15, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants