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

Effect aliasing #1141

Merged
merged 38 commits into from
Jun 21, 2022
Merged

Effect aliasing #1141

merged 38 commits into from
Jun 21, 2022

Conversation

Orbion-J
Copy link
Contributor

I added the possibility to write effect aliases, similarly as what already exists for types.

  • there is a new keyword effectname
  • to write an alias for an effect row : effectname MyEffectRow(a, ... ,e::Eff, ...) = { Op1 : type, ... | e } the arguments being type variable of any kind (kinds other than type must be explicit)
  • we can have, as above, an open row (the row variable is a parameter of the effect alias) or a closed one (just do not write the | e
  • To use it in a signature or in another type or effect alias just apply it with the right arguments as for typename things.
  • In arrows, we can use them as row variables: () -MyEffectRow(args)-> () (idem with ~>)
  • However, due to lack of kind inference, row variables and aliases have to be used carefully so that links does not think they are of kind type. We need to write them most of the time between braces { | ... }. For instance, if you have effectname E(a::Eff) = {X : ... | a } and a row variable e::Eff, you will have to write E({ |e}). (Idem for another effect alias instead of the variable). This makes the usage of several nested aliases a bit messy, it would be nice if we could avoid it.
  • We cannot write recursive effect aliases for now. In the branch visitor, I added another transformer that makes possible simple recursion by inlining a mu type in one pass.
  • For now the aliases are replaced by the row they correspond to : we do not keep aliases.
  • In the repl, effect alias definitions are printed but without the braces ! Rows are in general printed without braces and the alias body is a row. => this might need to be enhanced

About implementation, I copied and then merged most of the time what existed for typename.

@dhil dhil self-requested a review May 24, 2022 21:15
@jamescheney
Copy link
Contributor

I don't know the backstory of this, and haven't had a chance to look at this carefully, but instead of adding a keyword and duplicating a lot of code for typename, would it be possible (and wouldn't it be nicer) to augment typename to allow an optional kind, and generalize it to allow any kind of type alias (with Type being the default used if no explicit kind is given)?

e.g. instead of

effectname MyEffectRow(a, ... ,e::Eff, ...) = { Op1 : type, ... | e }

could we write

typename MyAlias(...) :: K = ty

for any kind K, with Eff being a special case, and just generalize the existing code for aliases to allow this?

The description and comments suggest that this may not be straightforward, because of existing idiosyncrasies, but the above design seems cleaner and something we should work toward (see also #44 and comments about row aliases). Perhaps a constructive compromise could be to adopt this PR with a small change: allowing (in addition or only)

typename  MyEffectRow(a, ... ,e::Eff, ...):: Eff =  { Op1 : type, ... | e }

as a special case without (yet) trying to generalize to allow aliases of any kind.

@Orbion-J
Copy link
Contributor Author

@jamescheney I can surely do something that looks like that. At least syntacticly, I can check during parsing which kind do we have and use after the same things I ve already done.
I will still try see how complicated it would be to extend to presence, in order to be able to at least use it for every primary kind.

Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

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

It looks to me like you are not expanding effect aliases in place, but rather following the typename infrastructure. Ultimately we want to do something like that, but as a first step I just wanted to use the expansion-technique to keep the diff and redundancy minimal. Alas, that ship may have sailed.

There are a couple of instances that I am not convinced are correct; I think some more testing and careful thought is necessary, e.g. see my comment about instantiate.ml.

bin/repl.ml Outdated Show resolved Hide resolved
bin/repl.ml Outdated Show resolved Hide resolved
bin/repl.ml Outdated Show resolved Hide resolved
bin/repl.ml Outdated Show resolved Hide resolved
bin/repl.ml Outdated Show resolved Hide resolved
core/desugarDatatypes.mli Outdated Show resolved Hide resolved
core/desugarEffects.ml Show resolved Hide resolved
core/parser.mly Outdated Show resolved Hide resolved
core/parser.mly Outdated Show resolved Hide resolved
core/instantiate.ml Outdated Show resolved Hide resolved
@dhil
Copy link
Member

dhil commented May 27, 2022

I don't know the backstory of this, and haven't had a chance to look at this carefully, but instead of adding a keyword and duplicating a lot of code for typename, would it be possible (and wouldn't it be nicer) to augment typename to allow an optional kind, and generalize it to allow any kind of type alias (with Type being the default used if no explicit kind is given)?

Ultimately, we want to do something like that. But we before we do that we should have a design in mind. The point of this change is to introduce the minimal thing that let us tweak the programming experience / presentation with / of effects and handlers.

In its current state this patch is more invasive than I had originally intended. However, it is probably fine, but we need to be a bit more careful about mixing kind type and row in one alias environment -- I am sure this approach opens up an avenue for a lot of new bugs.

Orbion-J and others added 12 commits May 27, 2022 17:09
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
@Orbion-J
Copy link
Contributor Author

I fixed the simple things.
I tried to add what james was suggesting, I had to do a a bit of a hack in the parser to make it work but it seems to be ok. I added also a few things for presence but this is neither complete nor working.
Anyway, you are right mixing everything could cause some unexpected behavior as :

links> typename A = Int ;
A = Int
links> effectname B = { Op:() -A-> () } ;
***: Error: File "core/types.ml", line 1435, characters 11-17: Assertion failed 

this should for instance cause a kind mismatch. I think checking in instantiate.ml would help but there could be other issues.

core/parser.mly Outdated Show resolved Hide resolved
@dhil
Copy link
Member

dhil commented May 27, 2022

I fixed the simple things. I tried to add what james was suggesting, I had to do a a bit of a hack in the parser to make it work but it seems to be ok. I added also a few things for presence but this is neither complete nor working. Anyway, you are right mixing everything could cause some unexpected behavior as :

links> typename A = Int ;
A = Int
links> effectname B = { Op:() -A-> () } ;
***: Error: File "core/types.ml", line 1435, characters 11-17: Assertion failed 

this should for instance cause a kind mismatch. I think checking in instantiate.ml would help but there could be other issues.

Yes. Do not implement James' suggestion. That's for later when we have worked out a proper design. It is too much to bundle it all up.

@jamescheney
Copy link
Contributor

Do not implement James' suggestion. That's for later when we have worked out a proper design. It is too much to bundle it all up.

Of course, I was just asking if this was a "good enough for now" change or intended as a long-term proper design.

@Orbion-J
Copy link
Contributor Author

Orbion-J commented May 30, 2022

I discussed with Sam today and he suggested to me to add kind information in the alias context. I also added that info to each alias instance, so that we can know easily the kind, without having to compute it. It solves then #1141 (comment). For now I just used the primary kind but it could easily be extended to a complete kind if needed.

Within this setting, we can fail when we encounter a situation like in #1141 (comment) :

links> typename A = Int ;
A = Int
links> effectname E = { X : () -A-> () } ;
<stdin>:1: Kind mismatch: Type argument 0 for type constructor A has kind Type, but an argument of kind Row was expected. 
In:
() -A-> ()

I used the kind mismatch error that already exists. I should probably create a new error because the message is not very appropriate. fixed

Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

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

LGTM modulo the requested changes.

core/desugarEffects.ml Outdated Show resolved Hide resolved
core/desugarEffects.ml Outdated Show resolved Hide resolved
core/desugarEffects.ml Outdated Show resolved Hide resolved
core/desugarEffects.ml Show resolved Hide resolved
core/types.ml Outdated Show resolved Hide resolved
core/types.ml Outdated Show resolved Hide resolved
@dhil dhil self-requested a review June 20, 2022 17:51
Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

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

LGTM. I will merge this tomorrow.

@dhil dhil merged commit 4af44bd into links-lang:master Jun 21, 2022
Orbion-J added a commit to Orbion-J/links that referenced this pull request Jul 8, 2022
I added the possibility to write effect aliases, similarly as what already exists for types.
- there is a new keyword `effectname`
- to write an alias for an effect row : `effectname MyEffectRow(a, ... ,e::Eff, ...) = { Op1 : type, ... | e }` the arguments being type variable of any kind (kinds other than type must be explicit)
- we can have, as above, an open row (the row variable is a parameter of the effect alias) or a closed one (just do not write the `| e`
- To use it in a signature or in another type or effect alias just apply it with the right arguments as for `typename` things.
- In arrows, we can use them as row variables: `() -MyEffectRow(args)-> ()` (idem with `~>`)
- However, due to lack of kind inference, row variables and aliases have to be used carefully so that links does not think they are of kind type. We need to write them most of the time between braces ` { | ... }`. For instance, if you have `effectname E(a::Eff) = {X : ... | a }` and a row variable `e::Eff`, you will have to write `E({ |e})`. (Idem for another effect alias instead of the variable). This makes the usage of several nested aliases a bit messy, it would be nice if we could avoid it.
- We cannot write recursive effect aliases for now. In the branch `visitor`, I added another transformer that makes possible simple recursion by inlining a mu type in one pass.
- For now the aliases are replaced by the row they correspond to : we do not keep aliases.
- In the repl, effect alias definitions are printed but without the braces ! Rows are in general printed without braces and the alias body is a row. => this might need to be enhanced

About implementation, I copied and then merged most of the time what existed for `typename`.

Co-authored-by: Daniel Hillerström <[email protected]>
Orbion-J added a commit to Orbion-J/links that referenced this pull request Jul 8, 2022
desalias inline

alias = effectname and typename | init

merge into 1 construct ok; still 2 contexts

merge type and effect aliases contexts

effectname body desugared into effectname
allow better printing

Revert "desalias inline"

This reverts commit 49a9a61.

Revert "effectname body desugared into effectname"

This reverts commit 5150615.

"effectname" in links-mode.el

cleaning before pr

re-cleaning

effectname -> typename _::Kind

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/desugarDatatypes.mli

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/defaultAliases.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/desugarDatatypes.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/desugarDatatypes.ml

Co-authored-by: Daniel Hillerström <[email protected]>

rename alias_env -> tycon_env as originally

Update core/desugarDatatypes.ml

Co-authored-by: Daniel Hillerström <[email protected]>

idem

fixes & add primary kind in aliases

new error kind mismatch

fix : embeded errors

fixes

fix : underscore in effect app

various fixes

short type in effectname + short fun non desugar

comment

Attempted workaround for links-lang#1136 (links-lang#1138)

always desugar op type with type application

correction tests

default arg in repl

Effect aliasing (links-lang#1141)

I added the possibility to write effect aliases, similarly as what already exists for types.
- there is a new keyword `effectname`
- to write an alias for an effect row : `effectname MyEffectRow(a, ... ,e::Eff, ...) = { Op1 : type, ... | e }` the arguments being type variable of any kind (kinds other than type must be explicit)
- we can have, as above, an open row (the row variable is a parameter of the effect alias) or a closed one (just do not write the `| e`
- To use it in a signature or in another type or effect alias just apply it with the right arguments as for `typename` things.
- In arrows, we can use them as row variables: `() -MyEffectRow(args)-> ()` (idem with `~>`)
- However, due to lack of kind inference, row variables and aliases have to be used carefully so that links does not think they are of kind type. We need to write them most of the time between braces ` { | ... }`. For instance, if you have `effectname E(a::Eff) = {X : ... | a }` and a row variable `e::Eff`, you will have to write `E({ |e})`. (Idem for another effect alias instead of the variable). This makes the usage of several nested aliases a bit messy, it would be nice if we could avoid it.
- We cannot write recursive effect aliases for now. In the branch `visitor`, I added another transformer that makes possible simple recursion by inlining a mu type in one pass.
- For now the aliases are replaced by the row they correspond to : we do not keep aliases.
- In the repl, effect alias definitions are printed but without the braces ! Rows are in general printed without braces and the alias body is a row. => this might need to be enhanced

About implementation, I copied and then merged most of the time what existed for `typename`.

Co-authored-by: Daniel Hillerström <[email protected]>

Fix assert error with relational lenses (links-lang#1143)

The `Lens` type traversal was unimplemented and filled in with an `assert false`. As a result, all RL code fails.

I don't think there is any really sensible default traversal due to the complexity of the Lens types, so I have just filled it in with the identity. This doesn't stop someone implementing a traversal -- they'll just need to write one for `Lens.Type.t` type and plug it in as usual.

Make `custom_js_runtime` a multi option (links-lang#1146)

Added the ability to link multiple custom js runtime files.

Co-authored-by: s1908422 <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>

Allow 'default' as an argument to settings in the REPL (links-lang#1147)

Some settings has the value 'default'. Prior to this patch the value 'default' could not be written in the REPL, because it is token. This patch rectifies this problem by allowing the token 'default' to appear in settings argument position in the REPL.

Co-authored-by: Daniel Hillerström <[email protected]>

Allow record extension in presence of temporal projections (links-lang#1129)

* Allow record extension in presence of temporal projections

This allows record extension to play nicely with temporal projections.
I was forgetting that all arguments to reduce_artifacts are already
eta-expanded, so we can work with record literals.

Fixes links-lang#1124.

* Allow extension to work with (shallow) temporal projections

We don't have the full generality due to links-lang#1130, but this patch should
allow record extension to be used on temporal projections on variables /
record literals.

Removal of unused headless testing (links-lang#1152)

The headless testing has been unused for about a year, because it is unmaintained. Even though it is not used, it generates a ton of security warnings here on GitHub. I am not interested in dealing with those, therefore this patch removes the headless testing directory from the source tree.

Attempted workaround for links-lang#1136 (links-lang#1138)

always desugar op type with type application

correction tests

default arg in repl

Option -> Maybe fix (links-lang#1142)

The `Option -> Maybe` refactoring patch links-lang#1131 missed one instance: the
`max` function. This patch changes the type signature of `max` to use
`Maybe` rather than `Option`.

Effect aliasing (links-lang#1141)

I added the possibility to write effect aliases, similarly as what already exists for types.
- there is a new keyword `effectname`
- to write an alias for an effect row : `effectname MyEffectRow(a, ... ,e::Eff, ...) = { Op1 : type, ... | e }` the arguments being type variable of any kind (kinds other than type must be explicit)
- we can have, as above, an open row (the row variable is a parameter of the effect alias) or a closed one (just do not write the `| e`
- To use it in a signature or in another type or effect alias just apply it with the right arguments as for `typename` things.
- In arrows, we can use them as row variables: `() -MyEffectRow(args)-> ()` (idem with `~>`)
- However, due to lack of kind inference, row variables and aliases have to be used carefully so that links does not think they are of kind type. We need to write them most of the time between braces ` { | ... }`. For instance, if you have `effectname E(a::Eff) = {X : ... | a }` and a row variable `e::Eff`, you will have to write `E({ |e})`. (Idem for another effect alias instead of the variable). This makes the usage of several nested aliases a bit messy, it would be nice if we could avoid it.
- We cannot write recursive effect aliases for now. In the branch `visitor`, I added another transformer that makes possible simple recursion by inlining a mu type in one pass.
- For now the aliases are replaced by the row they correspond to : we do not keep aliases.
- In the repl, effect alias definitions are printed but without the braces ! Rows are in general printed without braces and the alias body is a row. => this might need to be enhanced

About implementation, I copied and then merged most of the time what existed for `typename`.

Co-authored-by: Daniel Hillerström <[email protected]>

Fix assert error with relational lenses (links-lang#1143)

The `Lens` type traversal was unimplemented and filled in with an `assert false`. As a result, all RL code fails.

I don't think there is any really sensible default traversal due to the complexity of the Lens types, so I have just filled it in with the identity. This doesn't stop someone implementing a traversal -- they'll just need to write one for `Lens.Type.t` type and plug it in as usual.

Make `custom_js_runtime` a multi option (links-lang#1146)

Added the ability to link multiple custom js runtime files.

Co-authored-by: s1908422 <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>

wip

Allow 'default' as an argument to settings in the REPL (links-lang#1147)

Some settings has the value 'default'. Prior to this patch the value 'default' could not be written in the REPL, because it is token. This patch rectifies this problem by allowing the token 'default' to appear in settings argument position in the REPL.

Co-authored-by: Daniel Hillerström <[email protected]>

Allow record extension in presence of temporal projections (links-lang#1129)

* Allow record extension in presence of temporal projections

This allows record extension to play nicely with temporal projections.
I was forgetting that all arguments to reduce_artifacts are already
eta-expanded, so we can work with record literals.

Fixes links-lang#1124.

* Allow extension to work with (shallow) temporal projections

We don't have the full generality due to links-lang#1130, but this patch should
allow record extension to be used on temporal projections on variables /
record literals.

Removal of unused headless testing (links-lang#1152)

The headless testing has been unused for about a year, because it is unmaintained. Even though it is not used, it generates a ton of security warnings here on GitHub. I am not interested in dealing with those, therefore this patch removes the headless testing directory from the source tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants