-
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
Internal error in rewriting #5923
Comments
The internal error is raised here:
When trying to turn a type into a pattern in a rewrite rule, patternFrom stumbles over a dummy type. This dummy is created by makeSubstitution : agda/src/full/Agda/TypeChecking/Rewriting/NonLinMatch.hs Lines 413 to 420 in d4589f6
The bug history is non-monotonous:
|
Bisection blames commit 475c7e5 (ping @jespercockx ): |
This was a fun puzzle to minimize. Here is the result of about an hour of minimization work: {-# OPTIONS --rewriting #-}
open import Agda.Builtin.Equality
open import Agda.Builtin.Equality.Rewrite
open import Agda.Builtin.Unit
postulate
A : Set
f : {X : Set} → X → A
g : {X : Set} → A → X
rew-fg : {X : Set} (a : A) → f (g {X} a) ≡ a
{-# REWRITE rew-fg #-}
postulate
h : A → ⊤
rew-hf : h (f tt) ≡ tt
{-# REWRITE rew-hf #-} Error message:
Now I just need to fix the issue... |
Here's an even smaller version: {-# OPTIONS --rewriting -vrewriting:70 #-}
open import Agda.Builtin.Equality
open import Agda.Builtin.Equality.Rewrite
open import Agda.Builtin.Unit
postulate
A : Set
a : A
f : {X : Set} → X → A
g : {X : Set} → A → X
rew-fg : {X : Set} (a : A) → f (g {X} a) ≡ a
{-# REWRITE rew-fg #-}
test : f tt ≡ a
test = refl Debug output:
It seems that the matcher is eta-expanding the term |
I'm honestly at a loss of what should be the right behaviour here. Essentially, what we have here are two functions Other things we could consider doing to solve this:
I think for now I will go ahead and implement option 1, which should at least get rid of the internal error and replace it with a proper error message. |
… terms when variables are not bound after matching
I have pushed a fix to 73d146b, will merge it once CI is finished. Good news for @mikeshulman : with this patch, the example in the OP actually goes through without errors. However, please be mindful of the fact that you might still get spurious type errors with these rules as I explained above. |
…ms when variables are not bound after matching
Continued in #5929 |
Thanks! I agree that option 1 is better. |
I'm happy to report that Agda master with the patch also works without errors for my original much longer development. Thanks again for the very quick fix. |
I have the impression that by having syntactical matching instead of matching modulo eta this kind of problem could be prevented.
@jespercockx , I remember you have already explained this to me, but what was the reason why matching modulo eta was essential? |
…acs leg. WIP [ Makefile ] goal continue-cubical-test; update cubical submodule Fix typo in CHANGELOG (agda#5905) [ workflows ] fix stack/Win (ICU again) (agda#5909) Stack was updated to MSYS2 2022-05-03 upstream. This solved the keyring problem, but we need to reset the cache. Provenance of where modules in Internal syntax. Qualify some prelude functions in .ghci Running stack repl failed due to these functions not being in scope [ debug ] bump some verbosity levels Fix agda#5901: use --batch when installing agda-mode for emacs [ doc ] User-manual: installation: add agda to apt-get install Replaces PR agda#5896 by @RosieBaish, thanks for the contribution. User-manual: small improvements to section on `postulate` (agda#5916) * [ doc ] User-manual: trick how to postulate sth in an expression * [ doc ] remove silly `let open POSTULATE` trick again [ cubical ] cosmetics: generalize iApplyVars [ cosmetics ] turn absV (local def) into global def [ cosmetics ] rewrite some TelView functions Fixed agda#5920. Re agda#5924: add LANGUAGE TypeOperators to pacify GHC 9.4 Re agda#5901: setup agda-mode: don't --no-site-file with --user @nad reports errors with --no-site-file for agda-mode setup. [ user-manual ] +example repeat for Streams, link copatterns->coinduction Finish @RemcoSchrijver's PR agda#5918 [ re agda#5923 ] Don't apply rewrite rule instead of generating dummy terms when variables are not bound after matching whitespacefix wip wip wip
I would also be interested in hearing more about why rewriting happens modulo eta. According to issues like #2979 and #3335, it looks like it was intentional. In my development it's important that rewriting doesn't do eta-expansion, because in my hacky implementation of HOTT that would cause infinite rewrite loops. There the "higher-dimensional explicit substitution" Currently I'm working around this by not using actual records for things like product types. Instead I assert them and their constructors and fields as postulates, with their beta-reductions and eta-contractions (!) as postulated rewrites. This sort of works, but it would be nicer to be able to use actual records. I don't know how Agda's conversion-checking algorithm works, but from a bidirectional point of view, it seems to me that rewrites are part of the reduction phase, which is separated from the eta-expansion phase. Ordinary beta-reduction doesn't happen modulo eta-equality (otherwise it too would loop), so I wouldn't expect rewrites to do so. If there are applications that really do need rewriting to happen modulo eta, perhaps it could be enabled and disabled by a flag, either globally or locally for individual rewrite rules? |
You can turn off η-equality for a given record type by using |
But I want my product and sigma-types to have eta-equality. I suppose I could make them records with ordinary eta-equality turned off, and then add eta-contraction as a rewrite on top of that? |
I don't know if that would work, but you could try. |
To be honest, I have never really considered the possibility that rewrite rules should not respect eta-equality. IMO it is a core principle of Agda that everything should be invariant under replacing a term with a definitionally equal one (modulo performance). Not considering eta-equality would be a lot simpler to implement than the current algorithm, but it would destroy the completeness of Agda's conversion checker, even when the rules are confluent. However, as noted by @thiagofelicissimo perhaps this could be fixed by a notion of "typed confluence" that considers eta-rules as well as rewrite rules. AFAIK this has not yet been done in any system so it would be interesting to discuss it further. For the use case by @mikeshulman here, I guess that either you are fine with checking confluence by hand, or you do not mind working with a non-confluent system (and accept the consequences of incomplete conversion and lack of subject reduction). This has so far not been a use case I considered while implementing rewrite rules, but it perhaps does make sense to have this notion of "simple" / "even more unsafe" rewrite rules. As I mentioned above, I do not think this would be especially difficult to implement, as it amounts to basically disabling all the type-directed parts of the matching algorithm used for |
Ordinary beta-reduction does not have to do any matching, so there is no need to consider eta-equality for it. |
That's interesting, I hadn't thought of that. |
I said:
@nad replied:
I tried, and it doesn't work. record _×_ (A B : Set) : Set where
no-eta-equality
constructor _,_
field
fst : A
snd : B
open _×_
postulate
η, : {A B : Set} (u : A × B) → (fst u , snd u) ≡ u
{-# REWRITE η, #-} Leads to
This makes some sense: I guess |
But it works with {-# OPTIONS --rewriting #-}
open import Agda.Builtin.Equality
open import Agda.Builtin.Equality.Rewrite
data _×_ (A B : Set) : Set where
_,_ : A → B → A × B
fst : {A B : Set} (u : A × B) → A
fst (a , b) = a
snd : {A B : Set} (u : A × B) → B
snd (a , b) = b
postulate
η, : {A B : Set} (u : A × B) → (fst u , snd u) ≡ u
{-# REWRITE η, #-} Succeeds. |
While the non-linear patterns of rewriting can represent projection patterns, they are not generated:
Agda hits the case agda/src/full/Agda/TypeChecking/Rewriting/NonLinPattern.hs Lines 146 to 161 in fe6b24e
It is a bit suprising that it insists on Miller patterns when the whole LHS of a rewriting rule may contain arbitrary defined symbols. |
It definitely could work, since as you say it works with datatypes and defined projections. I was assuming Agda's perspective was that for records, |
I'm trying to hack an approximation of Higher Observational Type Theory (for the curious, see my talks at the CMU HoTT seminar) in Agda using rewrite rules. It was working surprisingly well, but then I hit an internal error. I've been able to minimize the error-producing code down to the following:
In Agda 2.6.2.2 this produces
Removing
REWRITE apU
makes the error go away. Curiously, removingREWRITE Id-AP
also makes the error go away, even though I can't immediately see any connection between these two rules. Removing theCopy
/uncopy
wrapper also makes the error go away.The text was updated successfully, but these errors were encountered: