Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix for retrieving UnboundMethod parameters #2232

Merged
merged 5 commits into from Mar 27, 2013

Conversation

Projects
None yet
3 participants
Owner

YorickPeterse commented Mar 24, 2013

This pull request patches UnboundMethod#parameters (and the code it depends on) so that it returns the correct argument types and names when dealing with method definitions that contain local variables. Thanks to @dbussink for helping me out with these changes!

YorickPeterse added some commits Mar 24, 2013

@YorickPeterse YorickPeterse Fix for retrieving UnboundMethod parameters.
Before this commit the code tasked with retrieving method parameters would
produce incorrect results. For example, take the following method definition:

    def example(number, &block)
      another_number = 20
    end

    method(:example).parameters

This would lead to the following output:

    [[:req, :number], [:block, :block], [:block, :another_number]]

This was caused due to everything that remains (after processing other argument
types) being assigned as block arguments.

This patch fixes this issue by keeping track of the index of the block argument
and comparing this to the list of local variables similar to how splat
arguments are handled.

Signed-off-by: Yorick Peterse <yorickpeterse@gmail.com>
215c42d
@YorickPeterse YorickPeterse Tests for correctly retrieving block arguments.
Signed-off-by: Yorick Peterse <yorickpeterse@gmail.com>
fdd376e
@YorickPeterse YorickPeterse Cleaned up code for retrieving method parameters.
The use of Array#map would require the use of Array#compact to get rid of
NilClass values in the parameters collection. An easier (and probably a bit
faster way) of doing this is to simply use Array#each and push values into
another Array.

Signed-off-by: Yorick Peterse <yorickpeterse@gmail.com>
c8a2535
Owner

YorickPeterse commented Mar 24, 2013

Sidenote: I have seriously no idea why the tests fail of a sudden. It seems that mspec somehow ignores the tag for the failing test.

@ryoqun ryoqun commented on the diff Mar 25, 2013

kernel/common/method.rb
- p += 1 if @executable.splat
-
- @executable.local_names.each_with_index.map do |name, i|
- if i < m
- [:req, name]
- elsif i < o
- [:opt, name]
- elsif @executable.splat == i
- [:rest, name]
- elsif i < p
- [:req, name]
- else
- [:block, name]
- end
- end
+ return @executable.respond_to?(:parameters) ? @executable.parameters : []
@ryoqun

ryoqun Mar 25, 2013

Member

Thanks for contributing! I think this is suspicious for the sudden CI failure. I'm not absolutely sure, but probably, I think @eXecutable can be some thing other than CompiledCode? How about changing this empty array with some thing like [:foo, :bar] to check the empty array of actual returned value really came from here in CI? If I'm wrong, I'll test your pull request locally.

@YorickPeterse

YorickPeterse Mar 25, 2013

Owner

The weird thing is that even with a clean working directory (before I committed these changes) the particular test would fail. This would indicate that something else is affecting it, but I have no idea why that would be the case all of a sudden.

@dbussink

dbussink Mar 25, 2013

Owner

I think you are still confused on tags work. If you run ./bin/mspec path/to/spec/file.rb directly, it will ignore tags. That means it will run all the specs, even the specs that are known to fail. The CI process does consider the tags, so you don't see the failure then. You won't see the failure either when running 'rake', since that just runs the CI task.

For you now, you can safely ignore this failing spec, since that's for a totally different issue, not related to the problem this pull request tries to fix.

@YorickPeterse

YorickPeterse Mar 25, 2013

Owner

Well that's the thing, the failures do occur both on Travis and when running rake.

@dbussink

dbussink Mar 25, 2013

Owner

Ah ok, then yeah, they do seem related to the changes here then. I don't see those currently for master or here locally, so there must be something different for you then indeed.

@YorickPeterse

YorickPeterse Mar 25, 2013

Owner

The odd thing is that when I reset my changes they still failed, which, according to Travis should not be possible. I'll take a closer look at this this evening.

@ryoqun

ryoqun Mar 25, 2013

Member

As I guessed, @executable can be BlockEnvironment::AsMethod along side CompiledCode. I fixed that at my branch based on your branch with this commit.

Could you rebase --interactive this, while unifying it into your refactoring commit?

@ryoqun ryoqun and 1 other commented on an outdated diff Mar 25, 2013

kernel/common/compiled_code.rb
+ #
+ # @param [Fixnum] position
+ #
+ def block=(position)
+ if position
+ add_metadata(:block_index, position)
+ end
+ end
+
+ ##
+ # @return [Fixnum|NilClass]
+ #
+ def block
+ return get_metadata(:block_index)
+ end
+
@ryoqun

ryoqun Mar 25, 2013

Member

Is there any specific reason to use block instead of block_index?

Usually, block is used to mean BlockEnvironment around here. So, I think just reusing of block_index is just fine to avoid confusion (btw, I'm paused to think that why it is needed for CompiedCode to have BlockEnvironment)

@dbussink

dbussink Mar 25, 2013

Owner

I think it's a good idea to use block_index to prevent confusion on what it means. The term "block" means a different thing in almost every other context, so it can be confusing if people encounter it later.

@ryoqun ryoqun and 3 others commented on an outdated diff Mar 25, 2013

lib/compiler/generator.rb
@@ -320,6 +322,7 @@ def package(klass)
code.post_args = @post_args
code.total_args = @total_args
code.splat = @splat_index
+ code.block = @block_index
code.local_count = @local_count
@ryoqun

ryoqun Mar 25, 2013

Member

oh, this is why you chose the ambiguous name of block? I think splat should be changed to something like splat_index.

Any ideas?, @dbussink

@YorickPeterse

YorickPeterse Mar 25, 2013

Owner

Correct. I didn't want to deviate too much from the existing code, though I agree that both "block" and "splat" aren't accurate names since they contain indexes and not entire argument objects.

@dbussink

dbussink Mar 25, 2013

Owner

For now I think we can ignore splat, but I agree that it might be better to change it to splat_index then, but I think we can do that better in a different pull request / change set / commit.

@ghost

ghost Mar 25, 2013

FWIW Rubinius::MachineCode uses "splat_position". It'd be cool to be consistent across the board.

Owner

dbussink commented Mar 25, 2013

@robgleeson Why not use splat_position as well in Rubinius::CompiledCode instead of having to change everything?

@ghost

ghost commented Mar 25, 2013

@dbussink Ah, I didn't know splat_position was used elsewhere and the preferred choice. Yeah, sounds better to go with splat_position. I'll try to do it, unless someone beats me to it.

ryoqun and others added some commits Mar 25, 2013

@ryoqun @YorickPeterse ryoqun + YorickPeterse Delegate AsMethod#parameters down to CompiledCode cbee9a2
@YorickPeterse YorickPeterse Use "block_index" for block argument indexes.
Instead of using the name "block", which is rather ambiguous, the name
"block_index" should be used.

Signed-off-by: Yorick Peterse <yorickpeterse@gmail.com>
c1b7723

@dbussink dbussink added a commit that referenced this pull request Mar 27, 2013

@dbussink dbussink Merge pull request #2232 from YorickPeterse/master
Fix for retrieving UnboundMethod parameters
8acde7a

@dbussink dbussink merged commit 8acde7a into rubinius:master Mar 27, 2013

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment