Skip to content
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

fix: reducing out-of-bounds swap! should return a, not default #3197

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Jan 18, 2024

Array.set! and Array.swap! are fairly similar operations, both
modify an array, both take an index that it out of bounds.

But they behave different; all of these return true

#eval #[1,2].set! 2 42 == #[1,2]    -- with panic
#reduce #[1,2].set! 2 42 == #[1,2]  -- no panic

#eval #[1,2].swap! 0 2 == #[1,2]    -- with panic
#reduce #[1,2].swap! 0 2 == default -- no panic

The implementations are

@[extern "lean_array_set"]
def Array.set! (a : Array α) (i : @& Nat) (v : α) : Array α :=
  Array.setD a i v

but

@[extern "lean_array_swap"]
def swap! (a : Array α) (i j : @& Nat) : Array α :=
  if h₁ : i < a.size then
  if h₂ : j < a.size then swap a ⟨i, h₁⟩ ⟨j, h₂⟩
  else panic! "index out of bounds"
  else panic! "index out of bounds"

It seems to be more consistent to unify the behaviors, and define

@[extern "lean_array_swap"]
def swap! (a : Array α) (i j : @& Nat) : Array α :=
  if h₁ : i < a.size then
  if h₂ : j < a.size then swap a ⟨i, h₁⟩ ⟨j, h₂⟩
  else a
  else a

Also adds docstrings.

Fixes #3196

`Array.set!` and `Array.swap!` are fairly similar operations, both
modify an array, both take an index that it out of bounds.

But they behave different; all of these return `true`
```

```

The implementations are
```
@[extern "lean_array_set"]
def Array.set! (a : Array α) (i : @& Nat) (v : α) : Array α :=
  Array.setD a i v
```
but
```
@[extern "lean_array_swap"]
def swap! (a : Array α) (i j : @& Nat) : Array α :=
  if h₁ : i < a.size then
  if h₂ : j < a.size then swap a ⟨i, h₁⟩ ⟨j, h₂⟩
  else panic! "index out of bounds"
  else panic! "index out of bounds"
```

It seems to be more consistent to unify the behaviors, and define
```
@[extern "lean_array_swap"]
def swap! (a : Array α) (i j : @& Nat) : Array α :=
  if h₁ : i < a.size then
  if h₂ : j < a.size then swap a ⟨i, h₁⟩ ⟨j, h₂⟩
  else a
  else a
```

Also adds docstrings.

Fixes #3196
@nomeata nomeata added the will-merge-soon …unless someone speaks up label Jan 18, 2024
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Jan 18, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 18, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc January 18, 2024 22:34 Inactive
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Jan 18, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Jan 18, 2024

Mathlib CI status (docs):

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 18, 2024
@nomeata nomeata added this pull request to the merge queue Jan 19, 2024
Merged via the queue into master with commit c0f264f Jan 19, 2024
18 of 23 checks passed
@nomeata nomeata deleted the joachim/swap-impl branch January 19, 2024 21:03
nomeata added a commit to nomeata/batteries that referenced this pull request Feb 5, 2024
this is possible after the fix in
leanprover/lean4#3197 and will hopefully help
with array bounds checking.
@somombo
Copy link

somombo commented Mar 5, 2024

@nomeata just curious .. why wasn't the fix for this inconsistency to change the implementation of Array.set! so as to make it panic instead of changing Array.swap! to make it no-longer panic

@nomeata
Copy link
Collaborator Author

nomeata commented Mar 5, 2024

Both set! and swap! do panic when run as compiled funcitons (note the export attribute), and kernel evaluation has no concept of panic, so there now both return the argument array unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN will-merge-soon …unless someone speaks up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Array.swap! reduces differently than Array.set!
3 participants