Bugzilla – Full Text Bug Listing
| Summary: | Give switch statements a default case | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Carsten Hammer <carsten.hammer> |
| Component: | UI | Assignee: | Carsten Hammer <carsten.hammer> |
| Status: | CLOSED WONTFIX | QA Contact: | Fabrice Tiercelin <fabrice.tiercelin> |
| Severity: | normal | ||
| Priority: | P3 | CC: | fabrice.tiercelin, kalyan_prasad |
| Version: | 4.19 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=ca21dda307bceba7c277428b01b091122a9ca168 https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174546 https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174551 https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174583 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=9d2f07e10bd487999a0d9bbd574142e209548edc https://bugs.eclipse.org/bugs/show_bug.cgi?id=374605 |
||
| Whiteboard: | |||
|
Description
Carsten Hammer
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174546 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=ca21dda307bceba7c277428b01b091122a9ca168 I donot think this change adds any value to the code. adding default: break; is of no use if error is not reported. The article talks about reporting error for not handled cases in default. Not just simply adding default. So, this patch actually achieves nothing. (In reply to Kalyan Prasad Tatavarthi from comment #2) > I donot think this change adds any value to the code. > > > adding > default: break; > is of no use if error is not reported. > > The article talks about reporting error for not handled cases in default. > Not just simply adding default. > > So, this patch actually achieves nothing. So how to switch on error reporting if the way used in the gerrit is wrong? (In reply to Carsten Hammer from comment #3) > (In reply to Kalyan Prasad Tatavarthi from comment #2) > > I donot think this change adds any value to the code. > > > > > > adding > > default: break; > > is of no use if error is not reported. > > > > The article talks about reporting error for not handled cases in default. > > Not just simply adding default. > > > > So, this patch actually achieves nothing. > > So how to switch on error reporting if the way used in the gerrit is wrong? What is the advantage of having error over Warning which is already being done now? As specified before , a default case with only a break statement does not handle errors. This only reduces warnings in our code. (In reply to Kalyan Prasad Tatavarthi from comment #4) > (In reply to Carsten Hammer from comment #3) > > (In reply to Kalyan Prasad Tatavarthi from comment #2) > > > I donot think this change adds any value to the code. > > > > > > > > > adding > > > default: break; > > > is of no use if error is not reported. > > > > > > The article talks about reporting error for not handled cases in default. > > > Not just simply adding default. > > > > > > So, this patch actually achieves nothing. > > > > So how to switch on error reporting if the way used in the gerrit is wrong? > > What is the advantage of having error over Warning which is already being > done now? > > As specified before , a default case with only a break statement does not > handle errors. This only reduces warnings in our code. The error should be logged in the default case, at the least. (In reply to Kalyan Prasad Tatavarthi from comment #5) > (In reply to Kalyan Prasad Tatavarthi from comment #4) > > (In reply to Carsten Hammer from comment #3) > > > (In reply to Kalyan Prasad Tatavarthi from comment #2) > > > > I donot think this change adds any value to the code. > > > > > > > > > > > > adding > > > > default: break; > > > > is of no use if error is not reported. > > > > > > > > The article talks about reporting error for not handled cases in default. > > > > Not just simply adding default. > > > > > > > > So, this patch actually achieves nothing. > > > > > > So how to switch on error reporting if the way used in the gerrit is wrong? > > > > What is the advantage of having error over Warning which is already being > > done now? > > > > As specified before , a default case with only a break statement does not > > handle errors. This only reduces warnings in our code. > > The error should be logged in the default case, at the least. The above also should be analyzed case by case. The intention is to make gerrit fail the build when someone creates a gerrit where you have a default case (you do not have always a default case!) that is not considered in the code. This can happen in a switch over int or switch over string. For a switch over enum you usually don‘t need that. So at time of the review it should get noticed. (In reply to Carsten Hammer from comment #7) > The intention is to make gerrit fail the build when someone creates a gerrit > where you have a default case (you do not have always a default case!) that > is not considered in the code. > This can happen in a switch over int or switch over string. For a switch > over enum you usually don‘t need that. So at time of the review it should > get noticed. As most of the switch cases are based on ASTNodes and all of the types of ASTNodes are not relevant for each case, They should be handled case by case. I do not think having a default case is mandatory in this scenario. I understand the thought behind this process, but in my opinion this may not be of much use in the JDT UI plugin. (In reply to Kalyan Prasad Tatavarthi from comment #8) > (In reply to Carsten Hammer from comment #7) > > The intention is to make gerrit fail the build when someone creates a gerrit > > where you have a default case (you do not have always a default case!) that > > is not considered in the code. > > This can happen in a switch over int or switch over string. For a switch > > over enum you usually don‘t need that. So at time of the review it should > > get noticed. > > As most of the switch cases are based on ASTNodes and all of the types of > ASTNodes are not relevant for each case, They should be handled case by > case. I do not think having a default case is mandatory in this scenario. I > understand the thought behind this process, but in my opinion this may not > be of much use in the JDT UI plugin. The same is the case with JDT Model elements, IProblem etc. New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174583 Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174583 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=9d2f07e10bd487999a0d9bbd574142e209548edc (In reply to Eclipse Genie from comment #11) > Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174583 was > merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=9d2f07e10bd487999a0d9bbd574142e209548edc Change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174546 reverted with above merge until the discussion is finalized on the bug. So what is the plan now? I understand that the current approach to allow missing default cases does not take into account that in most cases already using code that uses strings or ints where enum was possible is not type safe and therefore not optimal. Not to change the settings to enforce adding an error on missing default case means to being OK with not type safe programming and being reluctant to change to enums where possible. Do I understand that right? Just want to understand thinking behind. Then we can close this as "won't fix"... I just found out that the latest jdt.ui is broken regarding complaining about missing default case. Such code:
public class Blubb {
enum dummy {foh,bah }
void bla(dummy i) {
switch(i) {
case foh:
break;
case bah:
break;
}
}
}
makes eclipse quickassist complaining that a default case is missing - that obviously make no sense!
Kalyan, is that the reason you are sceptical? Because you know that enum support in jdt is slightly broken? In that case it does not make sense to have stronger warnings - we have to wait for this to be fixed.
(In reply to Carsten Hammer from comment #14) > I just found out that the latest jdt.ui is broken regarding complaining > about missing default case. Such code: > > public class Blubb { > enum dummy {foh,bah } > > void bla(dummy i) { > switch(i) { > case foh: > break; > case bah: > break; > } > } > } > > makes eclipse quickassist complaining that a default case is missing - that > obviously make no sense! > Kalyan, is that the reason you are sceptical? Because you know that enum > support in jdt is slightly broken? In that case it does not make sense to > have stronger warnings - we have to wait for this to be fixed. I did not know that enum support is broken. Is this a regression? Please open a bug for the same, so that it can be fixed. I was mainly sceptical because, for most of the cases such as ASTNode types and IProblem which are provided by JDT.Core, conversion to enum types may not be a feasible option. For those Enums that are in Jdt.ui which are not APIs, they can be changed to enums as necessary. So the cases need to be considered separately and not as one single problem. (In reply to Kalyan Prasad Tatavarthi from comment #15) > (In reply to Carsten Hammer from comment #14) > > I just found out that the latest jdt.ui is broken regarding complaining > > about missing default case. Such code: > > > > public class Blubb { > > enum dummy {foh,bah } > > > > void bla(dummy i) { > > switch(i) { > > case foh: > > break; > > case bah: > > break; > > } > > } > > } > > > > makes eclipse quickassist complaining that a default case is missing - that > > obviously make no sense! > > Kalyan, is that the reason you are sceptical? Because you know that enum > > support in jdt is slightly broken? In that case it does not make sense to > > have stronger warnings - we have to wait for this to be fixed. > > I did not know that enum support is broken. Is this a regression? > Please open a bug for the same, so that it can be fixed. > > I was mainly sceptical because, for most of the cases such as ASTNode types > and IProblem which are provided by JDT.Core, conversion to enum types may > not be a feasible option. For those Enums that are in Jdt.ui which are not > APIs, they can be changed to enums as necessary. So the cases need to be > considered separately and not as one single problem. Obviously jdt.core explicitly makes sure that this behaviour is there. I don't understand that. It does not make sense imho. Checking missing cases in a switch over enum is of big practical relevance for some kind of software and not being able to check that imho a major flaw. This is the test that jdt.core uses: // normal error when other warning is enabled public void test146b() { Map options = getCompilerOptions(); options.put(JavaCore.COMPILER_PB_SWITCH_MISSING_DEFAULT_CASE, JavaCore.WARNING); this.runNegativeTest( new String[] { "X.java", "public class X {\n" + " enum MyEnum {\n" + " A, B\n" + " }\n" + " final String test;\n" + " public X(MyEnum e) { // error\n" + " switch (e) {\n" + " case A:\n" + " test = \"a\";\n" + " break;\n" + " case B:\n" + " test = \"a\";\n" + " break;\n" + " // default: test = \"unknown\"; // enabling this line fixes above error\n" + " }\n" + " }\n" + "}\n" }, "----------\n" + "1. ERROR in X.java (at line 6)\n" + " public X(MyEnum e) { // error\n" + " ^^^^^^^^^^^\n" + "The blank final field test may not have been initialized\n" + "----------\n" + "2. WARNING in X.java (at line 7)\n" + " switch (e) {\n" + " ^\n" + "The switch over the enum type X.MyEnum should have a default case\n" + "----------\n", null, true, options); } And I don't understand your arguments against introducing an error checking for new code. Nobody forces you to change legacy code to use enums. Only code in jdt.ui making use of such old code that cannot be updated because of backward compatibility concerns would get a default case. And for new code one would reminded that an enum might be a type safe replacement. But if even checking missing cases is impossible because of jdt.core limitations that all does not make sense. |