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

Document unsafe operations of AssocMap #5838

Merged
merged 6 commits into from
Mar 19, 2024
Merged

Conversation

ana-pantilie
Copy link
Contributor

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Signed-off-by: Ana Pantilie <[email protected]>
Signed-off-by: Ana Pantilie <[email protected]>
@ana-pantilie ana-pantilie marked this pull request as ready for review March 15, 2024 13:19
@ana-pantilie ana-pantilie requested a review from a team March 15, 2024 13:19
-- have been checked to not contain duplicate pairs or duplicate keys, otherwise the 'Map' invariant
-- will be broken. As usual, the "keys" are considered to be the first element of the pair.
unsafeFromList :: [(k, v)] -> Map k v
unsafeFromList = Map

{-# INLINEABLE fromListSafe #-}
Copy link
Member

Choose a reason for hiding this comment

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

If the other one is called unsafeFromList, it's better to call this one safeFromList.

fromList = Map
{-# INLINEABLE unsafeFromList #-}
-- | Unsafely create a 'Map' from a list of pairs. This should _only_ be applied to lists which
-- have been checked to not contain duplicate pairs or duplicate keys, otherwise the 'Map' invariant
Copy link
Member

Choose a reason for hiding this comment

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

  • "duplicate pairs or duplicate keys" - is it not enough to just say "duplicate keys"?
  • It may not be obvious what "the 'Map' invariant will be broken" means. I think it's worth elaborating a bit.

@@ -53,15 +53,13 @@ import GHC.Generics (Generic)
import Language.Haskell.TH.Syntax as TH (Lift)
import Prettyprinter (Pretty (..))

{- HLINT ignore "Use newtype instead of data" -}

-- See Note [Optimising Value].
-- | A 'Map' of key-value pairs.
newtype Map k v = Map {unMap :: [(k, v)]}
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand the Haddock of the Map type to summarize which operations are unsafe? Otherwise people may miss the Haddock on the instances.

Signed-off-by: Ana Pantilie <[email protected]>
Signed-off-by: Ana Pantilie <[email protected]>
Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

👍

@zliu41 zliu41 enabled auto-merge (squash) March 19, 2024 17:42
@zliu41 zliu41 merged commit 5879f8e into master Mar 19, 2024
5 checks passed
@zliu41 zliu41 deleted the ana/plt-9511-audit-assocmap branch March 19, 2024 17:49
Lucsanszky added a commit to IntersectMBO/cardano-ledger that referenced this pull request Apr 8, 2024
`fromList` was deprecated in `1.24.0.0` by
IntersectMBO/plutus#5838
lehins pushed a commit to IntersectMBO/cardano-ledger that referenced this pull request Apr 8, 2024
`fromList` was deprecated in `1.24.0.0` by
IntersectMBO/plutus#5838
Lucsanszky added a commit to IntersectMBO/cardano-ledger that referenced this pull request Apr 8, 2024
`fromList` was deprecated in `1.24.0.0` by
IntersectMBO/plutus#5838
lehins pushed a commit to IntersectMBO/cardano-ledger that referenced this pull request Apr 9, 2024
* Plutus 1.25.0.0

* Replace `fromList` with `unsafeFromList`

`fromList` was deprecated in `1.24.0.0` by
IntersectMBO/plutus#5838

* Bump patch versions

---------

Co-authored-by: Lucsanszky <[email protected]>
Co-authored-by: Lucsanszky <[email protected]>
@zliu41 zliu41 mentioned this pull request Apr 29, 2024
11 tasks
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.

2 participants