[Lint] Specify default Qube binding in workspace

There should be a way to specify a default/fallback rule set, that actually comes from SonarQube for projects without a specific SonarQube binding.

Currently all not bound projects produce A LOT of unwanted problems, because the Lint settings are at default (and they cannot be imported and hence set to something useful).

Hi @mfroehlich

We are always interested to improve our default rule settings. Don’t hesitate to share concrete examples.

All SonarLint flavors allow to configure rules for non bound projects. This is not “coming from SonarQube” but I’m not sure to see the value when you can simply bind the projects if you really want to use a SonarQube specific profile.

Hi Julien, thanks for your reply.

I’m not sure, if it’s worth the effort to share my defaults and thoughts behind disabling over 100 rules, that I find misleading. I guess, you’ll have a completely different optinion, which is fine. But I will see to collect some thoughts about it.

I see two problems here.

  1. I cannot export/import the lint rule settings. Hence I cannot share them with my colleagues or even across workspaces. In the end that’s what Qube is for. But an export/import wouldn’t hurt anyway. And being able to import a lint export into qube would be great.

  2. If I just created a test project or imported code from someone, I don’t want to bind that to sonar Qube, because it may be of temporary character and I don’t want tobother anyone to pollute our valuable Qube environment with all my local/temporary projects. But having all these masses of unwanted sonar problems makes these projects a bit unusable.

Ok, here is a “short” list. Maybe it is incomplete.

Nested "enum"s should not be declared static (java:S2786)

This rule shouldn’t be enabled by default, because it doesn’t bring up a bug or a code smell, but
just “style”. I believe, static is correct here to point out, that we don’t want hidden references
to enclosing instance like it is the case for regular inner classes. Yes, enums are always static.
But like using public modifier for interface methods, even if it is default, even enforcing/encouraging
static modifier here is a good idea.

Cognitive Complexity of methods should not be too high (java:S3776)

This rule belongs to the ones, that should never be active for regular tests, but just applied on demand.
Some methods are complex, because they are complex. Yes, you could split them up into several methods.
But this doesn’t always recude cognitive complexity.
“Never change a running system.” So, you will probably not tough these too complex methods ever. But
suppressing the issue is not a good solution, too. Hence this rule should not be applied for regular
test runs, but just on demand through an explicit “strict” analysis.

Local variables should not be declared and then immediately returned or thrown (java:S1488)

This rule should be optional. It doesn’t describe a bug and not even a code smell in my opinion.
When Eclipse didn’t show the method return value in the debugger you had to help yourself by writing
the return value to a variable before returning it. So this thingy will show up a lot in existing code.
Even since Eclipse does show the return value in the debugger, it is not any kind of bad practice to
store the value of a (complex) computation to a variable just to return that variable.
It is not even a matter of reduced performance. In contrary the degugger performance is highly reduced
due to that option. In the end it is just style. And no default rule should comply about that.

Sections of code should not be commented out (java:S125)

This rule belongs to the ones, that should never be active for regular tests, but just applied on demand.
Sometimes you may want to know, where code has been commented out and molds there for a long time.
But for every day code work, it is absolutely ok to comment out some code. It sometimes even has
important documentary character.

Track uses of “FIXME” tags (java:S1134)
Track uses of “TODO” tags (java:S1135)
Deprecated code should be removed (java:S1133)
(maybe others like that)

Eclipse (like other IDEs, I think) have separate tools to list TODOs. It is of no worth to list them
as bugs or code smell in sonar. At the very least, these rules should be deactivated by default.

S1133 is very similar. You don’t want to be notified about that in your every day code report, but
just check that before a new release or so.

“switch” statements should have at least 3 “case” clauses (java:S1301)

This rule often hits for switches, that are either still in the works or which’s enums are still being
developed. So for most cases this doesn’t show up a code smell, but just list, where you’re currently
working on.
This rule should be applied on demand only and not be activated by default.

“throws” declarations should not be superfluous (java:S1130)

This rule complains about RuntimeExceptions bein explicitly thrown. This has documentary character
and is in no way a code smell or bug.
While some may consider this rule useful, it should not be active by default.

This rule also hits redundant exceptions due to inheritance. This is of more use, but also not really
a bug or even code smell, but just has info character.

Methods should not have too many parameters (java:S107)

This rule belongs to the ones, that should never be active for regular tests, but just applied on demand.
You’ll probably not “change a running system” just to reduce the number of parameters.

Collapsible “if” statements should be merged (java:S1066)

I don’t think, it is a good idea to always merge collapsible if statements. This potentially produces
unreadably long lines and doesn’t help when debugging.
This rule should be optional and be deactivated by default.

Unused method parameters should be removed (java:S1172)

Eclipse gives you the option to ignore unused method parameters, if they’re either documented,
annotated with ignoreWarnings or the method comes from an interface or abstract class. All three
options should be possible for this rule, too. And they should be “on” by default.

“static” base class members should not be accessed via derived types (java:S3252)

I don’t understand the worth of this rule at all. Should I change my code every time library code
decides to move a constant to a super class or interface? Is it important for me to know, in which
class or interface a constant is defined, if I use it though a sub class?

1 Like

Hi @mfroehlich

Just to thank you for your detailed feedback. We have started evaluation of each point. It may take a bit of time but I will come reply to this thread with the outputs.

Wow, thanks. That’s great. I didn’t expect that.

There used to be a rule, that complained about redundant interfaces. So, if my super class implements Closeable and my sub class does that, too, Sonar complains.

I can’t find that rule anymore. Maybe it got removed. If it still exists, it should be deactivated by default as well. The reason is, that the super class may not be under my control. And if I want to make sure, that my class implements Closeable, I should do that regardless of the super class already doing it.

Of course this rule is of very find use for on-demand “strict” or “info” scans.

Hi @mfroehlich

There is in fact an existing thread for your first feature request, with a suggested workaround that we think is easy enough to make the topic low priority on our side (which doesn’t mean we won’t do it :).
I encourage you to vote/watch this thread.

Regarding the rule specific feedback (thanks again), we had discussions in the team, that I will try to sum up.

Some rules are annoying when you are in the middle of editing code
This issue is common to most linters/compilers AFAIK.


Your suggestion to have 2 analysis modes, where some rules are only checked in the “full” mode is not in our plans. We want to help developers to find issues as soon as possible, and not relying on the developer to remember to run a manual scan before pushing code.
If this is at the cost of some false positive issues being displayed during a short moment, for now we consider this to be acceptable.

FIXME, TODO, deprecated code related rules are duplicating existing IDE features
We agree those rules can be too verbose, and with low value if IDE already handle those concepts. We plan to disable them by default. Of course, if using connected mode, the configuration from the server will still apply, to ensure consistency with SonarQube.

Controversial rules (java:S2786, java:S2333)
We already did a big effort to remove most controversial rules, but there will always been some disagreement. We consider those rules as educational, so we prefer to keep them on by default. Of course you can disable them in your own quality profile.
Adding filtering capabilities to hide minor/info rules temporarily could also help. If you are interested I encourage you to vote for:

Never change a running system (cognitive complexity, too many method parameters, …)
We can only agree. We have to rethink/improve the concept of leak period in SonarLint. Not scheduled this year but definitely a topic we are interested to work on. You can watch/vote for:

Code patterns used to improve the debugging experience
We tend to disagree with such practices, especially if it can hurt maintainability/readability. But that’s also a bit opinionated. You can of course decide to disable those rules.

Support of Eclipse specific @SuppressWarnings()
In particular @SuppressWarnings(“unused”). Nice suggestion.
Ticket created: https://jira.sonarsource.com/browse/SONARJAVA-3558

Declaring RuntimeException to be thrown (java:S1130)
We tend to agree with the documentation benefit, so we will change the rule:

1 Like

Nice. Did that.

Well, I don’t know, how this is handled in other companies. But from my experience I know, that code is not touched by many developers until they have to, because changes would require more or less intense testing, which they don’t want to put any effort into just to follow some rules.
Hence may issues are not being displayed for short moment, but rather a VERY long time, maybe even forever. This is, why I strongly vote for these two scan modes. It’s better to have frequent scan to only report those issues, that have a chance to be actually fixed, but not ignored and being annoyed about. Not talking of myself of course. :wink:

Nice! Thanks.

Of course I have disabled them in my quality profile. But using default Lint settings this is still on for unbound projects.

I see the point of educational rules. And I support this to be very clear. On the other hand I disagree with these rules at all. I think, it is bad habit to skip static, private and public modifiers, where they’re default. I would consider it as educational to force developers to use them explicitly, because it makes code much better understandable.

I have reduced all SonarLint findings to INFO anyway. So there’s no reason for me to vor for that topic.

:+1:

:+1:

:+1:

Thanks for the detailed feedback, Julien. I’m glad, that my own feedback has led to to improvements.

1 Like

FYI I’m closing this thread since the main topic is a duplicate of Improve rule configuration for non-connected projects
and we want to collect all votes/feedback on the same thread to ease the prioritization.

This topic was automatically closed after 8 hours. New replies are no longer allowed.