Skip to content

Commit

Permalink
Fix #1969: Make StaticScope and IRScope agree on var slot assignment
Browse files Browse the repository at this point in the history 
* IRBuilder was ignore duplicate "_" args in blocks whereas the parser
  would allocate slots in StaticScope for all of them. This effectively
  led the runtime to allocate DynamicScope with 1 fewer slot than what
  was required because a variable's slot id came from StaticScope
  but # variables for a DynamicScope came from IRScope.

  We are yet to find a way to clean up this static/lexical scope
  duplication/split from the old runtime.

* Added a new spec/regression test.
@subbuss
subbuss committed Sep 16, 2014
1 parent 158e0dd commit 4303b57
Showing 3 changed files with 23 additions and 1 deletion.
3 changes: 2 additions & 1 deletion core/src/main/java/org/jruby/ir/IRBuilder.java
View file
Original file line number Diff line number Diff line change
@@ -1692,7 +1692,8 @@ public void receiveRequiredArg(Node node, IRScope s, int argIndex, boolean post,
String argName = a.getName();
if (s instanceof IRMethod) ((IRMethod)s).addArgDesc("req", argName);
// SSS FIXME: _$0 feels fragile?
// Ignore duplicate "_" args in blocks.
// Ignore duplicate "_" args in blocks
// (duplicate _ args are named "_$0")
if (!argName.equals("_$0")) {
addArgReceiveInstr(s, s.getNewLocalVariable(argName, 0), argIndex, post, numPreReqd, numPostRead);
}
7 changes: 7 additions & 0 deletions core/src/main/java/org/jruby/parser/StaticScope.java
View file
Original file line number Diff line number Diff line change
@@ -137,6 +137,13 @@ private static boolean namesAreInterned(String[] names) {
* @return index+depth merged location of scope
*/
public int addVariableThisScope(String name) {
// Ignore duplicate "_" args in blocks
// (duplicate _ args are named "_$0")
// Dont allocate slots for them.
if (name.equals("_$0")) {
return -1;
}

int slot = exists(name);

if (slot >= 0) return slot;
14 changes: 14 additions & 0 deletions spec/regression/GH-1969_duplicate_underscore_block_arg.rb
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# https://github.com/jruby/jruby/issues/1969

def yielder; yield; end

def foo; yield 1,2,3,4,5,6,7; end

describe 'Blocks with duplicate _ args' do
it 'should not crash' do
foo do |x,_,_,_,a,b,c|
bar {}
[x,a,b,c]
end.should_be [1,5,6,7]
end
end

9 comments on commit 4303b57

@nirvdrum
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to assert _ is assigned the correct value in the spec as well.

@enebo
Copy link
Member

@enebo enebo commented on 4303b57 Sep 17, 2014

Choose a reason for hiding this comment

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

@nirvdrum I was going to say relying on value of _ is weird, but I noticed we have a weird bug with '_':

def foo(_, _=1); p _; end; foo(1,2)

'' binds to first actual value which is 1 but making an opt-arg out of second '' seems to bind the first value to 2?!?!?!

@nirvdrum
Copy link
Contributor

Choose a reason for hiding this comment

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

@enebo My experience has suggested that if MRI has an undefined, but predictable behavior, someone out there has exploited it :-(

@enebo
Copy link
Member

@enebo enebo commented on 4303b57 Sep 17, 2014

Choose a reason for hiding this comment

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

@nirvdrum

def foo(_, _=1, *_); p _; end; foo(1,2) #=> []
def foo(_, _); p _; end; foo(1,2) #=> 1

@nirvdrum
Copy link
Contributor

Choose a reason for hiding this comment

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

@enebo So, you're saying we need more specs? Or that you, too, now hate everything? :-P

@enebo
Copy link
Member

@enebo enebo commented on 4303b57 Sep 17, 2014

Choose a reason for hiding this comment

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

haha...yeah we should probably spec this out as well or report it as a bug. *_ overriding regular _ is pretty unexpected. Also first assignment wins is also weird. I would think last would win? I agree though someone no doubt uses _ for some nefarious purpose.

@nirvdrum
Copy link
Contributor

Choose a reason for hiding this comment

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

Using _ is fairly common in Scala and some other languages. My guess is a cross-language developer would do it without even thinking about it.

@subbuss
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "def foo(_, =1, *); p ; end; foo(1,2) #=> []" is a MRI bug and should be fixed there. rubyspecs already has a spec for the use of "" in blocks.

@subbuss
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at the AST for each of these:
def foo(_, _); p ; end; foo(1,2)
def foo(, _=5); p _; end; foo(1,2)

def yielder; yield 1,2; end

yielder { |,| p _ }
yielder { |,=5| p _ }

I was relying on the presence of _$0 to detect duplicates, but in the case of the opt-arg scenario, both args are named _. This is actually a bigger issue than bad results .. it can potentially cause other crashers of the kind that I fixed in this patch.

Please sign in to comment.