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

11331 add foreach #1420

Closed
wants to merge 7 commits into from
Closed

Conversation

hlindberg
Copy link
Contributor

This contains an implementation of method calls.
The grammar allows method calls to specify an optional lambda expression.
Evaluation of a method call is based on the existing function mechanism.
An implementation of the foreach function is included.
Tests for the lexer/grammar/parser changes as well as for the foreach function are included.

See details in the issue.

Additional functions collect, select, reject, reduce are also implemented and are available on a branch for testing (pending if they should go into puppet or standardlib.

@hlindberg
Copy link
Contributor Author

Apparently the spec run for Ruby 1.8.7 failed - need to debug that. Still possible to review the rest as it works for Ruby 1.9.3.

There is also a round of yardoc required.

# (This is made complicated due to the fact that the implementation of scope is overloaded with
# functionality and an inner ephemeral scope must be used (as opposed to just pushing a local scope
# on a scope "stack").
begin
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an abuse of the the ephemeral scope. The ephemeral scope is just supposed to be used for the numeric variables populated by regexes.

I was wondering how you were planning on handling scope inside the block for this. I think we need to come up with a better way. I think that with this implementation we have at least 2 problems:

  1. ephemeral scope is being manhandled for this case
  2. regular variables set inside the block (if that is possible, and if it isn't then it probably should be) will be set on the outer scope

The second one would also create a problem in which a foreach that includes a variable assignment will error on the second call to the lambda because the variable has already been set in the scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, variables will not be set in the outer scope when being in a local ephemeral scope. Works just fine - I changed that. The alternative was to refactor the entire messy scope implementation into a more traditional stack/chain of scopes. However, since scopes are looked up by name/context, the current scope is more like a root, and the ephemeral scope is a traditional stack of scopes.

So, your concerns are addressed, and there are tests.
The rule regarding variables is that they can be created, they shadow outer, but they are not visible to other scopes.

This adds support for "foreach" for arrays and hashes.
The implementation adds support for MethodCall and Lambda expressions to
the lexer and parser. 

A method call is simply a variant of function call that supports an
optional parameterized block (i.e. a lambda). The foreach method is
simply implemented as a function that accepts a "receiver" and a lambda.
The foreach function operates on arrays and hashes.

The implementation does not alter any existing syntax and does not
introduce any additional keywords or constraints on any lexer literals.

Since racc only has one token lookahead it was necessary to add such
lookahead in the lexer for LBRACE, which now produces a LAMBDA token if
 LBRACE is followed by PIPE. DOT token was reintroduced, and a PIPE
token added.

To allow a lambda to return the result of a single expression, the
syntax: {|...| = expr } may be used. Unfortunately, a major rewrite of
the grammar is required to allow this without a syntactic marker. Also
unfortunate is the fact that all functions invokable as statements does
not produce an rvalue and an rvalue function may not be used as a
statement. (To change that would also be a major rewrite). Hence, a
syntactic marker (in a lambda only), that allows a single expression to
be turned into a "rvalue" statment seems to be the best approach). If
the grammar is improved in the future, the = could be made optional.
This relaxes the constraint that a = expr must be the only statement
in a lambda (or be a sequence of statements). Is is now instead possible
to place this statement last after a sequence of statements.

This commit also fixes an issue where the last value of the lambda and
foreach function did not produce a value.
There was an inspect in a rescue that was only there for debugging.
Now removed.
Under some conditions, array zip in ruby 1.8.7 produces a single value
instead of an array. The result was that the integer value of the first
char was produced as the result instead of the expected string.

This fix checks if the name value is just a string name, or an array
with [name, value].
This makes it possible to invoke foreach as:
foreach($a) {|$x| ... }

The only caveat is that a parentheses-less call must have at least one
argument. This is not allowed:
foo {|$x| }

If someone implements a function that takes only a lambda that call
would need to be written as:
foo() {|$x| }

This since it is not allowed to pass a lambda as an argument (since this
would require implementing real closures).
@hlindberg
Copy link
Contributor Author

Closing the PR since we are discussing whether this style of foreach or something else should be implemented. Also, this PR is now behind and needs to be rebased.

@hlindberg hlindberg closed this Feb 11, 2013
@hlindberg hlindberg deleted the 11331-add-foreach branch September 16, 2017 08:33
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.

None yet

2 participants