-
Notifications
You must be signed in to change notification settings - Fork 364
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
Rewrite rule on constructor uses wrong type for matching #4755
Comments
The partial fix of #3211 in 1b6ba6d by @andreasabel is not quite right: it uses |
Here is a particularly nasty variant: open import Agda.Builtin.Unit
open import Agda.Builtin.Equality
open import Agda.Builtin.Equality.Rewrite
data Box : Set → Set₁ where
box : (A : Set) → Box A
data D (A : Set) : Set₁ where
c : A → Box A → D A
postulate
any : {A : Set} → A
one : {A : Set} → D A
rew : ∀ A → c any (box A) ≡ one
{-# REWRITE rew #-}
test : c tt (box ⊤) ≡ one
test = refl This doesn't crash Agda, but the rewrite rule fails to fire:
When looking at the debug output, it is clear what goes wrong:
The type of the constructor The reason why I call this example particularly nasty is that the only way to determine the value of the parameter here is by matching the second argument of the constructor. This smells very much like a general solution would require an approach where matching problems can be solved in any order, much like we do for the LHS unifier (with higher-order matching as a fun side-dish). |
My vote for solving this is by getting rid of the feature "rewrite rules on constructors of parametrized datatypes" (and hence also "rewrite rules on constructors coming from a parametrized module"). If you really want rewrite rules on constructors, you should use indices instead so they are not erased until after typechecking. One exception we could perhaps make is to allow rewrite rules on constructors that do not make any use of the parameters in their type. This would enable the reduced example of #3538. @andreasabel You are the only known user of rewrite rules on constructors. Would this be sufficient for your use cases? |
Well, rewriting on constructors violates the invariant that constructor parameters are not needed for computation and thus need not be stored. Keeping the constructor parameters just for the sake of rewriting is likely forbiddingly expensive. My hope with rewrite rules on constructors was to get definitional quotients in some special cases in a cheap way.
It is probably ok that the rewrite rule
But in this example, the module parameter |
Right, I missed that. So then my brutal solution is not sufficient after all.
The problem is that this would lose stability of reduction under eta-conversion: Currently the matching algorithm used for rewriting is type-directed and it uses the type of the specific term that's being matched. Perhaps it would be possible to instead use the (less informative) type of the pattern, which we can make available during reduction. But this would still not solve the problem with stability under eta-conversion above. To solve that problem, a solution would perhaps be to require that the type of each "properly matching" subpattern of a rewrite rule should be a rigid type that cannot compute further when it is instantiated. For example, in your example the rule This restriction would be similar to the restriction from normal pattern matching that the type of each constructor pattern is a datatype which does not compute further. With this restriction, it would be safe to use the type of the pattern instead of the type of the term under match. I will try to see if I can make this work, and how restrictive it is in practice. |
I fixed the problem in the OP by inserting dummy terms in case there are missing module parameters. However, this means rewriting is currently still not stable under eta-conversion, I will need some more time to implement the proper checks to rule out my second example. |
Here is another example that violates the rule I came up with two messages ago and causes weird behaviour as a result (from https://agda.zulipchat.com/#narrow/stream/238741-general/topic/rewriting.20eta-contraction.20for.20a.20datatype/near/289788505): {-# OPTIONS --rewriting -v rewriting:70 #-}
open import Agda.Builtin.Unit
open import Agda.Builtin.Equality
open import Agda.Builtin.Equality.Rewrite
data _⇨_ (A B : Set) : Set where
Λ : (A → B) → (A ⇨ B)
_∙_ : {A B : Set} (f : A ⇨ B) (a : A) → B
(Λ f) ∙ a = f a
_∘_ : {A B C : Set} (g : B ⇨ C) (f : A ⇨ B) → (A ⇨ C)
_∘_ {A} {B} {C} g f = Λ {A} {C} (λ x → g ∙ (f ∙ x))
postulate
Λη : {A B : Set} (f : A ⇨ B) → (Λ (λ x → f ∙ x)) ≡ f
{-# REWRITE Λη #-}
oops : {A B : Set} (f : ⊤ ⇨ B) → Set
oops {A} {B} f = {! _∘_ {A} {⊤} {B} f (Λ (λ _ → tt)) !} The term in the hole has type |
…s in parameter positions
I tried to implement the solution I suggested two posts ago, but this turns out to be a rather annoying and gnarly property to analyze. However, then I realized that the core problem here is actually that the pattern variable that is bound in the parameter position is also bound somewhere else in the pattern, i.e. it occurs non-linearly. This turns out to be easy to prevent and rules out all the bad examples while keeping the good ones. Phew! |
(This fix was indeed not released with 2.6.2.2.) |
The rewriting machinery does not correctly instantiate the parameters of a constructor when applying a rewrite rule. Here's an example where things go wrong:
This throws an error:
The text was updated successfully, but these errors were encountered: