squid:S1309 The @SuppressWarnings annotation should not be used - add option to use whitelist

[Adam Gabryś]

Hi,

I think that would be nice if the rule “The @SuppressWarnings annotation should not be used” (http://nemo.sonarqube.org/coding_rules#rule_key=squid%3AS1309) has option to switch from blacklist to whitelist. With whitelist you have a much larger control. What do you think? I can prepare a pull request. Default value will be equal to “blacklist” (backward compatibility).

 

Regards,

    Adam Gabryś

[Michel Pawlak]

I agree, it's a best practice to first forbid all then allow what is considered as ok.

Kind regards,

Michel

[Adam Gabryś]

Here you are: https://github.com/SonarSource/sonar-java/pull/326

 

Regards,

    Adam Gabryś

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/a2eaa082-360d-4f59-a833-2a56cf9f6f67%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Nicolas Peru]

Hi Adam,

Thanks for your contribution. I can understand the need for a whitelist but adding a parameter to allow to switch from one approach to another is definitely something we want to avoid here. It adds complexity of configuration which is something we really (_really_) want to avoid, as such I can't accept your PR as is.

So question would be more about : shall we approach this rule with a white list or black list ?

IMO, this approach by black list is sufficient for a lot of users, as by default it will raise issue for every annotation but please feel free to convince me otherwise.

Cheers, 

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/DUB129-DS14A524EE4C7629D1B552FB988F0%40phx.gbl.

[Adam Gabryś]

Hi,

With the whitelist we have full control, with blacklist we have “some” control. I added parameter for backward compability, but if we decide that rule should support whitelist, then I can remove it (with blacklist support).

 

Regards,

    Adam Gabryś

[Michel Pawlak]

Hi Nicolas,

After having re-read the rule's description and example I must say that it is rather difficult to understand. I just got that the rule only raise issues for suppressions that are blacklisted (i.e. with default configuration the rule never raises an issue, am I right ?)

I agree with Adam that the rule should by default raise a violation for ALL @SuppressWarnings it finds. Providing a white list can enable the definition a list of allowed suppressions. As I wrote previously it's a best practice in security to block all then allow what is considered non-dangerous. 

As-is this rule is currently useless in our context, however by reversing its behavior as Adam suggests it becomes an interesting rule (as we want to force developpers to not use suppressions unless there is no other way) .

By the way some suppressions should be allowed by default (for instance unchecked on an overridden method signature) in order to avoid false positives :-)

Kind regards,

Michel

[Michael Gumowski]

Hey,

I agree with you Adam and Michel and I also think that the rule should be changed to a white list behavior.

@Michel: To be clear, the current implementation of the rule will forbid all the SuppressWarnings when activated with an empty list (default), and will only forbid the one from the list if warnings are provided.

Instead of adding a new parameter to switch mode (blacklist/whitelist), I would rather go for a full whitelist implementation and drop the blacklist behavior. Handling authorized situation may be a bit tricky however.

As the rule is not activated by default, it will only annoy people having it already activated... But it will completely change its behavior. As it is a breaking change, a ticket should be created on our side to keep a track of it.

Regards,

Michael GUMOWSKI | SonarSource

Software Developer @ Language Team
http://sonarsource.com

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/4a70f2da-ec69-4750-87f8-e48835968345%40googlegroups.com.

[Michael Gumowski]

And here is the associated ticket: http://jira.sonarsource.com/browse/SONARJAVA-1202

Michael GUMOWSKI | SonarSource

Software Developer @ Language Team
http://sonarsource.com

[Adam Gabryś]

Hi,

I will implement this today.

 

Regards,

    Adam Gabryś

 

From: Michael Gumowski

Sent: Thursday, July 30, 2015 9:54 AM

To: Michel Pawlak

Cc: SonarQube ; adam....@live.com ; Nicolas Peru

Subject: Re: squid:S1309 The @SuppressWarnings annotation should not be used - add option to use whitelist

 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/CABGmqn%3DKvcgAQ9VeZe8Dd_Q9ebzc7s-y4eCD3g%2BV5RG-ZPO0Wg%40mail.gmail.com.

[Adam Gabryś]

Done: https://github.com/SonarSource/sonar-java/pull/345

 

Regards,

    Adam Gabryś

[Michael Gumowski]

Thank you Adam, I'll look at it asap!

Regards,

Michael GUMOWSKI | SonarSource

Software Developer @ Language Team
http://sonarsource.com

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/DUB129-DS22F3B2334EADB983D518F898760%40phx.gbl.

[Michael Gumowski]

Hello,

The Pull request has been merged on master.

Thank you for your contribution Adam. Thanks to you, the rule is now based on whitelist!

Regards,

Michael GUMOWSKI | SonarSource

Software Developer @ Language Team
http://sonarsource.com