Implement array merging, take II #1229
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implement array merging to fix merging not being idempotent on array, while it's a desirable fundamental property. As we don't want to pick an arbitrary merge strategy on arrays (there are several ones, and each one might be natural in one specific situation), we do the minimal thing: we only succeed in merging arrays when they're equal (using a lazy `Equal` contract). Additional behavior will be possible once custom merge functions land.
yannham
changed the title
vkleen
approved these changes
Apr 5, 2023
Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
yannham
commented
Apr 5, 2023
vkleen
added a commit
that referenced
this pull request
Aug 9, 2023
We previously used a bespoke formatting algorithm for `Type`. I replaced the analogous code for `Term`s by the pretty printer in #1262 but we were worried about some questionable code for contract error reporting before doing the same for types. Namely, at some point it relied on hard coded string offsets for pointing at parts of types that were inferred by Nickel and consequently had no `TermPos`. In #1229 we ripped out that code and replaced it by reparsing the pretty printer output when necessary. Incidentally, this change also fixes some terms being truncated when formatted. For example, previously ``` SomeUserDefinedContract "that" "takes" "many" "arguments" ``` would be printed as `SomeUserDefinedContract "that" …`. This is somewhat useful to prevent huge screenfuls of error messages sometimes, but it makes the `Display` implementation useless for other natural purposes.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 10, 2023
) * Use the pretty printer in the `Display` implementation for `Type` We previously used a bespoke formatting algorithm for `Type`. I replaced the analogous code for `Term`s by the pretty printer in #1262 but we were worried about some questionable code for contract error reporting before doing the same for types. Namely, at some point it relied on hard coded string offsets for pointing at parts of types that were inferred by Nickel and consequently had no `TermPos`. In #1229 we ripped out that code and replaced it by reparsing the pretty printer output when necessary. Incidentally, this change also fixes some terms being truncated when formatted. For example, previously ``` SomeUserDefinedContract "that" "takes" "many" "arguments" ``` would be printed as `SomeUserDefinedContract "that" …`. This is somewhat useful to prevent huge screenfuls of error messages sometimes, but it makes the `Display` implementation useless for other natural purposes. * Revert a useless change to snapshot test inputs * Factor out calling the pretty printer for `Display`
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1187 #1226.
Implement array merging by generating an
contract.Equalapplication, ie two arrays are properly merged only if they are equal.This required to slightly rework the way contract reporting work, see #1226 (comment). This PR doesn't panic if a type doesn't have a position anymore when reporting an error, but pretty-print this type, parse it again with positions, and use this type for error reporting.
Doing so, this PR also fixes an unreported bug, which was that reporting contract failure on deeply (more than one level) type constructor was off. On master:
This underlines
Array Stringinstead ofString. This manifested through dubious code in thety_path::spanmethod. This issue is fixed by this PR and a snapshot test was added.Error reporting when merging fails
The current error reporting for a failure of merging two arrays isn't optimal. We have to choose between reporting the generated contract (
contract.Equal some_array), which is indeed the contract that failed, but is not related (position-wise) to the original merge.We can instead report the original merge location as being the position of the type (which is a bit hacky, but whatever), but then the error reporting is underlining the merge with the message "expected type". For now we went with the latter. All the information required for good error reporting is propagated, but doing so requires to rethink a bit the structure of contract error reporting. For now, we use this imperfect reporting, and plan on improving on it later. It's arguably at least viable.