Skip to content

docs(corelib): document sortedness precondition for sorted array operations#2939

Open
giwaov wants to merge 1 commit into0xMiden:nextfrom
giwaov:docs/sorted-array-sortedness-warning
Open

docs(corelib): document sortedness precondition for sorted array operations#2939
giwaov wants to merge 1 commit into0xMiden:nextfrom
giwaov:docs/sorted-array-sortedness-warning

Conversation

@giwaov
Copy link
Copy Markdown
Contributor

@giwaov giwaov commented Mar 30, 2026

Add prominent warnings to all sorted array procedures (find_word, find_key_value, find_half_key_value, find_partial_key_value) clarifying that the input array must be sorted in non-decreasing lexicographic order.

Previously, find_word buried the sortedness requirement in a generic crash-conditions list and lacked a dedicated Panics section. The key-value procedures mentioned it under Panics but without emphasis.

Rationale

Issue #2832 notes that sorted array operations do not clearly communicate the sortedness precondition. Callers passing unsorted input get silently wrong results in the proof (the host event handler validates and returns an error, but this was not documented). This PR makes the contract explicit: sortedness is a bold, caller-ensured precondition on every procedure, with a note that the host event handler enforces it. Panics sections are limited to alignment and range checks only.

Changes

  • Add bold sortedness warning to each procedure's doc comment in the MASM source
  • Note that the host event handler validates sortedness and returns an error
  • Add a # Panics section to find_word (matching the style of the other procedures)
  • Remove duplicate Inputs/Outputs block from find_key_value doc comment
  • Regenerate sorted_array.md to match

Test plan

Docs-only change. CI check core library docs job verifies the committed markdown matches the generated output from MASM source. All 15 checks pass on the latest commit.

Fixes #2832

@giwaov giwaov force-pushed the docs/sorted-array-sortedness-warning branch from edbf9e6 to 2f1020c Compare March 30, 2026 20:24
@huitseeker huitseeker requested a review from Al-Kindi-0 April 1, 2026 16:50
#! # Panics
#!
#! Panics if:
#! - words are not sorted in non-decreasing order,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new warning says unsorted input is undefined behavior because proof verification only checks ordering near the returned pointer. Right below that, the new Panics list says unsorted input panics. Those read like two different contracts. Can we keep sortedness as a caller-ensured precondition here and limit Panics to the alignment and range checks?

Copy link
Copy Markdown
Contributor Author

@giwaov giwaov Apr 1, 2026

Choose a reason for hiding this comment

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

Done . removed the sortedness lines from all Panics sections. Sortedness is now only documented as a caller-ensured precondition in the bold warning, and Panics is limited to the alignment and range checks.

@giwaov giwaov force-pushed the docs/sorted-array-sortedness-warning branch from 2f1020c to 16b495b Compare April 1, 2026 21:19
Copy link
Copy Markdown
Contributor

@Al-Kindi-0 Al-Kindi-0 left a comment

Choose a reason for hiding this comment

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

I think that the LOWERBOUND_ARRAY_EVENT event will raise an error if the sorted assumption is not satisfied. Hence, it might make sense to reflect this

@giwaov giwaov force-pushed the docs/sorted-array-sortedness-warning branch from 16b495b to 37fdf39 Compare April 2, 2026 08:10
@giwaov
Copy link
Copy Markdown
Contributor Author

giwaov commented Apr 2, 2026

Good point. Checked the event handlers in sorted_array.rs and both handle_lowerbound_array and handle_lowerbound_key_value validate sortedness by scanning the entire array, returning SortedArrayError::NotAscendingOrder if any element is smaller than its predecessor. So it is correct to list sortedness under Panics.

Updated: removed the UB language, kept the bold precondition, and added sortedness back to all Panics sections.

Copy link
Copy Markdown
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

I think this is close — I left an inline comment.

#! # Panics
#!
#! Panics if:
#! - words are not sorted in non-decreasing order
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The contract still reads two ways here. The bold warning makes sortedness a caller precondition, but this Panics bullet turns it into a guaranteed crash. Proof verification only checks local ordering around the returned pointer.

Could we keep this the same as the earlier thread and treat sortedness as a precondition, not a Panics case? Since this PR is about making the precondition clearer, I think the simplest fix is to remove the words are not sorted... bullet here and in the generated markdown, and leave Panics for the alignment and range checks. If we want to mention the host event error, maybe that fits better in the warning text than in Panics.

@giwaov giwaov force-pushed the docs/sorted-array-sortedness-warning branch from 37fdf39 to de206e3 Compare April 3, 2026 13:23
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

Automated check (CONTRIBUTING.md)

Findings:

  • Add a short Rationale explaining why the change is needed.

Recommendations:

  • Consider adding a Test plan or clear review steps.

Next steps:

@giwaov
Copy link
Copy Markdown
Contributor Author

giwaov commented Apr 3, 2026

The latest push (de206e3) already addresses this. Sortedness is only in the bold warning as a caller-ensured precondition, with a note that the host event handler validates it and returns an error. The Panics sections are limited to alignment and range checks.

It looks like the review was based on commit 37fdf39, which still had the sortedness bullet in Panics. That was removed in the subsequent force push.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

Automated check (CONTRIBUTING.md)

Findings:

  • Add a short Rationale explaining why the change is needed.

Recommendations:

  • Consider adding a Test plan or clear review steps.

Next steps:

@giwaov giwaov force-pushed the docs/sorted-array-sortedness-warning branch from 96e8776 to 38e2c13 Compare April 3, 2026 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sorted array operations do not validate sortedness

3 participants