This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 228005 - Convert if to switch may change code behavior
Summary: Convert if to switch may change code behavior
Status: NEW
Alias: None
Product: java
Classification: Unclassified
Component: Hints (show other bugs)
Version: 8.1
Hardware: PC Windows 7
: P3 normal (vote)
Assignee: Svata Dedic
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-27 14:44 UTC by Jiri Prox
Modified: 2016-07-19 06:23 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Prox 2013-03-27 14:44:59 UTC
Convert several ifs to switch may change code behavior

Steps to reproduce:
1) have a code:
        String xx = null;
        if("".equals(xx)) {
            System.out.println("aaa");
        } else if("aaaa".equals(xx)) {
            System.out.println("bbb");
        }

2) use hint to convert it to switch
->
        String xx = null;
        switch (xx) {
            case "":
                System.out.println("aaa");
                break;
            case "aaaa":
                System.out.println("bbb");
                break;
        }

the converted code throws NPE



Product Version: NetBeans IDE 7.3 (Build 201302132200)
Java: 1.7.0_17; Java HotSpot(TM) 64-Bit Server VM 23.7-b01
Runtime: Java(TM) SE Runtime Environment 1.7.0_17-b02
System: Windows 7 version 6.1 running on amd64; Cp1250; en_US (nb)
User directory: C:\Users\jprox\AppData\Roaming\NetBeans\7.3
Cache directory: C:\Users\jprox\AppData\Local\NetBeans\Cache\7.3
Comment 1 Svata Dedic 2013-11-07 09:59:27 UTC
The JLS7 states that it was deliberately decided to implement String switch as expr.compare(const) instead of const.compare(expr). While I really don't second that decision, it's the definition the IDE has to count with.

I'll surround the generated code with a guard `if' and try to use NPE check internals to avoid the extra condition in obvious non-null cases.
Comment 2 Svata Dedic 2013-11-15 10:17:55 UTC
Fixed in http://hg.netbeans.org/jet-main/rev/cab197269570
Comment 3 Quality Engineering 2013-11-24 02:19:58 UTC
Integrated into 'main-silver', will be available in build *201311240002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/cab197269570
User: Svata Dedic <sdedic@netbeans.org>
Log: #228005, #230514: the fix inserts null check if the selector is not known to be non-null. Duplicate labels detected.
Comment 4 skoczko 2016-06-21 13:58:26 UTC
Hi,

There is an issue with this solution: it changes the semantics of the converted code.

E.g:

if "label1".equals(string) {
  doA();
} else if ("label2".equals(string) {
  doB();
} else {
  doC();
}

is converted to:

if (string != null) switch (string) {
  case "label1": doA(); break;
  case "label2": doB(); break;
  default: doC(); 
}

which means doC() is no longer fired for string = null;

There are a couple solutions:

 * generate else block after the switch and fill it with the same stuff as default label. Duplicates code making it hard to maintain though

 * add if (string == null) string = ""; before the switch. Assuming there's no explicit check for "" in any if/elseif
Comment 5 Svata Dedic 2016-07-19 06:22:51 UTC
The first part of the complaint is already fixed as issue #257492. However the generated code suffers from code duplication as pointed out in comment #4.

There's not many options how to convert the code nicely; each approach will satisfy some and annoy other:

* put an assert in front of the switch; the resulting code is compact/clean, alerts the programmer about preconditions - but possibly breaks the semantics

* make a temporary variable, assign in default and null case, and handle both case after the switch. 100% compatible, but visually pollutes the code.

* duplicate code. The precondition is visible at the top, but duplicates default/null handler

* assign a placeholder. not always possible as if may evaluate expression. Possibly make a temporary variable for it. Again more code pollution.

The above approaches may be implemented and selected by an option in hint configuration, or multiple fixes can be provided

I will leave this issue opened as an enhancement for further discussion and further release.