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

Not enough injectivity with --cubical #6047

Closed
plt-amy opened this issue Aug 22, 2022 · 6 comments · Fixed by #6219
Closed

Not enough injectivity with --cubical #6047

plt-amy opened this issue Aug 22, 2022 · 6 comments · Fixed by #6219
Assignees
Labels
cubical Cubical Agda paraphernalia: Paths, Glue, partial elements, hcomp, transp documented-in-changelog Issues already documented in the CHANGELOG injectivity Injectivity analysis for functions, application of injectivity during conversion regression on master Unreleased regression in development version (Change to "regression in ..." should it be released!)
Milestone

Comments

@plt-amy
Copy link
Member

plt-amy commented Aug 22, 2022

Minimized from a build failure in the cubical library (vindicated: I was sure it wasn't my inS/outS elision code).

{-# OPTIONS --cubical #-}

module test where

open import Agda.Builtin.Cubical.Path
open import Agda.Builtin.Nat
open import Agda.Primitive

data Fin : Nat  Set where
  zero : {n : Nat}  Fin (suc n)
  suc  : {n : Nat} (i : Fin n)  Fin (suc n)

data  : Set where

toℕ :  {n}  Fin n  Nat
toℕ zero    = 0
toℕ (suc i) = suc (toℕ i)

postulate
  inj-toℕ : {n : Nat} {k l : Fin n}  (toℕ k ≡ toℕ l)  k ≡ l
  isSetℕ : (x y : Nat) (p q : x ≡ y)  p ≡ q

ap :  {A : Set} {B : A  Set} (f :  x  B x)   {x y : A} (p : x ≡ y)
    PathP (λ i  B (p i)) (f x) (f y)
ap f p i = f (p i)

inj-cong : {n : Nat} {k l : Fin n}  (p : toℕ k ≡ toℕ l)  ap toℕ (inj-toℕ p) ≡ p
inj-cong p = isSetℕ _ _ _ _

2.6.2.2 is fine, master yells

Failed to solve the following constraints:
  toℕ _y_72 = toℕ l : Nat (blocked on _y_72)
  toℕ _x_71 = toℕ k : Nat (blocked on _x_71)

because toℕ has hcomp/trX clauses and won't be marked injective.

@plt-amy plt-amy added injectivity Injectivity analysis for functions, application of injectivity during conversion regression on master Unreleased regression in development version (Change to "regression in ..." should it be released!) cubical Cubical Agda paraphernalia: Paths, Glue, partial elements, hcomp, transp labels Aug 22, 2022
@andreasabel andreasabel added this to the 2.6.3 milestone Aug 23, 2022
@jespercockx
Copy link
Member

Would it be sound to ignore the generated hcomp/trX clauses for the purpose of injectivity analysis? @Saizan @mortberg ?

@plt-amy
Copy link
Member Author

plt-amy commented Aug 23, 2022

For reference here's the commit that caused this: ff2e436

and the two issues it solves: #5579 #5576

@andreasabel andreasabel added the language change Changes to the language which can be observed by Agda's userbase label Oct 20, 2022
plt-amy added a commit that referenced this issue Oct 24, 2022
* re #6047: don't invert hcomp/transp/path constructors

* fixup: give up for injectivity w/ IApply early

* fixup: line-wrapping-induced haddock failure
@andreasabel
Copy link
Member

@plt-amy : Currently this issue is both labeled language change (suggesting there should be a changelog entry explaining the change) and regression on master (suggesting it should not be in the changelog at all).
Could you help sort out this paradox, and add a changelog entry if needed?

(Reopening this so that we do not lose track of this formal issue.)

@andreasabel andreasabel reopened this Oct 25, 2022
@plt-amy plt-amy removed the language change Changes to the language which can be observed by Agda's userbase label Oct 25, 2022
@plt-amy
Copy link
Member Author

plt-amy commented Oct 25, 2022

Yes I can: The regression on master is as described in the issue: basically no injectivity was happening for e.g. functions that match on Fin n.

As part of the fix, the language change is that functions matching on higher inductive types are no longer considered for injectivity. Changelog for that is in 7a63062

@andreasabel
Copy link
Member

Thanks for the clarification! I think we can omit this issue from the changelog then.

@andreasabel andreasabel added the documented-in-changelog Issues already documented in the CHANGELOG label Oct 25, 2022
@andreasabel
Copy link
Member

Turns out that its PR #6219 already is mentioned in the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubical Cubical Agda paraphernalia: Paths, Glue, partial elements, hcomp, transp documented-in-changelog Issues already documented in the CHANGELOG injectivity Injectivity analysis for functions, application of injectivity during conversion regression on master Unreleased regression in development version (Change to "regression in ..." should it be released!)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants