False positive S1172 for constructor parameter

Report a False-positive / False-negative
, ,
#1

Typescript since v3 has a feature to skip assigning constructor parameter to a same named property.
SonarLint (and SonarCloud) treats this like an unused parameter of constructor function.

Code example:

class Test {
  constructor(
    public readonly x: number,
  ) {
    // No body necessary
  }
}

const a = new Test(1);
console.log(a.x); // 1

Will cause “Remove the unused function parameter “x” or rename it to “_x” to make intention explicit.sonarlint(typescript:S1172)”

1 Like
#2

Hey there.

Please be sure to read this post before raising a thread here:

#4

Edited the post to provide all details according to “How to …”

Any updates on this?

#6

Hi @igoralekseev

I cannot reproduce the issue. What version of SonarLint and which IDE are you using?

Cheers

#10

Hi!
I am using Visual Studio Code 1.68.1 with and SonarLint v3.5.4

#11

Hello @igoralekseev,

can you try upgrading to SonarLint 3.6.0?

Let me know if that helps please.

#12

This is also an issue with SonarQube 9.5 (9.5.0.56709)

image
image1002×508 26.3 KB

@igoralekseev please also tag SonarQube, thanks!

#13

Checked with SonarLint v3.7.0 – still relevant

#14

I have the same false positive on SonarCloud as well.

#15

For more context: these popped up a few days ago and I didn’t understand why. But now I got the bottom of it: I was using the Sonar Way (recommended) profile, but a few days ago that was removed: JavaScript and TypeScript "Sonar Way (recommended)" profile upgraded.
So as part of the new profile I got the S1172 rule which apparently gives a false positive on ‘parameter properties’ (https://www.typescriptlang.org/docs/handbook/2/classes.html#parameter-properties) in typescript. The constructor in this case will usually have an empty body.

I also don’t want to disable the rule completely, what’s next?

#16

Ah, found the culprit. When the parameter property had a default value of some kind, it would give a false positive. I created Fix FP S1172: Ignore parameter properties with default value by aukevanleeuwen · Pull Request #3286 · SonarSource/SonarJS · GitHub to fix it. It would be nice if it could be merged.

1 Like
#17

Ping @Colin / @victor.diez perhaps, because they responded from SonarSource side.

#18

Hi Igor and Auke,

Thank you for your effort on this.

This does indeed look like a FP, you can track the issue on Github.

Other information:

For unrelated reasons, we decided that S1172 will no longer be included in the “Sonar Way” profile. Starting from the next update coming in the following weeks to SonarCloud, to SonarQube 9.6, and to SonarLint upcoming release. This means most users will stop seeing this false positive.

Other options:

  • If you are using TypeScript, you could benefit from TS6133, which is a check built-in to the TypeScript compiler. This will avoid any false positives, but you might need to manually enable it.
  • If you are using JavaScript and like this rule or you are not affected by these false positives in TypeScript, you can manually add S1172 rule to your profile.

Hope this helps.

Cheers,
Gabriel

#20

Hey @gabriel.vivas,

Thanks for your reply. Still want to give some replies to this.

  1. Can you perhaps provide a bit more information about why it would be removed from the profile? To me it seems like a perfectly legitimate rule to be honest. The fact that it can be done with a TypeScript configuration option doesn’t say that projects will actually do that obviously. If you use SonarCloud as a “let’s-make-sure-that-the-quality-of-the-projects-in-our-organization-at-least-adheres-to-X” kind of tool, it’s nice to have these kinds of rules in I would say? Otherwise (for TypeScript specifically) you’d need some global check in an org to figure out if everybody is using noUnusedParameters in their TS configuration?
  2. For a few TypeScript options I kind of dislike having them in my configuration while developing. Let’s say I’m creating some Cucumber test (first thing that comes to mind). Not sure how familiar you are with the framework, but if I will just write some initial “step”, the framework will generate some code for me to actually implement. I.e. something like:
Given('I eat {int} cookies', function (amountOfCookies: number) {
  throw NotImplementedError();
});

It’s really annoying if while running the tests with something like ts-node that will start barking at you because you have not used the parameter amountOfCookies yet. Same goes for something like noUnusedLocals. If I comment out some code temporarily because it will help me debug an issue faster somehow, but then I run the tests (which invoke tsc) and it starts barking at me because now I’m not using some local variable somewhere that is annoying.
However I definitely don’t want to end up in production with these issues though. Running SonarCloud in CI to prevent those kind of errors I still find useful.

  1. As a more generic remark about the above: I can fix it probably with a CI specific tsconfig.json, but I like the fact that these rules are noted by SonarQube, giving me proper feedback on my pull request, rather than having a build step first fail on compilation (because of that flag in the CI specific configuration). git commit --amend; git push that, and only then running SonarQube which would give me another set of errors.

  2. I’m not sure if you had a look at my PR, but it seems the fix is (seems?) really quite trivial. There already is an exception for a (TypeScript) ParameterProperty, the implementation was just a bit off for parameter properties that had a default value assigned as well. If you keep the rule, i.e. don’t deprecate it, don’t you still want to have this fix in?

–
Auke

#21

Hello Auke,

Thanks for sharing your workflow and compromises, I can relate.

Sorry I was not clear, the reasoning is not about this particular FP, it is about the ecosystem not settling on renaming parameters with underscores when subsequent parameters are being used. We were considering accommodating that case.

However, instead of diluting the rule, we chose to make it opt-in by default, since some users do like its current behaviour.

PS: Edited my previous message for clarity. I added a Github issue too.

Cheers,
Gabriel