linter: fix all bugs in getter-return and promote to correctness

[Boshen]

getter-return is rewritten with cfg, but it is buggy

  ⚠ eslint(getter-return): Expected to always return a value in getter.
     ╭─[vscode/src/vs/workbench/services/themes/common/colorThemeData.ts:578:13]
 577 │
 578 │ ╭─▶     get type(): ColorScheme {
 579 │ │           switch (this.baseTheme) {
 580 │ │               case VS_LIGHT_THEME: return ColorScheme.LIGHT;
 581 │ │               case VS_HC_THEME: return ColorScheme.HIGH_CONTRAST_DARK;
 582 │ │               case VS_HC_LIGHT_THEME: return ColorScheme.HIGH_CONTRAST_LIGHT;
 583 │ │               default: return ColorScheme.DARK;
 584 │ │           }
 585 │ ╰─▶     }
 586 │
     ╰────
  help: Return a value from all code paths in getter.

  ⚠ eslint(getter-return): Expected to always return a value in getter.
    ╭─[vscode/src/vs/workbench/browser/window.ts:83:82]
 82 │         const originalClearTimeout = targetWindow.clearTimeout;
 83 │         Object.defineProperty(targetWindow, 'vscodeOriginalClearTimeout', { get: () => originalClearTimeout });
    ·                                                                                  ──────────────────────────
 84 │
    ╰────
  help: Return a value from all code paths in getter.

  ⚠ eslint(getter-return): Expected to always return a value in getter.
    ╭─[vscode/src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:52:23]
 51 │
 52 │ ╭─▶     get preferredWidth(): number | undefined {
 53 │ │           const activeComposite = this.getActivePaneComposite();
 54 │ │
 55 │ │           if (!activeComposite) {
 56 │ │               return;
 57 │ │           }
 58 │ │
 59 │ │           const width = activeComposite.getOptimalWidth();
 60 │ │           if (typeof width !== 'number') {
 61 │ │               return;
 62 │ │           }
 63 │ │
 64 │ │           return Math.max(width, 300);
 65 │ ╰─▶     }
 66 │
    ╰────
  help: Return a value from all code paths in getter.

cc @camc314 @TzviPM

[BlackSoulHub]

Hi. Is there anything I can do to help?

[Boshen]

Hi. Is there anything I can do to help?

Yes, you may try and fix these false positives in https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/eslint/getter_return.rs

[BlackSoulHub]

Hi. Is there anything I can do to help?

Yes, you may try and fix these false positives in https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/eslint/getter_return.rs

Ok. I'll look into it.

[BlackSoulHub]

  ⚠ eslint(getter-return): Expected to always return a value in getter.
    ╭─[vscode/src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:52:23]
 51 │
 52 │ ╭─▶     get preferredWidth(): number | undefined {
 53 │ │           const activeComposite = this.getActivePaneComposite();
 54 │ │
 55 │ │           if (!activeComposite) {
 56 │ │               return;
 57 │ │           }
 58 │ │
 59 │ │           const width = activeComposite.getOptimalWidth();
 60 │ │           if (typeof width !== 'number') {
 61 │ │               return;
 62 │ │           }
 63 │ │
 64 │ │           return Math.max(width, 300);
 65 │ ╰─▶     }
 66 │
    ╰────
  help: Return a value from all code paths in getter.

In this example, the complaint looks justified. In places with return undefined, it is not explicitly returned.
Or is it acceptable for functions declared as:

function fn(): SomeValue | undefined

[Boshen]

https://eslint.org/docs/latest/rules/getter-return#handled_by_typescript

It is safe to disable this rule when using TypeScript because TypeScript's compiler enforces this check.

Oh ... we can disable this rule for TypeScript.

We can abort the whole rule with

ctx.source_type().is_typescript()

[Boshen]

I turned getter-rule off for typescript in #3693 but still see lots of false positives.

[rzvxa]

@Boshen Are these false positives only happening in the typescript files? It might be a logical error instead of a control flow issue. If I have access to our current false positives to create a unit test for them I can start working on a fix.

[TzviPM]

I'm just seeing this. I haven't touched this code in a while, but the CFG generally works at the level of statements, rather than expressions, if I recall correctly. Many expression-level nodes are special-cased. I'd confirm the structure of the ast with this example, both for the getter and for the switch and case statements. I'd be happy to take a look later tonight as well.

[rzvxa]

If you find the time it would be awesome, But be prepared since the code has been changed quite a bit due to recent refactors for Traverse and CFG.