Bug #5068

Issue with "duplicated when clause is ignored"

Added by Stefano Mioli about 4 years ago. Updated over 3 years ago.

[ruby-core:38343]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada
ruby -v:1.9.2-p{180,290} Backport:

Description

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'.

Associated revisions

Revision 35459
Added by Koichi Sasada over 3 years ago

  • 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 1.9 - Bug #5068]
  • test/ruby/test_syntax.rb: add a test for above.

Revision 35459
Added by Koichi Sasada over 3 years ago

  • 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 1.9 - Bug #5068]
  • test/ruby/test_syntax.rb: add a test for above.

History

#1 Updated by Yusuke Endoh about 4 years ago

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 mame@tsg.ne.jp

#2 Updated by Yusuke Endoh about 4 years ago

  • Status changed from Open to Assigned
  • Assignee set to 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 mame@tsg.ne.jp

#3 Updated by Stefano Mioli about 4 years ago

Thanks.

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

#4 Updated by Yusuke Endoh over 3 years ago

  • Assignee changed from Yusuke Endoh to Koichi Sasada

Hello, ko1

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

Yusuke Endoh mame@tsg.ne.jp

#5 Updated by Koichi Sasada over 3 years ago

(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. May I commit it with modification?

--
// SASADA Koichi at atdot dot net

#6 Updated by Yusuke Endoh over 3 years ago

(why did you commit it?).

I just forgot :-)

May I commit it with modification?

Please!

--
Yusuke Endoh mame@tsg.ne.jp

#7 Updated by Koichi Sasada over 3 years ago

  • 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 1.9 - Bug #5068]
  • test/ruby/test_syntax.rb: add a test for above.

Also available in: Atom PDF