Community
Participate
Working Groups
The goal is to allow a development team to use the toolset as an automatic and reliable enforcer of a zero-warning coding policy. Currently, this isn't possible with JDT. I've attached a proposal document and a patch that implements the proposal.
Created attachment 152561 [details] implementation of solution in proposal
Created attachment 152562 [details] proposal
Created attachment 153402 [details] proposal solution Updated patch file. Fixes scenario where unused import warnings were accidentally suppressed. Also fixes JUnit test that wasn't expecting the new option in the compilation log.
John, I understand the need for better/faster control over all those preferences but don't like the direction taken in the patch. I'd prefer an approach similar to the 'Plug-in Development' > 'API Errors/Warnings' preference page where the existing preferences can be changed to a certain level with a single click. We already have bug 248306 for that. I suggest to close this bug here and you try to provide a patch for bug 248306.
(In reply to comment #4) > John, I understand the need for better/faster control over all those > preferences but don't like the direction taken in the patch. I'd prefer an > approach similar to the 'Plug-in Development' > 'API Errors/Warnings' > preference page where the existing preferences can be changed to a certain > level with a single click. We already have bug 248306 for that. I suggest to > close this bug here and you try to provide a patch for bug 248306. Dani, I think you're overlooking a key element to my proposal. The issue is not simply about changing preference values (I'll happily accept an alternate solution to that deficiency). Basically, we need a new warning/error state. One that allows a particular coding pattern to be flagged as an error but silenced all the same via @ignoreWarning. That's not possible today. If you read the section in my document titled "Deficiencies in JDT Today", this will become clearer.
I know it's not exactly what you want (especially when it comes to @suppress*) but I'm still not convinced that we need a new compiler option for that. What you try to solve here is rather a cultural problem that people tend to ignore warnings ;-) and the lack of a "@SuppressErrors" annotation.
(In reply to comment #6) > I know it's not exactly what you want (especially when it comes to @suppress*) > but I'm still not convinced that we need a new compiler option for that. What > you try to solve here is rather a cultural problem that people tend to ignore > warnings ;-) and the lack of a "@SuppressErrors" annotation. Indeed. I am trying to get JDT to help development teams solve a behavioral problem, and I don't see anything wrong with that. Much has already been done based on that objective. E.g., the fact that you can make a syntactically valid but undesirable coding pattern an error (instead of just a warning) has that same motivation behind it. Again, the capability I'm proposing is not revolutionary. It's standard in most development tools.
Is it really a good idea to have a state where the same problem is treated as a warning (-> suppressable) and also as an error? Is it really the responsibility of the compiler to accommodate your use case? Or could this request be modified to look for a hook where your policy could be implemented? E.g., team support could list a number of conditions that a specific source file must fulfil before commit, like, not having any warnings, having warnings only of specific categories etc. Doesn't team support provide some commit hooks? Maybe it's just a matter of exposing such things as preferences? At least I couldn't find such preference. OTOH, if you keep toggling your warning preferences, this will incur frequent full builds of your project/workspace, which sounds pretty bad to me. I would suggest to leave warnings as warnings, just tell other tools downstream whether or not (some) warnings are acceptable. Perhaps only some Export wizard should reject code with warnings (E.g., jar export already reports this - configurably). After all you are speaking of a zero-warning policy, not a zero-error policy. Let's not abuse errors for that. my 2c.
(In reply to comment #8) > Is it really a good idea to have a state where the same problem > is treated as a warning (-> suppressable) and also as an error? > ... I think it *is* a good idea. I think this is a basic enough capability that it shouldn't be pushed off to downstream tools. Not to mention, it's not only easy to present (GUI-wise), it's easy to implement. Again, I'll just point out this is standard in other tools. Also, I don't get the point about toggling the warning preference. My use case doesn't involve that. I want to turn on "promote warnings to errors" and leave it on indefinitely, to ensure warnings don't creep into the codebase.
>I want to turn on >"promote warnings to errors" and leave it on indefinitely, to ensure warnings >don't creep into the codebase. So simply look more careful for warnings. That's not a big deal once you have your code freed from all of them. If it helps: you can change the color for warnings to red in the Java editor or as Stephan pointed out, let the team provider reject files that have warnings and/or errors.
(In reply to comment #10) > So simply look more careful for warnings. That's not a big deal once you have > your code freed from all of them. If it helps: you can change the color for > warnings to red in the Java editor or as Stephan pointed out, let the team > provider reject files that have warnings and/or errors. Wow. OK. I feel like I'm beating a dead horse here. What I've proposed (and provided an implementation for) is something that will help development teams improve code-quality without manual policing. It's a fairly common-sense and standard feature, or at least I thought it was. What I'm hearing is "get some other tool to help you, or do it via manual inspection". Not really the response I was expecting, but oh well. I thank you guys for at least considering and discussing it. Thankfully, this is all open source and the feature is now part of the JDT we use to develop our commercial product. I was hoping I could use it in some of our CDT projects (as a committer), but it's looking like I'm not going to be successful. To quote Obama's understatement of the year: "you can't win 'em all."
I also don't like the "Promote all warnings to errors" option, mainly because it's not clear from the name what the effects of this option are. Couldn't we just make @SuppressWarnings also work if the suppressed warning has been promoted to "Error"? To see all suppressed problems, the user can still disable the @SuppressWarnings annotations. We could also add an option in the 'Annotations' group to toggle this explicitly (e.g. "Suppress optional errors with @SuppressWarnings").
(In reply to comment #12) > I also don't like the "Promote all warnings to errors" option, mainly because > it's not clear from the name what the effects of this option are. > > Couldn't we just make @SuppressWarnings also work if the suppressed warning has > been promoted to "Error"? To see all suppressed problems, the user can still > disable the @SuppressWarnings annotations. > > We could also add an option in the 'Annotations' group to toggle this > explicitly (e.g. "Suppress optional errors with @SuppressWarnings"). I initially suggested that @SuppressWarnings idea on the mailing list, but that idea was as well received as this one. I'm just looking for JDT to address the use case. How it's supported is really not that important to me. I think either of the two approaches would be fine. But I just don't see interest in supporting the use case, so I'm not holding my breath.
(In reply to comment #12) > Couldn't we just make @SuppressWarnings also work if the suppressed warning has > been promoted to "Error"? Sounds good to me. > To see all suppressed problems, the user can still > disable the @SuppressWarnings annotations. > > We could also add an option in the 'Annotations' group to toggle this > explicitly (e.g. "Suppress optional errors with @SuppressWarnings"). Thanks for suggesting this. I keep thinking that temporarily seeing again all suppressed warnings would be very helpful. If others agree I might propose a patch for this.
(In reply to comment #14) > If others agree I might propose a patch for this. I think implementing this might be the better way to do it. I don't like the first option to convert warnings to errors using a new option, as this should be done by changing the corresponding options. Now SuppressWarnings should work as its names stands for for warnings only. As we have a lot of warnings, I can see the use case of making it work for optional errors as well. So I would go in the direction of comment 12. If Markus suggested it, I am sure he will add the corresponding option in the UI. Stephan, if you can prepare a patch for this to fix the JDT/Core side, this would be great. Otherwise let me know and I'll do it. Thanks for the offer. I'll take the bug as I'll review the patch when ready.
Sounds good to me too. >I'll take the bug as I'll review the patch when ready. I suggest you assign it to Stephan and set the review flag to ? with your name behind.
> If Markus suggested it, I am sure he will add the corresponding option in the > UI. Yes (but I also wouldn't be upset if the patch already contained the UI;-). If this new sub-option of JavaCore#COMPILER_PB_SUPPRESS_WARNINGS is not enabled by default, then it would also be nice to have a quick fix that checks whether a SuppressWarnings annotation would hide a given optional error if it were just a warning, and if so, offer to enable the new sub-option. If someone (maybe John?) is keen on adding buttons to set all optional problems to Ignore/Warning/Error, we would accept a patch in bug 248306.
ok, done. Stephan, let's target M6.
(In reply to comment #17) > If someone (maybe John?) is keen on adding buttons to set all optional problems > to Ignore/Warning/Error, we would accept a patch in bug 248306. Unfortunately, 248306 does not really serve the use case I detailed in the attached PDF. The idea is to let the user quickly turn settings that are currently "warning" to "error". 248306 is about turning *all* settings in a page to a particular value. While that my be useful in some situations, it doesn't address my needs. If I have a page with ten settings and three of them are set to ignore, and I hit the "Set all to errors", I will then have to (a) remember which ones where "ignore", and (b) go through and manually flip them back to "ignore". It would be no harder for me to just go through and manually flip the "warning" ones to "error"; it may in fact prove to be easier overall. Now, where we to have a *button* called "Promote warnings to errors" that changed only the settings that are currently set to 'warning', then that addresses my use case. But if also throw in the three buttons to switch *all* settings to a value, then it becomes too messy in my opinion. The more I think about it, the cleaner my initial proposal/approach seems to me. But I understand no one that has spoken up agrees with it. That said, if the @SuppressWarnings behavior is changed as it appears it will, I'm happy with that. The second aspect to the use case can be handled by manipulating the contents of the pref files with a search-n-replace editor operation (my proposal describes that as acceptable)
Stephan, Any news on a possible patch for M6? Thanks.
Created attachment 160128 [details] Proposed patch - core part Hi Olivier, > Any news on a possible patch for M6? Sorry for the long wait. I was just slightly overwhelmed by moving Object Teams into Eclipse (e.g. pushing CQs for tens of MByte of code :) The proposed patch for the core part is actually mostly trivial. Only aligning the different usage levels may be a bit tricky: So in CompilerOptions we now have two flags, suppressWarnings and suppressOptionalErrors. Here suppressWarnings is stronger, i.e., if that's false nothing will be suppressed. These flags are exposed via JavaCore where I added COMPILER_PB_SUPPRESS_OPTIONAL_ERRORS. (The javadoc could use some smoothing I'm afraid). With the batch compiler you can now say -err:+suppress which sets both internal flags to true. I'm not checking for inconsistent combinations of, say, -warn:-suppress -err:+suppress, should I? I noticed that the command line help might be a bit misleading, as the above would literally translate into "disable the 'suppress' warning and enable 'suppress' to be reported as error". Are there more suboptions that are *not* warnings/errors? For the UI I would actually like to have only one combo box where you can select the "strength" of @SuppressWarnings: NONE|WARNING|ERROR. S.t. like "Select the level of problems that can be suppressed with @SuppressWarnings" or "Enable @SuppressWarning annotations to suppress: Nothing|Warnings|Errors" Would it be clear that "Errors" includes "Warnings"? please let me know what you think, Stephan
(In reply to comment #21) > Only aligning the different usage levels may be a bit tricky: I'm sorry, but I just don't see how this approach is being deemed less confusing than mine. My approach didn't change the semantics of "@SuppressWarnings", it introduces a single checkbox, and it completely addresses the use case with a high economy of user activity. The current proposal makes the semantics of @SuppressWarnings configurable, despite the annotation being explicit about what it's supposed to suppress. Furthermore, to address the use case, a user must now flip many individual project settings from warning to error, AND also modify a new combobox that is difficult to accurately describe and understand. Yes, the current proposal gives a higher level of control, but I contend that it's an over engineered solution to a fairly simple problem. I predict the higher level of control will benefit very few developers, but will likely introduce a good deal of confusion, Anyway, I would really appreciate it if those involved would re-examine the use case I detailed in the PDF and compare both proposals. I PROMISE this will be my last post on this matter and I'll sign off by thanking everyone who's been putting their time into this issue.
(In reply to comment #22) John, we do try to help you. I'm not a JDT committer but I created my patch because I hope it will help many people with similar needs, just by making this one thing configurable: how strong are @SuppressWarning annotations. This should solve exactly one of your problems, while being open for other uses. All arguments against your solution are not saying it doesn't work, but only that we are afraid it might interact badly with other options and habits. We can't tailor the tool to just fit your one use case without thinking about other use cases. What confuses me about your proposal (and perhaps others too?) is that it seems to introduce yet another level of severity: a problem can be ignored it can be a warning it can be reported as a error but suppressable like a warning it can be a non-suppressable error that still allows compilation to complete it can be a fatal error causing no code to be created Saying a problem is almost like an error but also almost like a warning requires too much explanation. I believe most people prefer if a problem is clearly either a warning or an error. But then we can have various tools to work with those errors and warnings. One such tool is the @SuppressWarnings annotation which my patch makes configurable. There are other tools in the chain. Wait, what SCM are you using??? What about this nice little option in the CVS preferences: "Commit resources with warnings: o Yes o No o Prompt" "Just say No" :) and you don't even need my patch. Am I missing anything? (Actually, by introducing not-quite-errors we would be messing up with such tools, because nobody would be certain whether an almost- warning would be counted as an error or a warning by these tools).
(In reply to comment #12) > We could also add an option in the 'Annotations' group to toggle this > explicitly (e.g. "Suppress optional errors with @SuppressWarnings"). (In reply to comment #21) > For the UI I would actually like to have only one combo box where > you can select the "strength" of @SuppressWarnings: NONE|WARNING|ERROR. > S.t. like > "Select the level of problems that can be suppressed with @SuppressWarnings" > or > "Enable @SuppressWarning annotations to suppress: Nothing|Warnings|Errors" > Would it be clear that "Errors" includes "Warnings"? I also thought about that, but discarded it because it just saves 1 line but causes more problems: - Even with the option enabled, you can't suppress non-optional errors. So just talking about "Errors" would be wrong. - As you said, it's not immediately clear that "Errors" also suppresses "Warnings". - The description of the combo becomes too long and thus hard to read if it needs to convey all the information. 2 separate options are easier to understand. Therefore, I think we should just keep the existing checkbox as a global switch and add "Suppress optional errors with @SuppressWarnings" as a sub-option. BTW: The existing "Unused '@SuppressWarnings' token:" could also be shown as a sub-option of the global "Enable '@SuppressWarnings' annotations" checkbox.
(In reply to comment #24) > Therefore, I think we should just keep the existing checkbox as a global switch > and add "Suppress optional errors with @SuppressWarnings" as a sub-option. > BTW: The existing "Unused '@SuppressWarnings' token:" could also be shown as a > sub-option of the global "Enable '@SuppressWarnings' annotations" checkbox. I think I would also favor this approach. It is inlined with other options that have sub options and it is clearer for the user that optional errors and warnings can be supress using @SuppressWarnings. To be honest I am not a big fan of using @SuppressWarnings to filter out errors even if they are optional errors, but I can accommodate this use case. It should help team work where team members might not share the same compiler preferences. Stephan, is it possible for you to make changes to the patch to match what Markus described? Thanks.
(In reply to comment #25) > (In reply to comment #24) > > Therefore, I think we should just keep the existing checkbox as a global switch > > and add "Suppress optional errors with @SuppressWarnings" as a sub-option. > > BTW: The existing "Unused '@SuppressWarnings' token:" could also be shown as a > > sub-option of the global "Enable '@SuppressWarnings' annotations" checkbox. > I think I would also favor this approach. It is inlined with other options that > have sub options and it is clearer for the user that optional errors and > warnings can be supress using @SuppressWarnings. > > To be honest I am not a big fan of using @SuppressWarnings to filter out errors > even if they are optional errors, but I can accommodate this use case. It > should help team work where team members might not share the same compiler > preferences. > > Stephan, > > is it possible for you to make changes to the patch to match what Markus > described? I haven't yet started with the UI part :) but if the core part has your OK I can prepare a UI patch following Markus' suggestions.
(In reply to comment #21) > With the batch compiler you can now say -err:+suppress which sets > both internal flags to true. I'm not checking for inconsistent > combinations of, say, -warn:-suppress -err:+suppress, should I? No, the last one takes it. > I noticed that the command line help might be a bit misleading, as the > above would literally translate into "disable the 'suppress' warning and > enable 'suppress' to be reported as error". Are there more suboptions > that are *not* warnings/errors? I'll see how to rephrase this new option. For: if (severity == ProblemSeverities.Warning || isEnabling) this.options.put( CompilerOptions.OPTION_SuppressWarnings, isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); if (severity == ProblemSeverities.Error || !isEnabling) this.options.put( CompilerOptions.OPTION_SuppressOptionalErrors, isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); Should not this be: if (severity == ProblemSeverities.Warning) this.options.put( CompilerOptions.OPTION_SuppressWarnings, isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); if (severity == ProblemSeverities.Error) this.options.put( CompilerOptions.OPTION_SuppressOptionalErrors, isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); By default it is disabled. I am finishing the review.
(In reply to comment #27) > For: > if (severity == ProblemSeverities.Warning || isEnabling) > this.options.put( > CompilerOptions.OPTION_SuppressWarnings, > isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); > if (severity == ProblemSeverities.Error || !isEnabling) > this.options.put( > CompilerOptions.OPTION_SuppressOptionalErrors, > isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); > > Should not this be: > if (severity == ProblemSeverities.Warning) > this.options.put( > CompilerOptions.OPTION_SuppressWarnings, > isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); > if (severity == ProblemSeverities.Error) > this.options.put( > CompilerOptions.OPTION_SuppressOptionalErrors, > isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); Since internally suppressWarnings is the master switch I figured -err:+suppress should also switch on this master, because otherwise it won't have an effect without additionally specifying -warn:+suppress. The second "if" is just modelled for symmetry so that -warn:-suppress also switches of the sub-option, which is actually effectless except for extremes situations like "-err:+suppress -warn:-suppress -warn:+suppress" My version will suppress warnings only, in your version suppressing errors will survive the middle option. If you prefer we can also use your version which directly exposes the flags as they are used internally, and will perhaps be a bit more in line with the UI. The difference is that the UI tells you about the dependence whereas the command line options cannot easily express a "sub option". So this was my reasoning - I'm fine with whichever version you prefer. > By default it is disabled. Yes, it is.
(In reply to comment #28) > Since internally suppressWarnings is the master switch I figured > -err:+suppress should also switch on this master, because otherwise it > won't have an effect without additionally specifying -warn:+suppress. > The second "if" is just modelled for symmetry so that -warn:-suppress > also switches of the sub-option, which is actually effectless > except for extremes situations like > "-err:+suppress -warn:-suppress -warn:+suppress" > My version will suppress warnings only, in your version suppressing > errors will survive the middle option. I see. We should add tests in the BatchCompilerTests to cover these cases. > If you prefer we can also use your version which directly exposes the > flags as they are used internally, and will perhaps be a bit more in line > with the UI. The difference is that the UI tells you about the dependence > whereas the command line options cannot easily express a "sub option". No this is not something trivial from the command line. > So this was my reasoning - I'm fine with whichever version you prefer. I'll take yours. I propose the following phrasing to explain the option in the batch help message: suppress + enable @SuppressWarnings When used with -err:, it can also silent optional errors and warnings I'll add more tests and I'll release the code in core.
(In reply to comment #22) > Anyway, I would really appreciate it if those involved would re-examine the use > case I detailed in the PDF and compare both proposals. I PROMISE this will be > my last post on this matter and I'll sign off by thanking everyone who's been > putting their time into this issue. There is no problem discussing this kind of issue. I think you have a point about how this would be displayed in the UI. Now I also believe that what Markus is proposing is as simple as what you suggest. Thanks for the bug report and hope the resolution works for you.
Last comment about option consistency. I think: "-err:+suppress,unused -warn:+suppress" should only suppress warnings and not errors. unused fields would be reported as an error with these settings. This is why I would write the handling of the new option as: } else if (token.equals("suppress")) {//$NON-NLS-1$ switch(severity) { case ProblemSeverities.Warning : this.options.put( CompilerOptions.OPTION_SuppressWarnings, isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); this.options.put( CompilerOptions.OPTION_SuppressOptionalErrors, CompilerOptions.DISABLED); break; case ProblemSeverities.Error : this.options.put( CompilerOptions.OPTION_SuppressWarnings, isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); this.options.put( CompilerOptions.OPTION_SuppressOptionalErrors, isEnabling ? CompilerOptions.ENABLED : CompilerOptions.DISABLED); } return; When filtering problems, I think we should use the fact that the irritant is 0 for mandatory errors. Otherwise we also try to apply @SuppressWarnings for mandatory errors. If you agree with this, I'll release shortly. Stephan, do you also want to take care of the patch for the UI side ?
Created attachment 160217 [details] Proposed fix + regression tests
Opened bug 303945 for the UI counterpart.
Released for 3.6M6. Now we need to enable it in the UI. Se bug 303945.
Created attachment 160223 [details] Proposed fix + regression tests Same patch with minor typos fixes.
(In reply to comment #31) > When filtering problems, I think we should use the fact that the irritant is 0 > for mandatory errors. Otherwise we also try to apply @SuppressWarnings for > mandatory errors. Just for my education: did you observe mandatory errors being suppressed or is this only defensive programming? I was assuming that it's enough to know that IrritantSet.isSet(0) will always return false (i.e., irritant-less errors can never be suppressed).
(In reply to comment #36) > or is this only defensive programming? I was assuming that it's enough to > know that IrritantSet.isSet(0) will always return false (i.e., irritant-less > errors can never be suppressed). Sure, but you still check each @SuppressWarnings to find out that you have nothing to do because the irritant is in fact 0 for mandatory errors. Did I miss something ?
Verified for 3.6M6 that SuppressWarning annotation can also suppress optional error when the appropriate option is selected.
I've cleaned up all warnings in a number of CDT projects and made use of the new JDT setting to ensure the projects remain warning-free (at least in the Java code). I really appreciate all the effort that went into making that possible.
I've found a side effect caused by this feature. I opened bug 371832
