-
Notifications
You must be signed in to change notification settings - Fork 43
Core: Sequence bounds preconditions and VC-printer fallback fix #1100
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
base: main
Are you sure you want to change the base?
Changes from all commits
9ed6582
4a73eb3
915164a
8325c0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,12 +102,13 @@ inductive PropertyType where | |
| | assert | ||
| | divisionByZero | ||
| | arithmeticOverflow | ||
| | outOfBoundsAccess | ||
| deriving Repr, DecidableEq | ||
|
|
||
| /-- Whether an unreachable path counts as pass for this property type. | ||
| Assertions pass vacuously when unreachable; covers fail. -/ | ||
| def PropertyType.passWhenUnreachable : PropertyType → Bool | ||
| | .assert | .divisionByZero | .arithmeticOverflow => true | ||
| | .assert | .divisionByZero | .arithmeticOverflow | .outOfBoundsAccess => true | ||
| | .cover => false | ||
|
|
||
| instance : ToFormat PropertyType where | ||
|
|
@@ -116,6 +117,21 @@ instance : ToFormat PropertyType where | |
| | .assert => "assert" | ||
| | .divisionByZero => "division by zero check" | ||
| | .arithmeticOverflow => "arithmetic overflow check" | ||
| | .outOfBoundsAccess => "out-of-bounds access check" | ||
|
|
||
| /-- Convert a `MetaData` entry's property-type classification string to the | ||
| `PropertyType` enum. Falls back to `.assert` when the metadata carries | ||
| no classification or an unrecognized string; callers that emit | ||
| propertyType classifications should add a matching arm here. -/ | ||
| def convertMetaDataPropertyType {P : PureExpr} [BEq P.Ident] | ||
| (md : MetaData P) : PropertyType := | ||
| match md.getPropertyType with | ||
| | some s => | ||
| if s == MetaData.divisionByZero then .divisionByZero | ||
| else if s == MetaData.arithmeticOverflow then .arithmeticOverflow | ||
| else if s == MetaData.outOfBoundsAccess then .outOfBoundsAccess | ||
| else .assert | ||
| | none => .assert | ||
|
Comment on lines
+122
to
+134
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per your "deferring to a follow-up PR" reply on the triple-representation thread — agreed on scope. One cheap intermediate measure worth adding here, before the full /-- Encoding `p` as a MetaData propertyType string and decoding it back
yields `p` (for all non-`.assert` variants; `.assert` is the default). -/
@[simp]
theorem convertMetaDataPropertyType_roundTrip
{P : PureExpr} [BEq P.Ident] (p : PropertyType) :
p ≠ .assert →
let md : MetaData P := MetaData.fromPair MetaData.propertyType
(match p with
| .divisionByZero => MetaData.divisionByZero
| .arithmeticOverflow => MetaData.arithmeticOverflow
| .outOfBoundsAccess => MetaData.outOfBoundsAccess
| .cover => "cover" -- if .cover is ever tagged via MetaData
| .assert => "")
convertMetaDataPropertyType md = p := by
intro hne
cases p <;> simp_all [convertMetaDataPropertyType, MetaData.getPropertyType, ...]
-- the `.cover` case needs a MetaData string constant if you want it covered here,
-- or split the theorem to non-`.cover`-and-non-`.assert` PropertyTypes.(Exact simp set depends on how When you do the full consolidation follow-up, this theorem becomes the inverse-of-forward lemma and is still useful. If a new
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deferring. I prototyped the three variant round-trip theorems locally and they go through, but proving them requires adding Exposing pre-existing Secondary note: the construction in your sketch uses |
||
|
|
||
| /-- | ||
| A proof obligation can be discharged by some backend solver or a dedicated | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -499,10 +499,53 @@ def seqAppendFunc : WFLFunc CoreLParams := | |||||||||||||||
| else #true))] | ||||||||||||||||
| ]) | ||||||||||||||||
|
|
||||||||||||||||
| /- A `Sequence` selection function with type `∀a. Sequence a → int → a`. -/ | ||||||||||||||||
| /-! ### Sequence bounds preconditions | ||||||||||||||||
|
|
||||||||||||||||
| `Sequence.select` / `update` / `take` / `drop` carry bounds | ||||||||||||||||
| preconditions; the other `Sequence.*` ops are total. -/ | ||||||||||||||||
|
|
||||||||||||||||
| /-- Choice of upper-bound operator in `mkSeqBoundsPrecond`: `Lt` (strict) for | ||||||||||||||||
| `Sequence.select`/`update`, `Le` (non-strict) for `Sequence.take`/`drop`. | ||||||||||||||||
| Restricting the parameter to this inductive rather than taking an | ||||||||||||||||
| arbitrary `WFLFunc` or `LExpr` makes it impossible to attach a partial | ||||||||||||||||
| operator (which would create a nested precondition obligation) by | ||||||||||||||||
| accident. -/ | ||||||||||||||||
| private inductive SeqBoundKind where | Lt | Le | ||||||||||||||||
|
|
||||||||||||||||
| /-- Returns the *upper-bound* comparison for `mkSeqBoundsPrecond`. | ||||||||||||||||
| The lower bound is always `0 ≤ x` (see `mkSeqBoundsPrecond`), so this | ||||||||||||||||
| method characterises only the upper comparison. A future partial | ||||||||||||||||
| Sequence op requiring a non-`int` comparison (e.g. a bitvector variant) | ||||||||||||||||
| should introduce a separate helper rather than extend this enum. -/ | ||||||||||||||||
| private def SeqBoundKind.upperOpExpr : SeqBoundKind → LExpr CoreLParams.mono | ||||||||||||||||
| | .Lt => (intLtFunc (T := CoreLParams)).opExpr | ||||||||||||||||
| | .Le => (intLeFunc (T := CoreLParams)).opExpr | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Also a naming nit:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — renamed to |
||||||||||||||||
|
|
||||||||||||||||
| /-- Precondition `0 <= varName && varName `k.upperOpExpr` Sequence.length(s)`. | ||||||||||||||||
|
|
||||||||||||||||
| Assumes the enclosing function names its `Sequence a` input `"s"`. | ||||||||||||||||
| Mismatches are caught at elaboration by `polyUneval`'s `h_precond` | ||||||||||||||||
| free-vars check. -/ | ||||||||||||||||
| private def mkSeqBoundsPrecond | ||||||||||||||||
| (varName : String) (k : SeqBoundKind) : | ||||||||||||||||
| Strata.DL.Util.FuncPrecondition (LExpr CoreLParams.mono) CoreLParams.Metadata := | ||||||||||||||||
| let sVar : LExpr CoreLParams.mono := .fvar default "s" (some (seqTy mty[%a])) | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implicit coupling: Either take a second
Suggested change
and update the four call sites to Or, if you prefer to keep the helper's signature narrow, add a one-line doc note:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a docstring note — the |
||||||||||||||||
| let xVar : LExpr CoreLParams.mono := .fvar default varName (some mty[int]) | ||||||||||||||||
| let zero : LExpr CoreLParams.mono := .intConst default 0 | ||||||||||||||||
| let lenS : LExpr CoreLParams.mono := .app default seqLengthFunc.opExpr sVar | ||||||||||||||||
| let lower : LExpr CoreLParams.mono := | ||||||||||||||||
| .app default (.app default (intLeFunc (T := CoreLParams)).opExpr zero) xVar | ||||||||||||||||
| let upper : LExpr CoreLParams.mono := | ||||||||||||||||
| .app default (.app default k.upperOpExpr xVar) lenS | ||||||||||||||||
| ⟨.app default (.app default (boolAndFunc (T := CoreLParams)).opExpr lower) upper, | ||||||||||||||||
| default⟩ | ||||||||||||||||
|
Comment on lines
+529
to
+541
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proof coverage: a @[simp]
private theorem mkSeqBoundsPrecond_freeVars (varName : String) (k : SeqBoundKind) :
((mkSeqBoundsPrecond varName k).expr.freeVars).map (·.1.name) = ["s", varName] := by
cases k <;>
simp [mkSeqBoundsPrecond, LExpr.freeVars, SeqBoundKind.opExpr,
LFunc.opExpr, intLeFunc, intLtFunc, boolAndFunc, seqLengthFunc](Exact simp lemmas will depend on how each helper is defined; the pattern in With that in place, the Second, stronger theorem that I think is more valuable — see the inline on the test file — is a structural theorem theorem mkSeqBoundsPrecond_form (varName : String) (k : SeqBoundKind) :
∃ lo up,
(mkSeqBoundsPrecond varName k).expr =
.app default (.app default (boolAndFunc (T := CoreLParams)).opExpr lo) up ∧
-- lo is `0 ≤ x` with `intLeFunc` (independent of `k`)
lo = .app default (.app default (intLeFunc (T := CoreLParams)).opExpr (.intConst default 0))
(.fvar default varName (some mty[int])) ∧
-- up is `x ≤/< length(s)` with `k.opExpr`
up = .app default (.app default k.opExpr
(.fvar default varName (some mty[int])))
(.app default seqLengthFunc.opExpr (.fvar default "s" (some (seqTy mty[%a])))) := by
cases k <;> exact ⟨_, _, rfl, rfl, rfl⟩If
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Landed Deferring Independently,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up: after pushback from @joscoh here, I've dropped I probed whether the theorem catches anything unique vs the per-op pretty-print snapshots (Test 10a) and confirmed it doesn't:
The theorem was only buying earlier detection at the feat-commit boundary, not unique coverage. Dropped to avoid the redundancy. Keeping the |
||||||||||||||||
|
|
||||||||||||||||
| /- A `Sequence` selection function with type `∀a. Sequence a → int → a`. | ||||||||||||||||
| Partial: requires `0 <= i && i < Sequence.length(s)`. -/ | ||||||||||||||||
| def seqSelectFunc : WFLFunc CoreLParams := | ||||||||||||||||
| polyUneval "Sequence.select" ["a"] | ||||||||||||||||
| [("s", seqTy mty[%a]), ("i", mty[int])] mty[%a] | ||||||||||||||||
| (preconditions := [mkSeqBoundsPrecond "i" .Lt]) | ||||||||||||||||
|
|
||||||||||||||||
| /- A `Sequence` build (snoc) function with type `∀a. Sequence a → a → Sequence a`. | ||||||||||||||||
| `build(s, v)` appends a single element `v` to the end of `s`. -/ | ||||||||||||||||
|
|
@@ -555,7 +598,8 @@ def seqBuildFunc : WFLFunc CoreLParams := | |||||||||||||||
| ]) | ||||||||||||||||
|
|
||||||||||||||||
| /- A `Sequence` update function with type `∀a. Sequence a → int → a → Sequence a`. | ||||||||||||||||
| `update(s, i, v)` returns a sequence identical to `s` except at index `i` where the value is `v`. -/ | ||||||||||||||||
| `update(s, i, v)` returns a sequence identical to `s` except at index `i` where the value is `v`. | ||||||||||||||||
| Partial: requires `0 <= i && i < Sequence.length(s)`. -/ | ||||||||||||||||
| def seqUpdateFunc : WFLFunc CoreLParams := | ||||||||||||||||
| polyUneval "Sequence.update" ["a"] | ||||||||||||||||
| [("s", seqTy mty[%a]), ("i", mty[int]), ("v", mty[%a])] | ||||||||||||||||
|
|
@@ -606,6 +650,7 @@ def seqUpdateFunc : WFLFunc CoreLParams := | |||||||||||||||
| (((~Sequence.select : (Sequence %a) → int → %a) %3) %0) | ||||||||||||||||
| else #true)))] | ||||||||||||||||
| ]) | ||||||||||||||||
| (preconditions := [mkSeqBoundsPrecond "i" .Lt]) | ||||||||||||||||
|
|
||||||||||||||||
| /- A `Sequence` contains function with type `∀a. Sequence a → a → bool`. | ||||||||||||||||
| `contains(s, v)` is true iff there exists an index `i` such that `select(s, i) == v`. -/ | ||||||||||||||||
|
|
@@ -628,7 +673,8 @@ def seqContainsFunc : WFLFunc CoreLParams := | |||||||||||||||
| ]) | ||||||||||||||||
|
|
||||||||||||||||
| /- A `Sequence` take function with type `∀a. Sequence a → int → Sequence a`. | ||||||||||||||||
| `take(s, n)` returns the first `n` elements of `s`. -/ | ||||||||||||||||
| `take(s, n)` returns the first `n` elements of `s`. | ||||||||||||||||
| Partial: requires `0 <= n && n <= Sequence.length(s)`. -/ | ||||||||||||||||
| def seqTakeFunc : WFLFunc CoreLParams := | ||||||||||||||||
| polyUneval "Sequence.take" ["a"] | ||||||||||||||||
| [("s", seqTy mty[%a]), ("n", mty[int])] | ||||||||||||||||
|
|
@@ -664,9 +710,11 @@ def seqTakeFunc : WFLFunc CoreLParams := | |||||||||||||||
| (((~Sequence.select : (Sequence %a) → int → %a) %2) %0) | ||||||||||||||||
| else #true))] | ||||||||||||||||
| ]) | ||||||||||||||||
| (preconditions := [mkSeqBoundsPrecond "n" .Le]) | ||||||||||||||||
|
|
||||||||||||||||
| /- A `Sequence` drop function with type `∀a. Sequence a → int → Sequence a`. | ||||||||||||||||
| `drop(s, n)` returns the sequence with the first `n` elements removed. -/ | ||||||||||||||||
| `drop(s, n)` returns the sequence with the first `n` elements removed. | ||||||||||||||||
| Partial: requires `0 <= n && n <= Sequence.length(s)`. -/ | ||||||||||||||||
| def seqDropFunc : WFLFunc CoreLParams := | ||||||||||||||||
| polyUneval "Sequence.drop" ["a"] | ||||||||||||||||
| [("s", seqTy mty[%a]), ("n", mty[int])] | ||||||||||||||||
|
|
@@ -709,6 +757,7 @@ def seqDropFunc : WFLFunc CoreLParams := | |||||||||||||||
| (((~Int.Add : int → int → int) %0) %1)) | ||||||||||||||||
| else #true))] | ||||||||||||||||
| ]) | ||||||||||||||||
| (preconditions := [mkSeqBoundsPrecond "n" .Le]) | ||||||||||||||||
|
|
||||||||||||||||
| def emptyTriggersFunc : WFLFunc CoreLParams := | ||||||||||||||||
| nullaryUneval "Triggers.empty" mty[Triggers] | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have three parallel representations of the same enum:
MetaData.divisionByZero/arithmeticOverflow/outOfBoundsAccess—Stringconstants (MetaData.lean:293–299).propertyTypeToClassification : PropertyType → String(SarifOutput.lean:92) — the display side, kebab-case.convertMetaDataPropertyType : MetaData → PropertyType— the decode side, using the constants from (1).Adding a new
PropertyTypevariant requires changes in all three places plus a new kebab/camel string pair, and forgetting (1)+(3) silently degrades to.assertat 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:
Then
propertyTypeToClassificationandconvertMetaDataPropertyTypeboth derive from this single source, and theMetaData.*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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on the architectural observation. Deferring per your "not blocking" framing — a
PropertyType.serializationtable with the round-trip lemma you sketched is the right shape, but it touches pre-existing constants (MetaData.divisionByZeroetc.) and the SARIF classifier, so a standalone cleanup PR is cleaner than folding it in here.