Bug #5068: Issue with "duplicated when clause is ignored"

• Status: Closed
• Assignee: ko1 (Koichi Sasada)
• Target version: 2.0.0
• ruby -v: 1.9.2-p{180,290}

[sm (Stefano Mioli)]

I'm filing this ticket as suggested by Ryan Davis here:

http://www.ruby-forum.com/topic/2154866#1011303

Let's consider this snippet:

x = 0
case x
when 1
when 0
when 1
end

Executing a file containing this code with ruby -w, the interpreter will print out this warning:

warning: duplicated when clause is ignored

Let's consider this one instead:

x = 0
case x
when 1
when 0 + 1
when 1
end

This will not print out the same warning, even though we still have a duplicated 'when'.

[mame (Yusuke Endoh)]

Hello,

It would be good to warn the code, but strictly, the warning
may be false positive:

$ cat t.rb
class Fixnum
def +(x)
def ===(x)
true
end
end
end

x = 0
case x
when 1 then p :foo
when 0 + 1 then p :bar
when 1 then p :baz
end

$ ./ruby t.rb
:baz

Ruby is really free :-P

--
Yusuke Endoh [e-mail]

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to mame (Yusuke Endoh)
• Target version set to 2.0.0

Here is a patch. It became bigger than expected...
I'll commit it unless there is objection.

diff --git a/compile.c b/compile.c
index 53149ca..c1012f7 100644
--- a/compile.c
+++ b/compile.c
@@ -1275,6 +1275,18 @@ static const struct st_hash_type cdhash_type = {
cdhash_hash,
};

+static int
+translate_cdhash(st_data_t key, st_data_t val, st_data_t arg)
+{

• VALUE lit = (VALUE)key;
• LABEL *lobj = (LABEL *)(val & ~1);
• VALUE *p = (VALUE *)arg;
• rb_hash_aset(p[0], key, INT2FIX(lobj->position - FIX2INT(p[1])));
• return ST_CONTINUE;
  +}

/**
ruby insn object list -> raw instruction sequence
*/
@@ -1402,30 +1414,11 @@ iseq_set_sequence(rb_iseq_t *iseq, LINK_ANCHOR *anchor)
* [obj, label, ...]
*/
int i;

•   	    VALUE lits = operands[j];
  

•   	    VALUE map = rb_hash_new();
  

•   	    RHASH_TBL(map)->type = &cdhash_type;
  

•   	    for (i=0; i < RARRAY_LEN(lits); i+=2) {
  

•   		VALUE obj = rb_ary_entry(lits, i);
  

•   		VALUE lv  = rb_ary_entry(lits, i+1);
  

•   		lobj = (LABEL *)(lv & ~1);
  

•   		if (!lobj->set) {
  

•   		    rb_compile_error(RSTRING_PTR(iseq->filename), iobj->line_no,
  

•   				     "unknown label");
  

•   		}
  

•   		if (!st_lookup(rb_hash_tbl(map), obj, 0)) {
  

•   		    rb_hash_aset(map, obj, INT2FIX(lobj->position - (pos+len)));
  

•   		}
  

•   		else {
  

•   		    rb_compile_warning(RSTRING_PTR(iseq->filename), iobj->line_no,
  

•   				       "duplicated when clause is ignored");
  

•   		}
  

•   	    }
  

•   	    hide_obj(map);
  

•   	    generated_iseq[pos + 1 + j] = map;
  

•   	    iseq_add_mark_object(iseq, map);
  

•   	    VALUE lits = operands[j], arg[2] = { lits, INT2FIX(pos+len) };
  

•   	    st_foreach(RHASH_TBL(lits), translate_cdhash, (st_data_t)arg);
  

•   	    hide_obj(lits);
  

•   	    generated_iseq[pos + 1 + j] = lits;
  

•   	    iseq_add_mark_object(iseq, lits);
    	    break;
    	}
          case TS_LINDEX:
  

@@ -2345,7 +2338,7 @@ case_when_optimizable_literal(NODE * node)
}

static VALUE
-when_vals(rb_iseq_t *iseq, LINK_ANCHOR *cond_seq, NODE *vals, LABEL *l1, VALUE special_literals)
+when_vals(rb_iseq_t *iseq, LINK_ANCHOR *cond_seq, NODE *vals, LABEL *l1, VALUE special_literal_opt, VALUE special_literals)
{
while (vals) {
VALUE lit;
@@ -2353,13 +2346,17 @@ when_vals(rb_iseq_t *iseq, LINK_ANCHOR *cond_seq, NODE *vals, LABEL *l1, VALUE s

val = vals->nd_head;

• if (special_literals &&

•   (lit = case_when_optimizable_literal(val)) != Qfalse) {
  

•   rb_ary_push(special_literals, lit);
  

•   rb_ary_push(special_literals, (VALUE)(l1) | 1);
  

• if ((lit = case_when_optimizable_literal(val)) != Qfalse) {

•   if (!st_lookup(rb_hash_tbl(special_literals), lit, 0)) {
  

•   rb_hash_aset(special_literals, lit, (VALUE)(l1) | 1);
  

•   }
  

•   else {
  

•   rb_compile_warning(RSTRING_PTR(iseq->filename), nd_line(val),
  

•   		   "duplicated when clause is ignored");
  

•   }
  

  }
  else {

•   special_literals = Qfalse;
  

•   special_literal_opt = Qfalse;
  

  }

  if (nd_type(val) == NODE_STR) {
  @@ -2375,7 +2372,7 @@ when_vals(rb_iseq_t *iseq, LINK_ANCHOR *cond_seq, NODE *vals, LABEL *l1, VALUE s
  ADD_INSNL(cond_seq, nd_line(val), branchif, l1);
  vals = vals->nd_next;
  }

• return special_literals;

• return special_literal_opt;
  }

static int
@@ -3049,13 +3046,14 @@ iseq_compile_each(rb_iseq_t *iseq, LINK_ANCHOR *ret, NODE * node, int poped)
break;
}
case NODE_CASE:{

• VALUE special_literals;
  NODE *vals;
  NODE *tempnode = node;
  LABEL *endlabel, *elselabel;
  DECL_ANCHOR(head);
  DECL_ANCHOR(body_seq);
  DECL_ANCHOR(cond_seq);

• VALUE special_literals = rb_ary_tmp_new(1);

• VALUE special_literal_opt = Qtrue;

  INIT_ANCHOR(head);
  INIT_ANCHOR(body_seq);
  @@ -3068,6 +3066,8 @@ iseq_compile_each(rb_iseq_t *iseq, LINK_ANCHOR *ret, NODE * node, int poped)

  node = node->nd_body;
  type = nd_type(node);

• special_literals = rb_hash_new();

• RHASH_TBL(special_literals)->type = &cdhash_type;

  if (type != NODE_WHEN) {
  COMPILE_ERROR((ERROR_ARGS "NODE_CASE: unexpected node. must be NODE_WHEN, but %s", ruby_node_name(type)));
  @@ -3091,12 +3091,12 @@ iseq_compile_each(rb_iseq_t *iseq, LINK_ANCHOR *ret, NODE * node, int poped)
  if (vals) {
  switch (nd_type(vals)) {
  case NODE_ARRAY:

•       special_literals = when_vals(iseq, cond_seq, vals, l1, special_literals);
  

•       special_literal_opt = when_vals(iseq, cond_seq, vals, l1, special_literal_opt, special_literals);
        break;
      case NODE_SPLAT:
      case NODE_ARGSCAT:
      case NODE_ARGSPUSH:
  

•       special_literals = 0;
  

•       special_literal_opt = 0;
        COMPILE(cond_seq, "when/cond splat", vals);
        ADD_INSN1(cond_seq, nd_line(vals), checkincludearray, Qtrue);
        ADD_INSNL(cond_seq, nd_line(vals), branchif, l1);
  

@@ -3133,7 +3133,7 @@ iseq_compile_each(rb_iseq_t *iseq, LINK_ANCHOR *ret, NODE * node, int poped)
ADD_INSNL(cond_seq, nd_line(tempnode), jump, endlabel);
}

• if (special_literals) {

• if (special_literal_opt) {
  ADD_INSN(ret, nd_line(tempnode), dup);
  ADD_INSN2(ret, nd_line(tempnode), opt_case_dispatch,
  special_literals, elselabel);
  @@ -5381,16 +5381,17 @@ iseq_build_from_ary_body(rb_iseq_t *iseq, LINK_ANCHOR *anchor,
  case TS_CDHASH:
  {
  int i;

•   	    VALUE map = rb_hash_new();
    	    op = rb_convert_type(op, T_ARRAY, "Array", "to_ary");
    	    op = rb_ary_dup(op);
    	    for (i=0; i<RARRAY_LEN(op); i+=2) {
    		VALUE sym = rb_ary_entry(op, i+1);
    		LABEL *label =
    		  register_label(iseq, labels_table, sym);
  

•   		rb_ary_store(op, i+1, (VALUE)label | 1);
  

•   		rb_hash_aset(map, rb_ary_entry(op, i), (VALUE)label | 1);
    	    }
  

•   	    argv[j] = op;
  

•   	    iseq_add_mark_object_compile_time(iseq, op);
  

•   	    argv[j] = map;
  

•   	    iseq_add_mark_object_compile_time(iseq, map);
    	}
    	break;
          default:
  

--
Yusuke Endoh [e-mail]

[sm (Stefano Mioli)]

Thanks.

Well, I guess it boils down to deciding which between tolerating false positives or false negatives.

[mame (Yusuke Endoh)]

• Assignee changed from mame (Yusuke Endoh) to ko1 (Koichi Sasada)

Hello, ko1

You changed case duplicate check recently.
What do you think about this issue?

--
Yusuke Endoh [e-mail]

[ko1 (Koichi Sasada)]

(2012/04/20 5:18), mame (Yusuke Endoh) wrote:

You changed case duplicate check recently.
What do you think about this issue?

Thank you for notification. I agree with the patch of [ruby-core:38351]
(why did you commit it?). May I commit it with modification?

--
// SASADA Koichi at atdot dot net

[mame (Yusuke Endoh)]

(why did you commit it?).

I just forgot :-)

May I commit it with modification?

Please!

--
Yusuke Endoh [e-mail]

[ko1 (Koichi Sasada)]

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r35459.
Stefano, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• compile.c: fix to output warning when the same literals
  are available as a condition of same case clause.
  And remove infomation ('#n') because we can find duplicated
  condition with explicit line numbers.
  [ruby-core:38343] [Ruby 1.9 - Bug #5068]
• test/ruby/test_syntax.rb: add a test for above.