feat: Implement PriorityQueue<T> collection

[clank-hayen]

Summary

• Adds PriorityQueue<T> collection type with max heap (default) and min heap support
• Max heap: PriorityQueue<T>() - highest value first
• Min heap: PriorityQueue<T>.min() - lowest value first
• Custom comparison via less_than method on structs
• Methods: push, pop, top, length, is_empty

Test plan

• Max heap with integers: push/pop/top order verification
• Min heap with integers: push/pop/top order verification
• length and is_empty methods
• Custom struct with less_than method (max and min heap)
• Float values
• top does not remove element

Files changed

• Lexer: PRIORITY_QUEUE token
• Parser: PriorityQueueCreate AST node with .min() support
• Typechecker: priority_queues.hpp/cpp and check_priority_queue.cpp
• Codegen: emit_priority_queue.cpp
• Runtime: bishop::MaxPriorityQueue and bishop::MinPriorityQueue templates
• Tests: test_priority_queue.b
• Docs: README.md Language Reference

Closes #39

🤖 Generated with Claude Code

[clank-hayen]

REVIEW

Summary

Well-implemented PriorityQueue collection with solid max/min heap support and custom comparison via less_than method.

Critical Issues

None found.

Suggestions (Non-blocking)

1. Empty queue access behavior - The pop() and top() methods in runtime/collections/priority_queue.hpp don't check for empty state before accessing elements. While this is consistent with std::priority_queue behavior (undefined behavior on empty queue), it might be worth documenting this clearly or considering a pop_or_none() method for safer access patterns in future versions.

2. const_cast usage - The comparators use const_cast on less_than() calls. The comment explains this is because Bishop methods don't have const qualifiers yet. This is acceptable as a workaround but worth tracking for when const methods are added to the language.

What Was Reviewed

Parser/Lexer:

• ✅ PRIORITY_QUEUE token added to lexer
• ✅ PriorityQueueCreate AST node with line number tracking
• ✅ Parse handles both PriorityQueue<T>() and PriorityQueue<T>.min() syntax
• ✅ Proper error message for unsupported static methods

Type Checker:

• ✅ check_priority_queue.cpp validates element types
• ✅ priority_queues.cpp defines all method signatures (push, pop, top, length, is_empty)
• ✅ Type substitution for T placeholder works correctly
• ✅ Proper error reporting for unknown methods and type mismatches

Code Generation:

• ✅ emit_priority_queue.cpp emits correct C++ for all methods
• ✅ emit_type.cpp maps PriorityQueue<T> to bishop::PriorityQueueBase<T>
• ✅ Method call routing in emit_method_call.cpp

Runtime:

• ✅ SFINAE-based has_less_than detection for custom comparators
• ✅ Separate comparators for max/min heaps with both primitives and custom types
• ✅ Clean template metaprogramming using std::conditional_t
• ✅ Proper docstrings on all methods and classes

Documentation:

• ✅ README.md updated with comprehensive PriorityQueue section
• ✅ Examples for max heap, min heap, and custom structs with less_than
• ✅ Method reference table included

Testing:

• ✅ Tests cover max heap with int
• ✅ Tests cover min heap with int
• ✅ Tests cover length/is_empty
• ✅ Tests cover custom struct with less_than
• ✅ Tests cover float values
• ✅ Tests verify top() doesn't remove elements

Security Check (OWASP)

No security concerns - this is a pure data structure implementation with no external input handling, network access, or file I/O.

Decision: APPROVE ✅

The implementation is correct, well-documented, and follows the codebase patterns. The test coverage is comprehensive. Good work on the SFINAE-based comparator selection!

[clank-hayen]

REVIEW

Summary: Clean implementation of PriorityQueue with max/min heap support and custom comparison via less_than method.

Security Review (OWASP Top 10)

• ✅ No injection vulnerabilities
• ✅ No authentication/authorization concerns
• ✅ No sensitive data handling
• ✅ No external input parsing that could be exploited
• ✅ No SSRF or deserialization risks

Code Quality Assessment

Strengths:

• Well-structured implementation following existing patterns for collections (List, Set, Map)
• Proper separation: lexer → parser → typechecker → codegen → runtime
• Good use of C++17 features (std::conditional_t, std::void_t for SFINAE)
• Comprehensive test coverage including max heap, min heap, custom structs, and floats
• Clear documentation in README matching the API
• AST node properly sets line number

Implementation Details:

• PriorityQueue<T>() creates max heap (default)
• PriorityQueue<T>.min() creates min heap
• Custom comparison via less_than method detected at compile-time using SFINAE
• Correct heap semantics: max heap uses <, min heap uses > (inverted for std::priority_queue)

Suggestions (Non-blocking)

1. Empty queue behavior: pop() and top() on empty queues will cause undefined behavior (same as C++ std::priority_queue). This is consistent with Bishop's List.pop() behavior, so it's a design choice, not a bug. Consider documenting this in README.

2. Rebase needed: Branch is behind master by 2 commits (including PR #130 which changed error handling syntax). You'll need to rebase before merging to resolve conflicts.

3. const_cast usage: The const_cast in comparators is necessary because Bishop methods aren't const-qualified. The comment explaining this is helpful. Long-term, consider adding const method support to Bishop.

Test Coverage

• ✅ Max heap with integers
• ✅ Min heap with integers
• ✅ Empty/length checks
• ✅ Custom struct with less_than method
• ✅ Float values
• ✅ top() without removal

Decision: APPROVE ✅

The implementation is correct, follows existing patterns, and has good test coverage. The code is production-quality.

---

@chrishayen - Please review when you have a chance. Note: rebase onto master needed before merge.

[clank-hayen]

REVIEW

summary: Well-implemented PriorityQueue collection with clean design and good test coverage.

Critical Issues

None found.

Security Review (OWASP Top 10)

• A01 Broken Access Control: N/A - no authorization in language runtime
• A02 Cryptographic Failures: N/A - no crypto operations
• A03 Injection: N/A - no user input processing
• A04 Insecure Design: N/A - standalone collection type
• A05 Security Misconfiguration: N/A
• A06 Vulnerable Components: Uses std::priority_queue - standard library, no CVEs
• A07-A10: N/A for this change

No security concerns identified.

Code Quality Review

Runtime Implementation (priority_queue.hpp)

• Clean separation of MaxPriorityQueue and MinPriorityQueue using template metaprogramming
• Proper use of SFINAE (has_less_than_v) to detect custom less_than methods
• The const_cast in comparators (lines 107, 132) is documented and justified - Bishop methods currently lack const qualifiers
• Well-documented with docstrings per CLAUDE.md requirements

Parser (parse_primary.cpp, parse_type.cpp)

• Correct handling of PriorityQueue<T>() and PriorityQueue<T>.min() syntax
• Line numbers properly set on AST nodes
• Good error message for invalid constructor: "PriorityQueue<T> only supports .min() constructor"

Typechecker

• priority_queues.cpp: Clean method signatures with proper T placeholder substitution
• check_priority_queue.cpp: Proper type validation and argument checking
• Consistent with existing collection patterns (List, Tuple)

Codegen

• emit_priority_queue.cpp: Clean mapping of Bishop methods to C++ equivalents
• emit_type.cpp: Uses PriorityQueueBase<T> for type declarations, actual max/min selection happens at creation

Tests (test_priority_queue.b)

• Covers: max heap, min heap, custom struct with less_than, floats, length/is_empty
• Tests verify correct ordering for both heap types
• Tests custom comparison with priority field

Documentation

• README.md updated with comprehensive examples for max heap, min heap, and custom comparison
• Keywords section updated to include PriorityQueue

Suggestions (Non-blocking)

1. Empty queue behavior (line 55-58 of priority_queue.hpp): pop() and top() on empty queue will cause undefined behavior (underlying std::priority_queue::top() on empty is UB). This matches existing Bishop patterns (e.g., List.first() on empty list) and is consistent with the language design - users should check is_empty() first. Consider documenting this expectation in README.

2. Test coverage: Consider adding a test for calling top() multiple times to verify it doesn't mutate state (currently tests top() once before pop()). ✅ Actually, test_top_no_remove() already covers this - good job!

Decision: APPROVE

The implementation is correct, well-tested, follows existing patterns, and has no security concerns. The code is clean, well-documented, and meets the issue requirements. Good work! 🎉

🤖 Reviewed with Claude Code

[clank-hayen]

Merge conflicts have been resolved! ✅

I merged master into this branch and resolved conflicts between the PriorityQueue implementation and the Set implementation. Both collections are now included:

• PriorityQueue: Max/min heaps with push, pop, top, length, is_empty
• Set: Unique element storage with add, remove, contains, and set operations

The PR is ready for merge.

[clank-hayen]

Merge conflicts have been resolved.

The conflict in CMakeLists.txt was straightforward - master added typechecker/check_map.cpp while this PR added the PriorityQueue source files. I've merged both sets of changes.

The PR should now be mergeable.