JS/TS: "Built-in objects should not be overridden" should exclude `valueOf` typescript:S2424

The rule https://sonar.etalytics.net/coding_rules?open=typescript%3AS2424&rule_key=typescript%3AS2424 checks that no local identifier shadow global ones.

In version 21, Angular introduced validation for signal forms that uses `valueOf` for conditional validation rules, see Validation • Angular and example below.

Now, at every place were we use this, sonar detects a violation of this rule, which technically is correct.

However, (as far as I understand) valueOf() is defined in Object.prototype.valueOf so it expects to be called upon an object. I can simply call valueOf() without a calling object specified, but then this is undefined and I get Uncaught TypeError: Cannot convert undefined or null to object.

In this case, I’d say it’s pretty unlikely that someone really would like to use valueOf() as global function call, so this could be excluded from this rule without causing harm.

I really would like to keep this rule enabled, and I would also prefer not to mark each of the validation rules manually as false positives.

@Component({
  selector: 'app-root',
  template: `
    <input type="checkbox" [field]="valueForm.mayNotBeEmpty" />
    <input type="text" [field]="valueForm.name" />
    <br/>{{valueForm().valid()}}`,
  changeDetection: ChangeDetectionStrategy.OnPush,
  imports: [Field],
})
export class App {
  protected readonly value = signal({ mayNotBeEmpty: true, name: '' });
  protected readonly valueForm = form(this.value, (p) => {
    required(p.name, { when: ({ valueOf }) => valueOf(p.mayNotBeEmpty) });
  });
}

Many thanks for considering this! :slight_smile:

This issue was reported to Angular in `valueOf` when destructering the callback of `hidden`, `readonly`, ... and more should be renamed · Issue #65547 · angular/angular · GitHub . That issue report also includes a workaround that calling code can use: ({ valueOf: getValueOf }).

I think it is a bad decision on Angular’s part to reuse the valueOf name from Object.prototype for something completely different, also because ECMAScript specifies that various implicit conversions call valueOf, the most likely one being code like console.log("debug: " + someFieldContext);. Note that console.log($"debug: {someFieldContext}"); is unaffected. Apparently, TypeScript and linters do not check for this kind of issue. If valueOf must be repurposed, then the issue with implicit conversions can also be fixed by defining a Symbol.toPrimitive method.

The proposed relaxation of the Sonar rule still has a small disadvantage for all code: by no longer discouraging local variables named valueOf, there will be no compile-time warning or error if the intention is to use a local variable named valueOf but the definition is missing. In this specific case, this could be compensated for by reporting a warning if the valueOf global is used (valueOf() always throws and is therefore clearly useless and valueOf.call(x) creates wrapper objects with usually undesirable behaviour; similar functionality is available as Object(x)). Another weaker relaxation could be to allow valueOf and similar names only if they originate from some property’s name.

Thanks for reporting this. After checking the rule implementation this is a false positive. There is no builtin global valueOf aside of the prototype method you mentioned. Usually we rely on the well-known globals npm package, however I’ve spotted 3 rules (S2424 among them) that still rely on an internal globals list which lists valueOf, incorrectly in my opinion. I created a ticket to fix this.

Cheers!

1 Like

Hello @ptandler,

We have now merged a PR to fix this issue: JS-1026 Use globals npm package instead of internal deprecated list by zglicz · Pull Request #6143 · SonarSource/SonarJS · GitHub

It will be addressed with the next release of the analyzer.

Kind regards,

Michal

2 Likes

Many thanks for so quickly fixing this! :heart_hands: