005 keywords maps sets

[akollegger]

This pull request introduces foundational support for keywords, maps, and sets in Pattern Lisp. It updates documentation to reflect their implementation, adds comprehensive specifications and contracts for their syntax and behavior, and provides example usages. These changes establish the data model, error handling, and function contracts for these new language features.

Language Feature Implementation

• Updated documentation (docs/pattern-lisp-syntax-conventions.md) to indicate that keywords, maps, and sets are now implemented, with details on their syntax and usage.
• Added an examples file (examples/keywords-maps-sets.plisp) demonstrating how to use keywords, maps, and sets, including map/set operations and combined structures.

Specification and Contracts

• Added a requirements checklist (specs/005-keywords-maps-sets/checklists/requirements.md) confirming the specification is complete, testable, and ready for planning.
• Introduced detailed contracts (specs/005-keywords-maps-sets/contracts/README.md) for the new primitives and their operations, specifying expected input/output, error conditions, and serialization requirements.
• Defined the data model (specs/005-keywords-maps-sets/data-model.md) for keywords, maps, and sets, including type hierarchies, validation rules, serialization, and error conditions.

Documentation and Planning Updates

• Updated feature planning and documentation rules to note that 005-keywords-maps-sets uses only in-memory data structures and has added Haskell (GHC 9.10.3) as its implementation language. (.cursor/rules/specify-rules.mdc) [1] [2]

[Copilot]

Pull request overview

This pull request implements foundational support for keywords, maps, and sets in Pattern Lisp. The implementation adds three new data types with comprehensive parsing, evaluation, and serialization support, along with complete test coverage following TDD principles.

Key changes:

• Added keyword syntax (name:) as self-evaluating values for use as map keys
• Implemented map literals ({key: value}) with keyword keys and map operation primitives
• Added set literals (#{1 2 3}) with set operation primitives and Subject label support

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/PatternLisp/Syntax.hs	Extended type system with Keyword, VKeyword, VMap, VSet and added Ord instance for Value
src/PatternLisp/Parser.hs	Added parsers for keyword, map, and set literal syntax
src/PatternLisp/Eval.hs	Implemented evaluation logic for new types and primitive operations
src/PatternLisp/Primitives.hs	Registered map and set operation primitives in initial environment
src/PatternLisp/Codec.hs	Added serialization/deserialization support for keywords, maps, and sets
test/PatternLisp/ParserSpec.hs	Parser tests for new syntax
test/PatternLisp/EvalSpec.hs	Evaluation tests for new types
test/PatternLisp/PrimitivesSpec.hs	Operation tests for map and set primitives
test/PatternLisp/CodecSpec.hs	Serialization round-trip tests
test/PatternLisp/GramSpec.hs	Integration tests for Subject labels
test/Properties.hs	Property-based tests for operations and serialization
examples/keywords-maps-sets.plisp	Usage examples for new features
docs/pattern-lisp-syntax-conventions.md	Updated documentation to reflect implementation status
specs/005-keywords-maps-sets/*	Complete specification with contracts, data model, tasks, and planning

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Duplicate pattern matching for primitiveFromName \"contains?\". The first pattern (line 247) will always match, making the second pattern (line 261) unreachable. This prevents MapContains from ever being used. The function should use a single primitive that handles both sets and maps, or the name mapping needs to be refactored to return the correct primitive based on context.

Suggested change

	primitiveFromName "contains?" = Just SetContains
	primitiveFromName "set-contains?" = Just SetContains

[Copilot]

The KeywordKey newtype wrapper appears redundant since map values use VKeyword directly in the evaluator (lines 107, 214 in Eval.hs) but the map type signature uses Map.Map KeywordKey Value. This creates unnecessary conversion overhead. Consider using VKeyword directly as map keys or document why the wrapper provides value beyond type safety.

Suggested change

	-- | Keyword type for map keys (newtype wrapper for type safety)
	-- | Keyword type for map keys.
	--
	-- This is intentionally a distinct newtype instead of using 'String' or
	-- 'VKeyword' directly, so that:
	--
	-- * The type of map keys clearly communicates that 'VMap' only ever uses
	-- keyword-like keys (no arbitrary strings or symbols).
	-- * Functions that construct or consume maps cannot accidentally mix keyword
	-- keys with other 'String'-based keys, because the types differ.
	-- * If we later enrich keyword keys with additional structure or invariants,
	-- we can do so here without changing the public 'Value' representation.
	--
	-- The small conversion cost between 'VKeyword' and 'KeywordKey' in the
	-- evaluator is therefore accepted in exchange for clearer, safer types.

[Copilot]

The property test expects evalExpr to return VMap after an assoc operation, but the actual return type should be the value retrieved by get, not the map itself. The pattern match on line 217 will always fail because get returns a value, not a map. The test logic needs to be corrected to properly verify the assoc then get property.

Suggested change

	Right (VMap resultMap) -> Map.lookup (KeywordKey keyStr) resultMap
	Right v -> Just v

[Copilot]

The symbol parser uses nested try combinators and lookahead checks (notFollowedBy), which can be inefficient. Consider reordering parsers in atomParser (line 63) to try more specific patterns first (keywords, numbers) before falling back to symbols, reducing the need for backtracking.

[Copilot]

Map literal evaluation processes pairs recursively but builds the map in reverse order (inserting into restMap which is processed from the tail). While this works due to Map.insert overwriting, it's less efficient than building left-to-right with folding. Consider using foldM over pairs for clearer intent and potentially better performance.

Suggested change

	processPairs [] = return Map.empty
	processPairs (k:v:rest) = do
	keyVal <- eval k
	valVal <- eval v
	restMap <- processPairs rest
	case keyVal of
	VKeyword name -> return $ Map.insert (KeywordKey name) valVal restMap
	_ -> throwError $ TypeMismatch ("Map keys must be keywords, got: " ++ show keyVal) keyVal
	processPairs _ = throwError $ ParseError "Map literal must have even number of elements (key-value pairs)"
	processPairs exprs = go Map.empty exprs
	where
	go :: Map.Map KeywordKey Value -> [Expr] -> EvalM (Map.Map KeywordKey Value)
	go acc [] = return acc
	go acc (k:v:rest) = do
	keyVal <- eval k
	valVal <- eval v
	case keyVal of
	VKeyword name -> go (Map.insert (KeywordKey name) valVal acc) rest
	_ -> throwError $ TypeMismatch ("Map keys must be keywords, got: " ++ show keyVal) keyVal
	go _ _ = throwError $ ParseError "Map literal must have even number of elements (key-value pairs)"

[Copilot]

The get-in test comments indicate that the intended test case is not actually being tested due to keyword conversion issues with quoted lists. The test falls back to a simple get operation instead. A proper test for get-in with a keyword path should be added to ensure this functionality works correctly.