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

Open goal inside record causes internal error (eta-contraction) #5478

Closed
mr-ohman opened this issue Jul 15, 2021 · 5 comments · Fixed by #5483 or #5485
Closed

Open goal inside record causes internal error (eta-contraction) #5478

mr-ohman opened this issue Jul 15, 2021 · 5 comments · Fixed by #5483 or #5485
Assignees
Labels
eta contract Problem introduced by eta-contraction meta Metavariables, insertion of implicit arguments, etc records Record declarations, literals, constructors and updates regression in 2.5.2 Regression that first appeared in Agda 2.5.2 type: bug Issues and pull requests about actual bugs ux: interaction Issues to do with interactive development (holes, case splitting, etc)
Milestone

Comments

@mr-ohman
Copy link

The following code produces an internal error:

open import Agda.Builtin.Sigma

record R1 (A : Set) : Set where

instance
  prodR1 : {A : Set} {B : A  Set}  ⦃ {a : A}  R1 (B a) ⦄  R1 (Σ A B)
  prodR1 = record {}

record R2 (A : Set) ⦃ aR1 : R1 A ⦄ : Set₁ where
  field
    f : A  Set
open R2 ⦃...⦄ public

record R3 (A : Set) (B : A  Set) : Set₁ where
  instance
    bR1 : {a : A}  R1 (B a)
    bR1 = {!!} -- record {}
  field
    ⦃ r2 ⦄ : R2 (Σ A B)
    fab :  {a} {b : B a}  f (a , b)
An internal error has occurred. Please report this as a bug.
Location of the error: __IMPOSSIBLE__, called at src/full/Agda/TypeChecking/Records.hs:734:11 in Agda-2.6.3-D2V0PkTCBiXC7IJuWrP7mz:Agda.TypeChecking.Records

If the goal of bR1 is replaced with record {} then the code type checks. This error occurred with Agda versions 2.6.2 and master branch at commit 23e07c0.

@andreasabel andreasabel added eta contract Problem introduced by eta-contraction ux: interaction Issues to do with interactive development (holes, case splitting, etc) meta Metavariables, insertion of implicit arguments, etc records Record declarations, literals, constructors and updates type: bug Issues and pull requests about actual bugs labels Jul 17, 2021
@andreasabel
Copy link
Member

The error already occurs with

record  : Set where

record R : Set₁ where
  foo : ⊤
  foo = ?  -- works with _ instead of ?
  field X : Set

@andreasabel andreasabel changed the title Instance with open goal inside record causes internal error Open goal inside record causes internal error (eta-contraction) Jul 17, 2021
@andreasabel andreasabel added the regression in 2.5.2 Regression that first appeared in Agda 2.5.2 label Jul 17, 2021
@andreasabel andreasabel added this to the 2.6.3 milestone Jul 17, 2021
@andreasabel
Copy link
Member

andreasabel commented Jul 17, 2021

Bisection says: 56c804c is the first bad commit.

Date:   Fri Jul 8 20:09:47 2016 +0100

    Add warnings machinery

(Patch comprises 14 changed files with 380 additions and 97 deletions - sigh...)
Ping @fredrikNordvallForsberg @gallais .

UPDATE: This patch isn't the problem, maybe it exposed the problem, or it is a false alarm.

@andreasabel
Copy link
Member

It seems like the crashing eta-reduction happens during this instantiateFull:

cs <- instantiateFull {- =<< mapM rebindClause -} cs

It is called on foo r = _1 r and I supposed _1 := record{} so we end up with the ill-formed record{} r.
I suppose when the meta is created during computing the type of the constructor (hello #434) there is no record variable r in the context, but when we check the declaration foo there is. This would explain why the problem does not surface with _ instead of ?. With an interaction meta, we are trying to not create two metas (one an in each path), and instead reuse the meta created on the first run-through, which happens to be created in a context without record variable r.

@andreasabel
Copy link
Member

I suppose the heuristics here:

newQuestionMark'
:: (Comparison -> Type -> TCM (MetaId, Term))
-> InteractionId -> Comparison -> Type -> TCM (MetaId, Term)
newQuestionMark' new ii cmp t = do
-- Andreas, 2016-07-29, issue 1720-2
-- This is slightly risky, as the same interaction id
-- maybe be shared between different contexts.
-- Blame goes to the record processing hack, see issue #424
-- and @ConcreteToAbstract.recordConstructorType@.
let existing x = (x,) . MetaV x . map Apply <$> getContextArgs
flip (caseMaybeM $ lookupInteractionMeta ii) existing $ {-else-} do

blows up in this case. We need to use a meta-variable created in one context (without the r variable) in an extended extension (with the r variable)? Is this something we can do with checkpoints @jespercockx @UlfNorell ?

@andreasabel andreasabel self-assigned this Jul 20, 2021
andreasabel added a commit that referenced this issue Jul 21, 2021
An interaction meta might be created in a context with one binding for
each record field, but then reused in a context with just a single
binding of the record variable.

This patch tries to correctly instantiate the original meta (created
during the constructor type construction run) when used during a
second type-checking run (this time, the record declarations).
andreasabel added a commit that referenced this issue Jul 21, 2021
Introduce dependencies, giving it more opportunities to fail...
andreasabel added a commit that referenced this issue Jul 21, 2021
Introduce dependencies, giving it more opportunities to fail...
andreasabel added a commit that referenced this issue Jul 21, 2021
An interaction meta might be created in a context with one binding for
each record field, but then reused in a context with just a single
binding of the record variable.

This patch tries to correctly instantiate the original meta (created
during the constructor type construction run) when used during a
second type-checking run (this time, the record declarations).
andreasabel added a commit that referenced this issue Jul 21, 2021
Introduce dependencies, giving it more opportunities to fail...
andreasabel added a commit that referenced this issue Jul 21, 2021
An interaction meta might be created in a context with one binding for
each record field, but then reused in a context with just a single
binding of the record variable.

This patch tries to correctly instantiate the original meta (created
during the constructor type construction run) when used during a
second type-checking run (this time, the record declarations).
@andreasabel
Copy link
Member

andreasabel commented Jul 22, 2021

While #5483 fixed my shrunk MWE, the OP still produces an internal error (but a new one):

record R3 (A : Set) (B : A  Set) : Set₁ where
  instance
    bR1 : {a : A}  R1 (B a)
    bR1 = {!!} 
expecting meta-creation context (with fields instead of record var)
  [B, A]
to be an expansion of the meta-reuse context (with record var)
  [a, r, B, A]
  glen  = 2
  g1len = 1
  g0len = 2
  frontMatch = False
  rearMatch  = True

It seems that on the first pass, Agda did not insert the hidden binding {a}, but on the second pass.

andreasabel added a commit that referenced this issue Jul 26, 2021
A reused meta might be under additional local assumptions, so the previous fix was not complete.

There is still a problem with --double-check remaining, I suppose because of hidden abstractions not always eagerly introduced in the same way.
andreasabel added a commit that referenced this issue Jul 28, 2021
@andreasabel andreasabel modified the milestones: 2.6.3, 2.6.2.1 Aug 18, 2021
andreasabel added a commit that referenced this issue Nov 17, 2021
Introduce dependencies, giving it more opportunities to fail...
andreasabel added a commit that referenced this issue Nov 17, 2021
An interaction meta might be created in a context with one binding for
each record field, but then reused in a context with just a single
binding of the record variable.

This patch tries to correctly instantiate the original meta (created
during the constructor type construction run) when used during a
second type-checking run (this time, the record declarations).
andreasabel added a commit that referenced this issue Nov 17, 2021
A reused meta might be under additional local assumptions, so the previous fix was not complete.

There is still a problem with --double-check remaining, I suppose because of hidden abstractions not always eagerly introduced in the same way.
andreasabel added a commit that referenced this issue Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eta contract Problem introduced by eta-contraction meta Metavariables, insertion of implicit arguments, etc records Record declarations, literals, constructors and updates regression in 2.5.2 Regression that first appeared in Agda 2.5.2 type: bug Issues and pull requests about actual bugs ux: interaction Issues to do with interactive development (holes, case splitting, etc)
Projects
None yet
2 participants