Core: Sequence bounds preconditions and VC-printer fallback fix

[fabiomadge]

Blocker for Laurel Seq/Array in #1073. Two related Core changes:

1. Fix VC-printer fallback for unknown 0-ary ops

handleZeroaryOps used to emit re.none() as its fallback, so VCs containing any 0-ary op outside the regex set (e.g. Sequence.empty) rendered as re.none() in printer output. Switched to mkGenericCall, matching how handleUnaryOps / handleBinaryOps already handle unknown ops.

2. Bounds preconditions for Sequence.select / update / take / drop

Following the Int.SafeDiv pattern:

Op	Precondition
Sequence.select	0 <= i && i < Sequence.length(s)
Sequence.update	0 <= i && i < Sequence.length(s)
Sequence.take	0 <= n && n <= Sequence.length(s)
Sequence.drop	0 <= n && n <= Sequence.length(s)

Sequence.length / empty / append / build / contains remain total.

PrecondElim picks the obligations up automatically at call sites in imperative code (via transformStmt) and in pure positions like requires / ensures / quantifier bodies / function bodies (via the synthetic $$wf procedures).

Obligations carry propertyType = "outOfBoundsAccess" (new MetaData constant, mirroring divisionByZero), flow through a new PropertyType.outOfBoundsAccess enum variant and the three metadata-to-PropertyType conversion sites, and render as "out-of-bounds-access" in SARIF output.

Incidental fix

While wiring the SARIF classification, I noticed propertyTypeToClassification in SarifOutput.lean was pre-existing dead code: vcResultToSarifResult never set properties.propertyType, so every obligation defaulted to "assert" in SARIF output. This PR wires it up, so divisionByZero and arithmeticOverflow obligations now also classify correctly in SARIF alongside the new outOfBoundsAccess.

Testing

New tests in StrataTest/Transform/PrecondElim.lean:

• 10 / 10c / 10d — Sequence.select in procedure body, in a requires clause (triggers mkContractWFProc), and in a function body (triggers mkFuncWFStmts).
• 11 — collectPrecondAsserts attaches outOfBoundsAccess metadata for all four partial ops and a nested Sequence.select(Sequence.update(...)) call. Mirrors OverflowCheckTest.lean.
• 12 — regression test: Sequence.empty renders as a generic call in VC printer output, not re.none().

Plus new property-classification tests in StrataTest/Languages/Core/Tests/SarifOutputTests.lean covering all five PropertyType variants.

Collateral updates to existing tests reflect the new obligations (Seq.lean), updated requires on Sequence function signatures (ProgramEvalTests.lean), and the 0-ary printer fix (Loops.lean). Note: the bounds obligations in Seq.lean appear as true && 0 < length(...) — the partial evaluator simplifies 0 <= 0 to true but does not further simplify true && X to X. This is a pre-existing evaluator gap, not newly introduced by this PR; the SMT solver discharges the obligation trivially.

Known downstream impact

PR #1073 (Laurel Seq/Array) emits Sequence.select / update / take / drop calls from its translation. Its T18_Sequences test uses #guard_msgs(drop info, error) on a diagnostics-only pipeline, so the test assertions should still pass syntactically — but individual sequence-manipulation programs now require bounds guards to fully verify. PR #1073 will need to adjust any end-to-end verification examples after this one merges. No in-repo Laurel tests are broken by this PR.

Out of scope

Parseable Sequence.empty<T>() syntax in raw Core source — a grammar-level feature for a separate PR. The printer fix here handles the display side; SMT encoding was already correct.

[joscoh]

This does not need to be a in a comment. You can just change the visibility

[fabiomadge]

Done — reverted to private and dropped the stale "Exposed" docstring. No test actually needs it public (the metadata-check test uses collectPrecondAsserts + metadata inspection instead).

[atomb]

This looks nice. There's a little room to reduce repetition, however.

[tautschnig]

Proof-coverage suggestions ("what to prove next"). These are all follow-ups, not blocking.

• convertMetaDataPropertyType_inverse_of_classifyPrecondition — for every (fn, idx) that classifyPrecondition maps to some metaString, convertMetaDataPropertyType applied to a MetaData tagged with that string returns a non-.assert PropertyType. Concretely: there shouldn't be a gap where classifyPrecondition tags a call but convertMetaDataPropertyType silently reverts to .assert. Trivial to state, would catch the string-mismatch class of regressions the triplication enables.

• PropertyType-to-SARIF round-trip — (propertyTypeToClassification p).fromClassification? = some p for all p : PropertyType, where fromClassification? is a matching deserializer (not yet written). Would make PropertyType the single source of truth and mechanically prune the MetaData.* string constants.

• mkSeqBoundsPrecond_form — a structural theorem about (mkSeqBoundsPrecond x k).expr: it equals boolAnd (intLe 0 x) (k.opExpr x (seqLength s)) (or the concrete LExpr shape). Makes Test 10's off-by-one blind spot (see inline comment #2) explicit and proved rather than merely tested. Small.

• seqSelect_denote_partial — semantic soundness of the bounds precondition with respect to Lean's List.get?: 0 ≤ i < s.length → (seqSelect s i).denote = .some (s.get! i). This is FactoryCorrect.lean territory (cf. PR #1103) rather than this PR, but worth tracking as the obvious continuation: the preconditions added here are only useful insofar as they correspond to a semantic partiality.

• collectPrecondAsserts-spec — a purely structural theorem that for every sub-expression that is a call to a function with N preconditions, collectPrecondAsserts produces exactly N assert statements, each tagged by classifyPrecondition. Test 10 verifies counts 1, 1, 1, 1, and 2 empirically; a theorem would subsume all of that and catch e.g. a future refactor that stops visiting nested calls.

Refactoring opportunities (not blocking).

• As noted in finding #3, consolidate MetaData.{divisionByZero,...} / propertyTypeToClassification / convertMetaDataPropertyType into a single table.
• The new assertOutOfBoundsObligations helper in the tests is a good pattern; the pre-existing overflow/division tests in the same file (Test 1, Test 3, Test 5) could share a sibling helper, reducing per-test boilerplate uniformly.

[tautschnig]

Carryover from joscoh's thread at this location (#1100 (comment)...). The reply says:

Done — reverted to private and dropped the stale "Exposed" docstring. No test actually needs it public (the metadata-check test uses collectPrecondAsserts + metadata inspection instead).

But the code in bbf0d27f7 still has def classifyPrecondition (public in the module sense) and still carries the "Exposed (not private)" paragraph. I grepped the tree: the only callers of classifyPrecondition are line 113 in this same file — the tests use collectPrecondAsserts + md.getPropertyType as the reply says — so private is genuinely safe. This is presumably an unpushed intent.

Suggested replacement for lines 64–70:

Suggested change

	/-- Classify a function precondition into a property type for SARIF reporting.
	For functions with multiple preconditions (e.g., SafeSDiv has both div-by-zero
	and overflow), the precondition index distinguishes them. -/
	private def classifyPrecondition (funcName : String) (precondIdx : Nat := 0) : Option String :=
	and overflow), the precondition index distinguishes them.
	Exposed (not `private`) so that tests can verify the classification for each
	partial op without having to inspect metadata on generated asserts. -/
	def classifyPrecondition (funcName : String) (precondIdx : Nat := 0) : Option String :=
	/-- Classify a function precondition into a property type for SARIF reporting.
	For functions with multiple preconditions (e.g., SafeSDiv has both div-by-zero
	and overflow), the precondition index distinguishes them. -/
	private def classifyPrecondition (funcName : String) (precondIdx : Nat := 0) : Option String :=

[tautschnig]

Implicit coupling: mkSeqBoundsPrecond hard-codes "s" here, which is only correct because the four callers (seqSelectFunc, seqUpdateFunc, seqTakeFunc, seqDropFunc) all happen to name their sequence input "s". The h_precond proof obligation on polyUneval catches mismatches at definition time — so this isn't a correctness trap, but the constraint isn't documented at the point where it bites.

Either take a second seqVarName parameter (symmetric with the existing varName for the index/count):

Suggested change

	let sVar : LExpr CoreLParams.mono := .fvar default "s" (some (seqTy mty[%a]))
	/-- Precondition `0 <= varName && varName `k.opExpr` Sequence.length(seqVarName)`. -/
	private def mkSeqBoundsPrecond
	(seqVarName : String) (varName : String) (k : SeqBoundKind) :
	Strata.DL.Util.FuncPrecondition (LExpr CoreLParams.mono) CoreLParams.Metadata :=
	let sVar : LExpr CoreLParams.mono := .fvar default seqVarName (some (seqTy mty[%a]))
	let xVar : LExpr CoreLParams.mono := .fvar default varName (some mty[int])

and update the four call sites to mkSeqBoundsPrecond "s" "i" .Lt etc. — slightly more verbose at the call sites, but the helper's contract is now self-describing.

Or, if you prefer to keep the helper's signature narrow, add a one-line doc note: Assumes the enclosing function names its sequence input "s".

[tautschnig]

assertOutOfBoundsObligations checks the obligation count and the metadata tag, but not the obligation content. That means an off-by-one regression in mkSeqBoundsPrecond — say, someone flips intLtFunc to intLeFunc for the .Lt case, producing ... && i <= length(s) for Sequence.select — slides past Test 10 entirely (count and tag both stay right).

The proof-coverage suggestion in the main review body (mkSeqBoundsPrecond_form) is the strong version. A lighter-weight test-level check that would catch this class of regression cheaply:

/-- Check that `collectPrecondAsserts` produces exactly `expectedCount`
    obligations from `expr`, each tagged with `outOfBoundsAccess` and
    each having the expected `boolAnd`-of-two-int-comparisons shape
    (so off-by-one regressions in `mkSeqBoundsPrecond` are caught). -/
private def assertOutOfBoundsObligations
    (expr : Core.Expression.Expr) (expectedCount : Nat) : IO Unit := do
  let stmts := collectPrecondAsserts Core.Factory expr "test" #[]
  assert! stmts.length == expectedCount
  for s in stmts do
    match s with
    | Statement.assert _ e md =>
      assert! md.getPropertyType == some MetaData.outOfBoundsAccess
      -- The obligation is `boolAnd (intLe 0 _) (intLt|intLe _ (seqLength _))`.
      match e with
      | LExpr.app _ (LExpr.app _ andOp _) _ =>
        assert! andOp.isBoolAnd  -- pseudo; real predicate will depend on LExpr shape
      | _ => assert! false
    | _ => assert! false

(Exact predicate isBoolAnd would need spelling out; the intent is to fail loudly if the top-level constructor stops being boolAnd.) Happy to prepare a concrete patch if you'd like.

[tautschnig]

We now have three parallel representations of the same enum:

1. MetaData.divisionByZero / arithmeticOverflow / outOfBoundsAccess — String constants (MetaData.lean:293–299).
2. propertyTypeToClassification : PropertyType → String (SarifOutput.lean:92) — the display side, kebab-case.
3. convertMetaDataPropertyType : MetaData → PropertyType — the decode side, using the constants from (1).

Adding a new PropertyType variant requires changes in all three places plus a new kebab/camel string pair, and forgetting (1)+(3) silently degrades to .assert at runtime — there's no compile-time check that (1) and (3) stay in sync.

One way to consolidate: a single definition that tabulates the serialization plus inverse:

/-- Canonical mapping from `PropertyType` to the pair of (metadata tag, SARIF classification) strings. -/
def PropertyType.serialization : PropertyType → Option (String × String)
  | .divisionByZero     => some ("divisionByZero",     "division-by-zero")
  | .arithmeticOverflow => some ("arithmeticOverflow", "arithmetic-overflow")
  | .outOfBoundsAccess  => some ("outOfBoundsAccess",  "out-of-bounds-access")
  | .cover              => some ("cover",              "cover")
  | .assert             => none

Then propertyTypeToClassification and convertMetaDataPropertyType both derive from this single source, and the MetaData.* constants can go. A #[simp] round-trip lemma (fromClassification? (toClassification p) = some p) gives compile-time coverage of the set. See the proof-coverage suggestion in the main review body.

[MikaelMayer]

🤖 Clean PR. The convertMetaDataPropertyType extraction is a good de-duplication, SeqBoundKind is a nice way to prevent nested-precondition accidents, and the SARIF property-type wiring fix is a genuine improvement (was dead code before). Test coverage is solid.

No new issues beyond what's already been flagged in existing threads.

[MikaelMayer]

🤖 Nit: "Previously, the handleZeroaryOps fallback silently substituted re.none()." — this is a non-stateless comment referencing the old behavior. Consider rephrasing to describe what the test verifies without referencing the prior state, e.g.:

-- Test: unknown 0-ary operation renders as a generic call (e.g.
-- `Sequence.empty`, which is registered in the factory but not yet
-- parseable in raw Core source).