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

Misc cleanups and improvements #32

Merged
merged 13 commits into from
Apr 22, 2024
Merged

Misc cleanups and improvements #32

merged 13 commits into from
Apr 22, 2024

Conversation

ibbem
Copy link
Collaborator

@ibbem ibbem commented Apr 17, 2024

These are mostly just some simple cleanups left on my TODO list.

The exception is 47321e3 which updates Agda and the standard library. I think that's a good idea because, first of all, this will hopefully get rid of some bugs we encountered. We might also get some other improvements (e.g. a performance improvement on NixOS). And lastly, this is probably better for the first impression: When people look at a library and it already uses some outdated dependencies, it leaves the impression that the library is already unmaintained.
The most incompatibilities I encountered where:

  • Enabling --large-indices because we make use of it in some places (it was implicitly enabled by default before).
  • Changing _≡˘⟨_⟩_ to the new syntax _≡⟨_⟨_ (the standard library changed this for a nice alignment and symmetry)

Related to 8fa5f9f: Should we rename EPVL to something else? I think this name stands for the old paper title "Expressive Power of Variability Languages" right?

Should the README include an example for how to use this library as a dependency in Nix (ba2a8f0)?

ibbem added 2 commits April 17, 2024 23:55
This will allow cloning of this repository (once it's public) without
having to authenticate to GitHub.
@ibbem ibbem requested a review from pmbittner April 17, 2024 22:20
Copy link
Member

@pmbittner pmbittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ibbem,

thank you for this pull request. Your changes are highly appreciated. :) I left some comments and questions below.

src/Data/IndexedSet.lagda.md Show resolved Hide resolved
default.nix Show resolved Hide resolved
src/Lang/OC.lagda.md Outdated Show resolved Hide resolved
@pmbittner
Copy link
Member

Also, I am unsure about the large-indices flag. Where exactly is it needed? I searched for it in the Agda documentation but its description was quite abstract so I did not really understand why we need it.

@pmbittner
Copy link
Member

Should we rename EPVL to something else?

Yes we should do. I am currently still thinking about a proper name. :(

@pmbittner
Copy link
Member

Should the README include an example for how to use this library as a dependency in Nix (ba2a8f0)?

That would be great! We probably need such an example / setup instructions for a potential artifact submission later on.

ibbem added 4 commits April 19, 2024 10:21
All other `VariabilityLanguage`s use a single `L` prefix and it was
already renamed in the reexport of `Lang.All`.
The interface of `cappedFin` using an implicit parameter which also
doesn't represent the predecessor of the maximum value is very nice to
use. Most uses in the code are already using `cappedFin`.

`minFinFromLimit` also implements the same functionality but is
currently unused and it seems like proofs about it are harder than for
`cappedFin`.
@ibbem ibbem force-pushed the misc-improvements branch from fa495e3 to 2382b67 Compare April 19, 2024 08:22
@ibbem
Copy link
Collaborator Author

ibbem commented Apr 19, 2024

The option --large-indicies is needed for most of our languages because of the 𝕍 argument. A minimal example is

Index : Set₁
Index = Set  Set

data Example : Index  Set where
  value :  i  Example i

Here, Example is rejected because Index : Set₁ whereas Example only constructs a type in Set. This was previously allowed but an inconsistency was discovered and the Agda team decided to disable --large-indicies by default (which was also introduced because of that inconsistency). See agda/agda#6661 for details.

I just noticed that the option which conflicts with --large-indicies is actually still enabled 😅. Unfortunately, we cannot enable --safe to check for such inconsistencies because we use --sized-types.

We can either avoid --large-indicies by refactoring (I have no idea yet how complicated this will get) or add --no-forced-argument-recursion (the option which avoids the inconsistency) where we use --large-indicies. Maybe it's also a good idea to move most options to the .agda-lib file which applies the options to the whole project.

@pmbittner
Copy link
Member

Ok interesting. Maybe that is a reason to avoid 𝕍 then and to specialize it to Rose for most definitions. 🤔 Maybe, we can keep 𝕍 in the framework, where most types are somewhat meta (Set_1) but always specialize it for languages and proofs. This is actually how we do it in the paper. For example, CCC semantics would then always produce a Rose tree.

The idea to add the options to the library file sounds good to me.

Someone should build an explicit feature model for Agda. :D

ibbem added 7 commits April 19, 2024 17:20
In some places the new option `--large-indices` is required because an
inconsistency with `--forced-argument-recursion` (which is enabled by
default) was discovered.
Untracked files shouldn't influence the build. Hence, it's a good idea
to ensure that they can't.
This also installs all Agda and Agda interface files instead of, like
before, only keeping the executable.
This is currently an experimental Nix feature. However, it's already
widely used so it makes sense to offer such a standardized interface.
The only exception is `--allow-unsolved-metas` because it's useful to
know where unsolved problems might hide.
@ibbem ibbem force-pushed the misc-improvements branch from 2382b67 to 0acd639 Compare April 19, 2024 17:07
@pmbittner pmbittner self-requested a review April 20, 2024 09:50
Copy link
Member

@pmbittner pmbittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge when you think the PR is ready. :) Some items in our discussion might be better suited for future PRs.

@ibbem ibbem merged commit adc7902 into develop Apr 22, 2024
1 check passed
@ibbem ibbem deleted the misc-improvements branch April 23, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants