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

Evaluate private definitions (toplevel interaction) #4647

Closed
andreasabel opened this issue May 12, 2020 · 38 comments · Fixed by #4652 or #5402
Closed

Evaluate private definitions (toplevel interaction) #4647

andreasabel opened this issue May 12, 2020 · 38 comments · Fixed by #4652 or #5402
Assignees
Labels
dead-code Concerning the dead-code removal optimization modules Issues relating to the module system regression in 2.5.1 Regression that first appeared in Agda 2.5.1 scope Issues relating to scope checking status: wontfix No action will be taken (not in changelog) type: bug Issues and pull requests about actual bugs ux: interaction Issues to do with interactive development (holes, case splitting, etc)
Milestone

Comments

@andreasabel
Copy link
Member

Private definitions cannot be evaluated in interaction top level:

open import Agda.Builtin.Nat
private n = 5

Then C-c C-n n gives an error complaining that n is not in scope.
The same problem arises with non-public open.

Here is Thorsten Altenkirch's original feature request:

I have come across this little nuisance when using agda especially when doing life hacking in lectures or talks.

I try to evaluate an expression which depends on opening some modules which I have opened in my file but agda is not aware of this. E.g.

record Stream (A : Set) : Set where
  constructor _∷_
  coinductive
  field
    hd : A
    tl : Stream A

open Stream

from : Stream ℕ
hd (from n) = n
tl (from n) = from (suc n)

And then I say C-c C-n hd (from 0) and agda responds

1,1-3
Not in scope:
  hd at 1,1-3 (did you mean 'Stream.hd'?)
when scope checking hd

Ok I can just introduce a shed

x = {!!}

and evaluate inside there but this is a bit clumsy. Isn’t there a better way to do this?

Ok I can say open Stream public – but do I want this?

@andreasabel andreasabel added type: enhancement Issues and pull requests about possible improvements modules Issues relating to the module system scope Issues relating to scope checking ux: interaction Issues to do with interactive development (holes, case splitting, etc) labels May 12, 2020
@andreasabel andreasabel added this to the 2.6.2 milestone May 12, 2020
@gallais
Copy link
Member

gallais commented May 12, 2020

May be worth having a distinct command "evaluate in context at cursor".
This way we can evaluate things on the RHS or inside a local module with
local let-bound definitions.

@jespercockx
Copy link
Member

May be worth having a distinct command "evaluate in context at cursor".
This way we can evaluate things on the RHS or inside a local module with
local let-bound definitions.

That would require us to store the context of every subterm, which I don't think is really feasible.

@UlfNorell
Copy link
Member

I do believe we have the "inside scope" available, that is the scope that would be used for any new definitions at the end of the module. It ought not be too difficult to use this instead of the "outside scope", which is what other modules importing this module sees.

@xekoukou
Copy link

I mean, what @gallais proposes is the holy grail of interactive theorem provers.
Which means that all the internal data structures need to be reconsidered with this in mind.

@nad
Copy link
Contributor

nad commented May 12, 2020

Related: #317. I wrote that 'I think the parameters should be in scope when "top-level" commands are executed, because these commands should behave in the same way as commands executed in a top-level goal at the end of the module'.

@andreasabel
Copy link
Member Author

@UlfNorell :

I do believe we have the "inside scope" available, that is the scope that would be used for any new definitions at the end of the module. It ought not be too difficult to use this instead of the "outside scope", which is what other modules importing this module sees.

We do use the inside scope. However, the private definitions are removed from the insideScope:

(insideScope, insideDecl) <- scopeCheckModule r m am tel $
toAbstract insideDecls
let scope = over scopeModules (fmap $ restrictLocalPrivate am) insideScope
setScope scope
return $ TopLevelInfo (outsideDecls ++ [ insideDecl ]) scope

You changed this in the course of the "deadcode" optimizations:

  • In c823aa9 you include a test case that defines "Not in scope" for evaluating private definitions as the specified behavior of Agda.
  • In 7f47d51 the release notes for Agda 2.4.4 state:

Private definitions of a module are no longer in scope at the Emacs mode top-level.
The reason for this change is that .agdai-files are stripped of unused private definitions (which can yield significant performance improvements for module-heavy code).
To test private definitions you can create a hole at the bottom of the module, in which private definitions will be visible.

So while the OP used to work, there was a decision made to sacrifice it for the sake of performance.

It seems there are three interest pursued which cannot all be integrated with the current architecture:

  1. The OP.
  2. To use the current module in its optimized form after serialization and deserialization.
  3. The deadcode optimization in interface files to reduce the memory consumption.

A possible fix (Alt A) could look like this: There are two different formats of the interface file, one that includes the private definitions (2) (say .agdatop) and one that doesn't (1) (.agdai). In interaction mode, the current module is then deserialized from the interface file (2), but recursive deserialization refers to the interfaces of type (1). The interactive command C-c C-l (load) would create an interface of type (2), all other instances of processing a file would create an ordinary interface (1).

An alternative fix (Alt B) would drop goal 2 and remove private definition only just before serialization. The current main module (in its internal representation) would then always be created from source, rather than loaded from an interface. However, this has the drawback that switching file in emacs would always mean that the whole file is type-checked again from scratch. @nad has complained about such behavior in the past. Yet currently, rechecking is anyway often needed because the highlighting is not reliably loaded from the interface file, see issue #3549.

Input needed from @UlfNorell (defined the current specified behavior) and @nad (expertise on import and interaction logic).

@andreasabel andreasabel changed the title Evaluate private definitions Evaluate private definitions (toplevel interaction) May 13, 2020
@UlfNorell
Copy link
Member

What about Alt B, but we add an emacs command to switch between "evaluation mode" (not using the interface for the current file) and "regular mode" (the normal behaviour). This should solve the drawback when switching files, if we reset the to regular mode when changing buffer.

@andreasabel
Copy link
Member Author

C-c C-n could switch automatically to evaluation mode.
Automatically resetting to regular mode when changing buffer makes sense, but keeping in mind that the first C-c C-n requires rechecking then. Anyway, I suppose C-c C-n is more used in demonstration situations of Agda than in serious development (I very very rarely use it at all), thus extra checking caused by C-c C-n is likely harmless!?

@UlfNorell
Copy link
Member

Also checking should be quick due to caching, no?

@andreasabel
Copy link
Member Author

Note: It seems the present issue is a duplicate of #1804.

@txa
Copy link

txa commented May 13, 2020

The other option seems to be not to strip private definitions. Why is this so important?

@andreasabel
Copy link
Member Author

For performance: #1804 (comment)

@andreasabel andreasabel self-assigned this May 13, 2020
andreasabel added a commit that referenced this issue May 13, 2020
FindFile: more precise type for toIFile
@nad
Copy link
Contributor

nad commented May 13, 2020

Another option is perhaps to switch to some kind of structured signature so that the optimisation is not necessary. This may also have other performance benefits (#4331).

@andreasabel
Copy link
Member Author

@nad:

Another option is perhaps to switch to some kind of structured signature so that the optimisation is not necessary. This may also have other performance benefits (#4331).

The problem isn't in the signature but in the scope. The private declarations get removed from the ScopeInfo when the top level module is finished scope checking. This is what I am addressing here.

@UlfNorell :

What about Alt B, but we add an emacs command to switch between "evaluation mode" (not using the interface for the current file) and "regular mode" (the normal behaviour). This should solve the drawback when switching files, if we reset the to regular mode when changing buffer.

I am implementing this, but this will be disruptive to some interaction tests that accumulate solutions for metas (e.g. Long). A top-level command, triggering a reload, will now lose these solutions since they are not written to a file. Maybe this isn't such a problem. We have a similiar problem already with the case-splitting command, which does not actually feed back to the state since it would require a reload from the updated source file.

andreasabel added a commit that referenced this issue May 13, 2020
These values helped me trace the missing highlighting introduced by the
load caused by top-level interaction commands to the agda mode, which
supplied "None" as highlighting level to these commands.
andreasabel added a commit that referenced this issue May 13, 2020
This commit addresses the problem that interaction commands that run
in the top-level, like C-c C-n, do not have access to private decls of
the main module.  These are deleted from the scope before
serialization, for reasons of performance (see #1806).

The solution implemented here is that interaction gets two modes:

1. RegularInteraction: Initial mode after C-c C-l.  The internal
representation of the top module, hence its insideScope, is loaded
from the interface file and thus has no knowledge of private
declarations.

2. TopLevelInteraction: Top-level interaction commands switch to this
mode, leading to a reload of the current top module in a way that
private declarations are preserved in the insideScope.  The module is
not serialized.

When a new file is loaded, we switch back to RegularInteraction.

The emacs mode had to be changed to switch on highlighting for
top-level commands, which was off because no highlighting would
happen.  Now, highlighting can happen due to a reload.
andreasabel added a commit that referenced this issue May 13, 2020
Due to more reloads caused by top-level commands, the interaction test output can more noisy: More status reports.
andreasabel added a commit that referenced this issue May 13, 2020
These interaction tests now produce the expected results (fruits of
fixing #4647).
andreasabel added a commit that referenced this issue May 13, 2020
These interaction tests are broken by the fix of #4647, due to the
additional reloads triggered by top-level commands.

Solutions given to metas by interaction commands get lost by reloads,
since they are not incorporated into the source file (naturally).  The
situation is now similar to why case splits do no update the state of
the interaction.
@andreasabel
Copy link
Member Author

PR #4652 for review.

@nad
Copy link
Contributor

nad commented May 13, 2020

With this change in place, if I load the file, edit it, and use a top-level command to (say) normalise an expression, will Agda reload the file (and possibly fail)? In that case I am opposed to this change.

andreasabel added a commit that referenced this issue May 18, 2020
These values helped me trace the missing highlighting introduced by the
load caused by top-level interaction commands to the agda mode, which
supplied "None" as highlighting level to these commands.
andreasabel added a commit that referenced this issue May 18, 2020
This commit addresses the problem that interaction commands that run
in the top-level, like C-c C-n, do not have access to private decls of
the main module.  These are deleted from the scope before
serialization, for reasons of performance (see #1806).

The solution implemented here is that interaction gets two modes:

1. RegularInteraction: Initial mode after C-c C-l.  The internal
representation of the top module, hence its insideScope, is loaded
from the interface file and thus has no knowledge of private
declarations.

2. TopLevelInteraction: Top-level interaction commands switch to this
mode, leading to a reload of the current top module in a way that
private declarations are preserved in the insideScope.  The module is
not serialized.

When a new file is loaded, we switch back to RegularInteraction.

The emacs mode had to be changed to switch on highlighting for
top-level commands, which was off because no highlighting would
happen.  Now, highlighting can happen due to a reload.
andreasabel added a commit that referenced this issue May 18, 2020
Due to more reloads caused by top-level commands, the interaction test output can more noisy: More status reports.
andreasabel added a commit that referenced this issue May 18, 2020
These interaction tests now produce the expected results (fruits of
fixing #4647).
andreasabel added a commit that referenced this issue May 18, 2020
These interaction tests are broken by the fix of #4647, due to the
additional reloads triggered by top-level commands.

Solutions given to metas by interaction commands get lost by reloads,
since they are not incorporated into the source file (naturally).  The
situation is now similar to why case splits do no update the state of
the interaction.
andreasabel added a commit that referenced this issue May 18, 2020
Catch not-in-scope errors in top-level interaction commands such as

  C-c C-d  infer
  C-c C-n  evaluate
  C-c C-o  module contents
  C-c C-w  why in scope

and try to handle by reloading file in TopLevelInteraction mode that
preserves the private declarations.

Caveat: commands that do not throw scope errors such as

  C-c C-z  search about

will by default (RegularInteraction) not include the private
declarations, only if some other top-level command switches to
TopLevelInteraction, then the private declarations will also be listed.
andreasabel added a commit that referenced this issue May 18, 2020
This commit addresses the problem that interaction commands that run
in the top-level, like C-c C-n, do not have access to private decls of
the main module.  These are deleted from the scope before
serialization, for reasons of performance (see #1806).

The solution implemented here is that interaction gets two modes:

1. RegularInteraction: Initial mode after C-c C-l.  The internal
representation of the top module, hence its insideScope, is loaded
from the interface file and thus has no knowledge of private
declarations.

2. TopLevelInteraction: Top-level interaction commands switch to this
mode, leading to a reload of the current top module in a way that
private declarations are preserved in the insideScope.  The module is
not serialized.

When a new file is loaded, we switch back to RegularInteraction.

The emacs mode had to be changed to switch on highlighting for
top-level commands, which was off because no highlighting would
happen.  Now, highlighting can happen due to a reload.

UPDATE: only switch to TopLevelInteraction when NotInScope error.

Catch not-in-scope errors in top-level interaction commands such as

  C-c C-d  infer
  C-c C-n  evaluate
  C-c C-o  module contents
  C-c C-w  why in scope

and try to handle by reloading file in TopLevelInteraction mode that
preserves the private declarations.

Caveat: commands that do not throw scope errors such as

  C-c C-z  search about

will by default (RegularInteraction) not include the private
declarations, only if some other top-level command switches to
TopLevelInteraction, then the private declarations will also be listed.

Note: Top-level commands that lead to a reload have to be used
with care in interaction tests.
Solutions given to metas by interaction commands get lost by reloads,
since they are not incorporated into the source file (naturally).  The
situation is now similar to why case splits do no update the state of
the interaction.

Instructions how to revert ...
------------------------------

... the changes introduced in interaction that are introduced by this
patch:

A simple `git revert` won't do since there are code improvements
contained in this patch that should not be reverted.

However, a manual revert is: remove data type

  Agda.Interaction.Base.InteractionMode

and go with the flow.  Any choices that use this bit should collapsed
as if there was only the value

  RegularInteraction
andreasabel added a commit that referenced this issue May 18, 2020
Should the new behavior of top-level interaction commands (reload on
not-in-scope errors) be disruptive to one's workflow, this option brings
back the old behavior (with private declarations not in scope).
andreasabel added a commit that referenced this issue May 18, 2020
These values helped me trace the missing highlighting introduced by the
load caused by top-level interaction commands to the agda mode, which
supplied "None" as highlighting level to these commands.
andreasabel added a commit that referenced this issue May 18, 2020
This commit addresses the problem that interaction commands that run
in the top-level, like C-c C-n, do not have access to private decls of
the main module.  These are deleted from the scope before
serialization, for reasons of performance (see #1806).

The solution implemented here is that interaction gets two modes:

1. RegularInteraction: Initial mode after C-c C-l.  The internal
representation of the top module, hence its insideScope, is loaded
from the interface file and thus has no knowledge of private
declarations.

2. TopLevelInteraction: Top-level interaction commands switch to this
mode, leading to a reload of the current top module in a way that
private declarations are preserved in the insideScope.  The module is
not serialized.

When a new file is loaded, we switch back to RegularInteraction.

The emacs mode had to be changed to switch on highlighting for
top-level commands, which was off because no highlighting would
happen.  Now, highlighting can happen due to a reload.

UPDATE: only switch to TopLevelInteraction when NotInScope error.

Catch not-in-scope errors in top-level interaction commands such as

  C-c C-d  infer
  C-c C-n  evaluate
  C-c C-o  module contents
  C-c C-w  why in scope

and try to handle by reloading file in TopLevelInteraction mode that
preserves the private declarations.

Caveat: commands that do not throw scope errors such as

  C-c C-z  search about

will by default (RegularInteraction) not include the private
declarations, only if some other top-level command switches to
TopLevelInteraction, then the private declarations will also be listed.

Note: Top-level commands that lead to a reload have to be used
with care in interaction tests.
Solutions given to metas by interaction commands get lost by reloads,
since they are not incorporated into the source file (naturally).  The
situation is now similar to why case splits do no update the state of
the interaction.

Instructions how to revert ...
------------------------------

... the changes introduced in interaction that are introduced by this
patch:

A simple `git revert` won't do since there are code improvements
contained in this patch that should not be reverted.

However, a manual revert is: remove data type

  Agda.Interaction.Base.InteractionMode

and go with the flow.  Any choices that use this bit should collapsed
as if there was only the value

  RegularInteraction
andreasabel added a commit that referenced this issue May 18, 2020
Should the new behavior of top-level interaction commands (reload on
not-in-scope errors) be disruptive to one's workflow, this option brings
back the old behavior (with private declarations not in scope).
@andreasabel
Copy link
Member Author

I have merged the PR to subject it to testing in practice.

@nad : Please use Agda "naturally" (as you always use it) and see if any disruption of your workflows happens in practice. There is an "emergency brake", option --top-level-interaction-no-private that brings back the old behavior, should you get irritated by the new one. Please report situations where the new behavior is disrupting.

jespercockx pushed a commit to jespercockx/agda that referenced this issue May 21, 2020
jespercockx pushed a commit to jespercockx/agda that referenced this issue May 21, 2020
These values helped me trace the missing highlighting introduced by the
load caused by top-level interaction commands to the agda mode, which
supplied "None" as highlighting level to these commands.
jespercockx pushed a commit to jespercockx/agda that referenced this issue May 21, 2020
This commit addresses the problem that interaction commands that run
in the top-level, like C-c C-n, do not have access to private decls of
the main module.  These are deleted from the scope before
serialization, for reasons of performance (see agda#1806).

The solution implemented here is that interaction gets two modes:

1. RegularInteraction: Initial mode after C-c C-l.  The internal
representation of the top module, hence its insideScope, is loaded
from the interface file and thus has no knowledge of private
declarations.

2. TopLevelInteraction: Top-level interaction commands switch to this
mode, leading to a reload of the current top module in a way that
private declarations are preserved in the insideScope.  The module is
not serialized.

When a new file is loaded, we switch back to RegularInteraction.

The emacs mode had to be changed to switch on highlighting for
top-level commands, which was off because no highlighting would
happen.  Now, highlighting can happen due to a reload.

UPDATE: only switch to TopLevelInteraction when NotInScope error.

Catch not-in-scope errors in top-level interaction commands such as

  C-c C-d  infer
  C-c C-n  evaluate
  C-c C-o  module contents
  C-c C-w  why in scope

and try to handle by reloading file in TopLevelInteraction mode that
preserves the private declarations.

Caveat: commands that do not throw scope errors such as

  C-c C-z  search about

will by default (RegularInteraction) not include the private
declarations, only if some other top-level command switches to
TopLevelInteraction, then the private declarations will also be listed.

Note: Top-level commands that lead to a reload have to be used
with care in interaction tests.
Solutions given to metas by interaction commands get lost by reloads,
since they are not incorporated into the source file (naturally).  The
situation is now similar to why case splits do no update the state of
the interaction.

Instructions how to revert ...
------------------------------

... the changes introduced in interaction that are introduced by this
patch:

A simple `git revert` won't do since there are code improvements
contained in this patch that should not be reverted.

However, a manual revert is: remove data type

  Agda.Interaction.Base.InteractionMode

and go with the flow.  Any choices that use this bit should collapsed
as if there was only the value

  RegularInteraction
jespercockx pushed a commit to jespercockx/agda that referenced this issue May 21, 2020
Should the new behavior of top-level interaction commands (reload on
not-in-scope errors) be disruptive to one's workflow, this option brings
back the old behavior (with private declarations not in scope).
andreasabel added a commit that referenced this issue May 19, 2021
This commit unfixes #4647 by removing the
`InteractionMode` (`TopLevelInteraction`/`RegularInteraction`) and the
option to switch off the logic (which didn't fully work).

For #4647 there is a simple workaround: add a hole to the file so that
the interface is not serialize and thus the private definitions are
not removed.
andreasabel added a commit that referenced this issue May 19, 2021
This commit unfixes #4647 by removing the
`InteractionMode` (`TopLevelInteraction`/`RegularInteraction`) and the
option to switch off the logic (which didn't fully work).

For #4647 there is a simple workaround: add a hole to the file so that
the interface is not serialize and thus the private definitions are
not removed.
andreasabel added a commit that referenced this issue May 19, 2021
This commit unfixes #4647 by removing the
`InteractionMode` (`TopLevelInteraction`/`RegularInteraction`) and the
option to switch off the logic (which didn't fully work).

For #4647 there is a simple workaround: add a hole to the file so that
the interface is not serialize and thus the private definitions are
not removed.
andreasabel added a commit that referenced this issue May 19, 2021
This commit unfixes #4647 by removing the
`InteractionMode` (`TopLevelInteraction`/`RegularInteraction`) and the
option to switch off the logic (which didn't fully work).

For #4647 there is a simple workaround: add a hole to the file so that
the interface is not serialize and thus the private definitions are
not removed.
andreasabel added a commit that referenced this issue May 19, 2021
This commit unfixes #4647 by removing the
`InteractionMode` (`TopLevelInteraction`/`RegularInteraction`) and the
option to switch off the logic (which didn't fully work).

For #4647 there is a simple workaround: add a hole to the file so that
the interface is not serialize and thus the private definitions are
not removed.
@andreasabel andreasabel linked a pull request May 19, 2021 that will close this issue
@andreasabel andreasabel added the status: wontfix No action will be taken (not in changelog) label May 19, 2021
@andreasabel
Copy link
Member Author

I undid the fix. A workaround (in many cases) is to leave a hole in the file one wants to issue top-level interaction commands.

@andreasabel andreasabel added the dead-code Concerning the dead-code removal optimization label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dead-code Concerning the dead-code removal optimization modules Issues relating to the module system regression in 2.5.1 Regression that first appeared in Agda 2.5.1 scope Issues relating to scope checking status: wontfix No action will be taken (not in changelog) type: bug Issues and pull requests about actual bugs ux: interaction Issues to do with interactive development (holes, case splitting, etc)
Projects
None yet
8 participants