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

EEP-49: First proof of concept implementation #5216

Closed
wants to merge 35 commits into from
Closed

Conversation

ferd
Copy link
Contributor

@ferd ferd commented Sep 20, 2021

As part of our spawnfest project, @peerst and I spent the week-end setting up the first proof of concept implementation of EEP-49.

What is this?

See the EEP-49, or for a more terse discussion, the README from the Spawnfest repo

Differences from EEP-49

While implementing we figured out that the choice of else in EEP-49 was more trouble than expected. Not only would it collide with usage of atom else but also with the -else attribute of epp.

So we decided to go with begin ... cond ... end instead. Also trying out the alternative begin ... catch ... end in not merged PR Switch from 'cond' to 'catch' in maybe clauses. Keyword cond was always reserved but not used so far and catch is used for exceptions but that usage wouldn't collide. We also discussed cond ... catch ... end which might or might not collide with the initially intended cond expression.

If the OTP team has a suggestion regarding how to make else work without clashing with pre-processor macros, we're all ear. For the rest of it, we feel that this is a decent representation of the EEP.

Implementation Details

This branch is forked off master branch as recommended by the contributing guides. We have covered the following level of features:

  • Syntax support
  • Implementation handled through the Abstract Syntax Tree (AST)
  • Transformations in the compiler's first core conversion pass to nested case ... of ... end expressions in the AST, which gets translated to standard core Erlang
  • Similar transformations in the shell, so usage of the new construct in the shell is fully supported
  • Pretty-printing support (although indentation is a bit odd maybe still)
  • Introducing new exceptions of the form cond_clause, similar to if_clause and case_clause exceptions
  • Tests
  • Adapt standard Emacs mode to indent and highlight cond correctly in a begin...end

What we haven't really had the time to check:

  • Dialyzer compatibility
  • fancier pretty printing and more advanced syntax tool cases

The approach we took for the implementation is based on tagging and returning values of the begin ... cond ... end construct and transforming them in the first pass of the compiler changing the AST to core format. This test shows the sort of original format vs. the intended transformation:

maybe6() ->
    begin
        X = 2+2,
        {ok, 4} <- {error, 5},
        Z = X*2,
        {error, expected} <- {ok, 3},
        id(Z),
        X+Z
    cond
        {error, _} -> a;
        {ok, _} -> b
    end.

case6() ->
    case
        begin
            X = 2+2,
            case {error, 5} of
                {ok, 4} ->
                    Z = X*2,
                    case {ok, 3} of
                        {error, expected} ->
                            id(Z),
                            {successful_begin, X+Z};
                        Other ->
                            {begin_fail_branch, Other}
                    end;
                Other ->
                    {begin_fail_branch, Other}
            end
        end
    of
        {successful_begin, V} -> V;
        {begin_fail_branch, Err} ->
            case Err of
                {error, _} -> a;
                {ok, _} -> b
            end
    end.

A previous form took the cond ... end segment and carried it into every branch as it was simpler and had (we suppose) more limited overhead, but this would end up carrying a general set of clauses that cover the whole begin ... cond section and apply them to individudal MaybeExprs (Pat <- Expr), and BEAM SSA passes would correctly warn about unreachable clauses. We nevertheless kept this implementation for the shell, since it won't do this type of linting.

What do we expect?

At this point, we'd like to drive the discussion forwards for the EEP, having cleared up most of the initial hurdles, to know how to get this ready for OTP-25.

ferd and others added 30 commits September 17, 2021 20:20
This will make it easier for the judges to understand the project and
for us to add specific instructions to help them.
…erlang#5)

* Make it build, extend type of block

* Reuse cr_clauses since its identical to the definition of else_clauses

* Fix the missing pass through from normal expr to maybe_expr

* Back to 3 tuples for non else clause

* Fix maybe_expr value not being a proper list

* Fix the abstract block type
)

* Do the easy {block,_,_,_} support bits

Adds the basic forms to erl_syntax, parse transforms, etc.

Ignores the trickier bits, namely:
- erl_lint, which we hope to avoid for the parse transform phase
- erl_eval, which may reuse a parse transform's code
- v3_core, which would have a final non-ptrans handling as well.

* actually keep backwards compat on cover

* clean up erl_syntax a bit

* instructions updated

* Erroneously entering this one.

* Damn it more garbage

* fixing syntax error

* fixing syntax error again

* fixing bad variables

* move from else to catch, causes issues with conditional macros
Variables bound before cond are detected bound in the clauses
Maybes are converted into simple matches with no conditionals
This more complex test seems to show the linter going awry on begin ...
cond expressions where a clause can be used but the linter complains it
wouldn't be used!
* Simple trailing maybes without cond work

* Works in all corner cases ... once

* Make it work more than once

* Refactored and simplified

* Refactor, fold all corner cases
The previous format caused issues where shared cond clauses injected
deeper into the tree could cause the core ssa passes to warn about
clauses that would never match.

This patch instead uses a single large shared cond clause that uses the
union of all possible results to match them. It creates a few
intermediary tagged terms, but fully silences the compiler.

Not sure what impact it'd have on Dialyzer warnings.
Fix history display, erl_eval exception
Without these, the various syntax tools that also support block would
have blown up all the time.
Complete syntax support by adding {maybe, _, _, _} exprs
@ferd
Copy link
Contributor Author

ferd commented Sep 20, 2021

CI seems to implode due to the github docker packages service being degraded right now.

@okeuday
Copy link
Contributor

okeuday commented Sep 20, 2021

@ferd It would be nice to avoid the use of catch due to not being connected to error/exit/throw signals. It looks good with cond!

@KennethL
Copy link
Contributor

KennethL commented Oct 4, 2021

The OTP team had a discussion about this last week and we think this is a good suggestion for an language addition but we want the following to be solved:

  • Not good to reuse the keyword begin since it does not really say what it is about and furthermore we don't want export of variables from the block like it is for begin ... end. We rather want this block to work in the same way as try that does not export any variables.
  • The operator <- is a reuse from the generator in list comprehensions where it has a different meaning. We would like another
    operator and discussed =~ or ?=.
    We need to check if some of the suggested operators could come in conflict with a new notation for binary literals <<"abc">>. For that we have discussed ~b"abc" or similar contructs.
  • Keyword combinations discussed were maybe... else ... end. We expect that it will be possible to use the keyword else although it is used as a preprocessor directive.
  • We are working on a new mechanism for how to activate new language features, which we expect will be used to activate this feature (new EEP for that soon). By doing that a new keyword will only introduce potential incompatibilities in modules which activated the new language feature.

I suggest that the discussion continues with the EEP-49 as base. Will come with proposals in an updated version of the EEP soon.

@ferd
Copy link
Contributor Author

ferd commented Oct 4, 2021

I would have really appreciated that feedback in the three years that EEP-49 was open for review, and even vetted with these keyword choices after rounds of reviews, because they drastically change the implementation and we could have gotten it closer to everything on the first try!

  • I figure the maybe keyword is really easy to add
  • We'd ideally use else as well, but need guidance on how to set that up.
  • I don't necessarily understand the issue with the operator, since semantically it is very close to the generator (at least on the left-hand side), though I guess ?= would fit a maybe construct well instead of a begin, but I guess this would also run in a conflict with the preprocessor using ? for macros (For example X ??Y where -define(Y, =f()). now causes ambiguity between returning the string format of Y or expanding to the proper expression). =~ is something I've seen used in other languages to refer to regular expression matches, and would imagine used in mathematical notation for approximate matches, so I'm a bit more annoyed by that one since it feels more counter-intuitive to my background.
  • I decided to think a bit on the non-exporting of scope, because this compiles down to case expressions, not to a try catch. Particularly, there are use cases such as the following that would now be impossible:
maybe
    {ok, Config} <- parse_config(X),
    ok <- validate_config(X),
    MinSize = get_min_size(Config, DefaultMin)
    MaxSize = get_max_size(Config, DefaultMax)
else
    {error, _} -> 
        MinSize = DefaultMin,
        MaxSize = DefaultMax
end

This sort of approach is currently allowed with case ... of ... end expressions. That being said, if the intent is to also eventually drop this from the case expressions then I guess it makes sense to force new constructs to also take the form:

{MinSize, MaxSize} =
maybe
    Config <- parse_config(X),
    ok <- validate_config(X),
    {get_min_size(Config, MinDefault),
     get_max_size(Config, MaxDefault)}
else
    {error, _} -> {MinDefault, MaxDefault}
end

They can be equivalent, and I figure that the bottom approach is safer if there because cases like the following would be hard to get right:

maybe
    {ok, Config} <- parse_config(X),
    ok <- validate_config(X),
    {ok, MinSize} <- get_min_size(Config)
    {ok, MaxSize} <- get_max_size(Config)
else
    {error, _} -> 
        MinSize = DefaultMin,
        MaxSize = DefaultMax
end

because now a failure on MinSize leaves a confusing switch open. So yeah, I guess the try ... catch form of not exporting variables is safer on edge cases and makes sense. I think it wouldn't be too hard to retrofit the current transforms into nested try ... of ... end blocks rather than the case ... of ... end block in the core pass to get the desired semantics. That would be easy to add to the EEP as a rationale.

@peerst and I can try and make most of these changes work. Is the expectation that the OTP team would update EEP-49 ("Will come with proposals in an updated version of the EEP soon") or that I should be updating the EEP as well?

@okeuday
Copy link
Contributor

okeuday commented Oct 4, 2021

Another alternative to <- could be |= to remind the developer of a pipeline concept while being clearly different from /=. Otherwise, ?= seems like a good option though it would be usage of ? outside the preprocessor (using ? could be confusing).

@meox
Copy link
Contributor

meox commented Oct 4, 2021

Opinion from a new Erlang dev: "reusing operator is a plus"!

@essen
Copy link
Contributor

essen commented Oct 4, 2021

{MinSize, MaxSize} =
maybe
    Config <- parse_config(X),
    ok <- validate_config(X),
    {get_min_size(Config, MinDefault),
     get_max_size(Config, MaxDefault)}
else
    {error, _} -> {MinSize, MaxSize}
end

I assume the {MinSize, MaxSize} in the else clause are not to be confused with the {MinSize, MaxSize} that's returned by the maybe .. else .. end block, right?

@ferd
Copy link
Contributor Author

ferd commented Oct 4, 2021

I assume the {MinSize, MaxSize} in the else clause are not to be confused with the {MinSize, MaxSize} that's returned by the maybe .. else .. end block, right?

yeah sorry, I reused the values and the two in the else clause should be MinDefault and MaxDefault, will edit my comment.

@KennethL
Copy link
Contributor

KennethL commented Oct 5, 2021

I would have really appreciated that feedback in the three years that EEP-49 was open for review, and even vetted with these keyword choices after rounds of reviews, because they drastically change the implementation and we could have gotten it closer to everything on the first try!

Sorry for the late feedback. We have liked this proposal for quite some time now. Our view of it has matured and now we have looked at it from a different angle + more detailed. That is why we now have some detailed suggestions about the keywords, the export of variables and the operator. Your PR triggered us to have a new meeting about this and now you have got our feedback. We think/hope it would be possible to include this in OTP 25 together with a new mechanism for activating new language features per module.

Our plan to make this happen is that we create a new version of the EEP where our suggestions are incorporated and then we have a meeting with @ferd and @peerst where we can agree on the next steps.

* I figure the maybe keyword is really easy to add

Yes it is easy to add maybeand we did not want to reuse begin. If there are better suggestions than maybe we are open for that.

* We'd ideally use `else` as well, but need guidance on how to set that up.

We expect that we will find a solution for using else here. If the current implementation blocks us from using keywords that also are used as preprocessor directives we want to solve that.

* I don't necessarily understand the issue with the operator, since semantically it is very close to the generator (at least on the left-hand side), though I guess `?=` would fit a `maybe` construct well instead of a `begin`, but I guess this would _also_ run in a conflict with the preprocessor using `?` for macros (For example `X ??Y` where `-define(Y, =f()).` now causes ambiguity between returning the string format of `Y` or expanding to the proper expression). `=~` is something I've seen used in other languages to refer to regular expression matches, and would imagine used in mathematical notation for approximate matches, so I'm a bit more annoyed by that one since it feels more counter-intuitive to my background.

* I decided to think a bit on the non-exporting of scope, because this compiles down to case expressions, not to a try catch. Particularly, there are use cases such as the following that would now be impossible:

We think that the export of variables out of a block is a mistake from the beginning and don't want to introduce yet another block with that problem.

@peerst and I can try and make most of these changes work. Is the expectation that the OTP team would update EEP-49 ("Will come with proposals in an updated version of the EEP soon") or that I should be updating the EEP as well?

Yes we plan to update the EEP and to work with the implementation as well. The tricky thing is to agree on the exact syntax and sematics. Then the implementation should be rather straightforward.

@ferd
Copy link
Contributor Author

ferd commented Oct 5, 2021

Thanks for the update. I'll be awaiting your input for the next steps, and will see if I have time to update this PR until then.

@bjorng bjorng self-assigned this Oct 26, 2021
@bjorng
Copy link
Contributor

bjorng commented Nov 2, 2021

I have now implemented compiler support for EEP 49 in a draft pull request. Differences from the EEP and your implementation are described in the pull request description.

@bjorng
Copy link
Contributor

bjorng commented Nov 2, 2021

@ferd Kenneth and I have updated EEP 49 in a pull request to reflect the notes from the OTP Technical Board. Please have a look.

@ferd
Copy link
Contributor Author

ferd commented Nov 2, 2021

Already done! Thanks for the heads up. I'll close this PR since it has been superseded.

@ferd ferd closed this Nov 2, 2021
@bjorng
Copy link
Contributor

bjorng commented Nov 2, 2021

Already done! Thanks for the heads up.

That was quick! Thanks!

I'll close this PR since it has been superseded.

I haven't implemented support for maybe everywhere; I will probably wait for the EEP to be approved before doing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants