Rule-specific NOSONAR

The main options for blocking reporting of an issue in SQ are either to mark the issue itself, or annotate the code, either with NOSONAR or @suppresswarnings(). We normally use the former but would like to use the latter in some cases.

For instance, the null pointer rule java:S2259 often has false positives. In many cases (at least for US) this is because our code has convoluted logic where we know an NPE can’t occur at a certain place, but only because we have high-level knowledge of the application that is beyond SQ’s ken. We could mark that instance FP, but there’s the risk that a dev might make a subtle change that then violates the assumptions and makes the NPE possible.

For such cases, we’d prefer an explicit annotation in the actual code. Something that says, in effect: “WE TOOK THE GUARD RAIL OFF HERE, SO IF YOU CHANGE THIS METHOD, YOU’D BETTER MAKE SURE YOU DIDN’T INTRODUCE AN NPE HERE, OR WE’LL HANG AN ALBATROSS AROUND YOUR NECK!”

We could tag the specific line of code with NOSONAR, but that suppresses ALL issues on that line. @Suppresswarnings gives rule-specificity, but only at the granularity of a method (if I’m reading current documentation correctly). What I’d like is to combine the two, something like //NOSONAR(“java:S2259”) or make @Suppresswarnings only apply to one region of code.

Hi,

It’s not clear to me what you expect.

This part:

makes me think you want/expect the previously-ignored-in-code warning to be re-raised when the code is changed…?

That’s possible with the Won’t Fix/False Positive mechanism implemented server-side; when the code changes the issue can be re-raised. But with an in-code notation of “these are not the droids you were looking for” … that code will always be “not the droids” regardless of whether it changes or not.

 
Ann

Well it gets tricky with S2259, the NPE check, because there can be large distances involved. We might have something like this pseudocode:

  1. X = null;
  2. do stuff
  3. if (Y) { X = a proper object }
  4. do a bunch of stuff
  5. if (Z) { // Z only true if Y was true before
  6. foo(X.bar)

SQ can’t prove there doesn’t exist a path from 1 to 6 and therefore flags it. (Sometimes the logic can be quite convoluted. For instance, Y and Z could be identical method calls, and we have high-level knowledge that the two calls will return the same value, but SQ doesn’t know that.)

If we mark it FP, what happens if someone goes in to part 4 and breaks the connection between Z and Y? Is changing any line of code in the flow between 1 and 6 sufficient to tickle SQ so it ignores the FP setting and calls the NPE a new issue?

(And I love the Star Wars line!)

Hi,

Actually… once you finally upgrade to a supported version :wink: I think you’ll find the detection has gotten a lot better and this is probably possible. I’m not going to swear we’ve worked on the NPE rule yet, but if we haven’t it’s on the list.

 
:smiley:
Ann

Yeah yeah… :flushed: But we’re getting side-tracked. My OP said “for instance” so the NPE was just one instance where I could use what I’m asking for – being able to make //NOSONAR apply to just one or some rules.

Hi,

With no expectation that the issue would be re-raised when the code changed?

 
Ann

Right. That’s how //NOSONAR currently works, right?

Yes. It is.

I also think this would make a lot of sense when all you want is ignore a specific issue on a specific line:

  • We shouldn’t ignore all rules on the same line (with NOSONAR comments), just the problematic one.
  • We shouldn’t ignore a specific rule in all lines of a file (with module-level ignores using a pattern).

And it would align with other tools, for example:

  • ESLint has // eslint-disable-line no-alert.
  • TypeScript has @ts-expect-error, which works differently, but just as good.
References:

And this is a little off topic, but we should also support putting standalone # NOSONAR comments on previous lines in (not just)Dockerfile files, because otherwise:

Cool – I didn’t know SQ could do Docker… learn something new every day. :slight_smile: I’ll tell our Docker folks…

Hi all,

Great news @rkrisztian, we’ve since introduced sonar-resolve, which requires that you specify rule key(s) and leave a comment.

 
HTH,
Ann

Oh, sorry, I forgot to subscribe to the blog. This is the entry I missed:

Hi, @ganncamp,

Unfortunately, I still see downsides of the current situation:

1. Only C/C++/Objective-C supported at the moment. I would assume this will change in the future, but the documents linked above do not make it clear.

2. It still requires a comment on the same line. Look at for example what I have to do for HTML template code in an Angular project:

@@ -12,7 +12,11 @@
   <mat-card-content>
     <mat-form-field>
       <mat-label>Name</mat-label>
-      <input matInput [formField]="nodeEditorForm.name" placeholder="Enter node name" />
+      <!-- //NOSONAR Web:InputWithoutLabelCheck: label is generated by mat_label --><input
+        matInput
+        [formField]="nodeEditorForm.name"
+        placeholder="Enter node name"
+      />
       @if (nodeEditorForm.name().touched() && nodeEditorForm.name().invalid()) {
         @for (error of nodeEditorForm.name().errors(); track error) {
           <mat-error>
@@ -24,7 +28,7 @@
 
     <mat-form-field>
       <mat-label>Content</mat-label>
-      <textarea
+      <!-- //NOSONAR Web:InputWithoutLabelCheck: label is generated by mat_label --><textarea
         matInput
         [formField]="nodeEditorForm.content"
         placeholder="Enter node content"

The comment is not even on the end of the line, otherwise, SonarQube for IDE won’t pick it up.

=> For this reason, it would help readability a lot if we could place the comment on the preceding line.

Thanks for considering.

Hi,

That’s my understanding, yes. We designed this for and implemented first in C-Family languages as part of our MISRA support. But it was designed as a replacement for //NOSONAR.

For languages like COBOL where you can’t comment at the end of the line, we do look at the line before for //NOSONAR and I would expect the same for sonar-resolve. This is a language-by-language implementation detail though. To date, for languages where you can comment on the same line, that’s the behavior we expect: a suppression comment where the issue is raised.

I’ll pass this on to PMs tho. Can you give an example of your ideal comment format?

 
Thx,
Ann

I don’t think it should matter much, as long as the comment is allowed to be placed on the preceding line of what is still supported by the language, although see my comment about certain linting tools challenging the idea.

But here you go, I can show some examples, as long as you keep in mind that I don’t mean to specify any tiny details like whether // should be required or not (although I think it makes sense to prefer the language-specific line comment syntax over requiring // in every language):

1. Because Dockerfile does not support trailing comments:

# sonar-resolve [accept] docker:S6504 "must be writable"
COPY --chown=nginx:nginx --chmod=ug=wr,o=r docker/config.json.template /usr/share/nginx/html/config.json

2. Because for XML and related markup languages the “trailing” comment would go to the beginning of the line:


<mat-form-field>
  <mat-label>Name</mat-label>
  <!-- // sonar-resolve Web:InputWithoutLabelCheck "label is generated by mat-label" -->
  <input
        matInput
        [formField]="nodeEditorForm.name"
        placeholder="Enter node name"
  />
</mat-form-field>

3. Of course, I still find it reasonable to allow trailing comments where the language supports it and the line does not get too long, even though some style guides discourage this pattern:

auto wrapper = [fn](Args... args) {  // sonar-resolve cpp:S3584 "some reason"
    if (isSameThread()) fn(args...);
};

4. Sadly, sometimes we need to be compatible with the ignore syntaxes of multiple linter tools, and in particular, ESLint is not the most user friendly to such situations, because ESLint requires the comment to affect the next line, which makes the sonar-resolve comment have to ignore in-between comments:

export interface Constructor<T> {
  prototype: T;
  // sonar-resolve some:rule "some-reason"
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  new (...args: any[]): T;
}

This is real code from GitHub, but I don’t understand why it would work for the NOSONAR comment, regardless, it shows that I’m not alone with this idea:

export type ContextApiName = RouterContext<
  "/api/:apiName",
  {
    apiName: string;
  } & Record<string | number, string | undefined>,
  // NOSONAR
  // deno-lint-ignore no-explicit-any
  Record<string, any>
>;

5. Then of course we may still want to allow multiple tools to ignore linting with the same comment:

<... deviant construct ...>  // NOLINT sonar-resolve some:rule "some reason"

I hope I covered everything that’s important.

I found this late, but another interesting case is formatters like Biome enforcing line wraps, so it’s not necessarily just a style guide issue. Because of that, the NOSONAR comment never lands on the same line as where the issue is, unless you disable formatting, like in this example with a max length of 100:

export const SomeContextProvider = ({ children }: { children: ReactNode }) => {
  // (...)
  return (
    // biome-ignore format: NOSONAR comment must stay on the same line
    <SomeContext.Provider value={{ data1, data2 }}>  {/* NOSONAR: typescript:S6481: using React 18+. */}
      {children}
    </SomeContext.Provider>
  );
};

Without the auto-formatting disabled, Biome changes the code to this:

  return (
    <SomeContextProvider.Provider value={{ data1, data2 }}>
      {/* NOSONAR: typescript:S6481: using React 18+. */}
      {children}
    </SomeContextProvider.Provider>
  );

By the way, I know that there is already an issue report about this particular problem:

And I also know that the better fix is disabling the rule for the whole project. I am only showing this case as a working example.