Rewrite getPropFromRaw slightly to surface T in the parameters

[Andarist]

cc @jakebailey

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[jakebailey]

The problem is that below, we still return the result if not every array element matches the type guard. So, I don't really know if this is a good idea as it seems to imply safety that isn't real either.

[Andarist]

That is true and I pondered about it - this could be potentially rewritten further (although I'm not up to it as I don't really care about this ;p). I just feel that this proposed structure is less surprising to the reader - not necessarily when it comes to its behavior but to the code structure itself. And since the type predicate has to be written at the call site (or referenced), it at least gives a vague notion of this function being on the edge of safety.

[jakebailey]

Thinking about it some more, I think we could safely change this function to be the following:

    function getPropFromRaw<T>(prop: "files" | "include" | "exclude" | "references", validateElement: (value: unknown) => value is T, elementTypeName: string): PropOfRaw<T> {
        if (hasProperty(raw, prop) && !isNullOrUndefined(raw[prop])) {
            const value = raw[prop];
            if (isArray(value)) {
                const result = filter(value, validateElement);
                if (!sourceFile && result.length !== value.length) {
                    errors.push(createCompilerDiagnostic(Diagnostics.Compiler_option_0_requires_a_value_of_type_1, prop, elementTypeName));
                }
                return result;
            }
            else {
                createCompilerDiagnosticOnlyIfJson(Diagnostics.Compiler_option_0_requires_a_value_of_type_1, prop, "Array");
                return "not-array";
            }
        }
        return "no-prop";
    }

So, we'll always get back a list of T; I think this is okay because it's extremely likely that the "bad" things would cause some sort of crash.

[Andarist]

Applied the suggested change and added !!element && to predicates testing for ProjectReference (they were just checking typeof element === 'object'). The consuming code validates further typeof ref.path === 'string' (although probably a little bit late since at this point in time the type already should/could be "trusted", not a big deal for this though) but originalPath, prepend, circular are passed through as-is to the caller so I wouldn't be surprised if that could cause some issues. Perhaps those should get normalized to default values if they have invalid values (extra validation messages would also be nice)?

[jakebailey]

Applied the suggested change and added !!element &&

I don't know if that's strictly required; the function below calls !isNullOrUndefined(raw[prop]), right?

Perhaps those should get normalized to default values if they have invalid values (extra validation messages would also be nice)?

Yeah, though that'd require some tests I think.

[Andarist]

I don't know if that's strictly required; the function below calls !isNullOrUndefined(raw[prop]), right?

If you refer to the check within getPropFromRaw then this only happens for raw[prop] but validateElement operates on typeof raw[prop][number]

[Andarist]

It turns out that the added filter here was at fault for the broken tests on main. Everything passes with this patch:

commit cddebbddcfb2a2fa9ea4511771e9376812012326
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Mon Mar 20 10:33:16 2023 +0100

    Do not filter out invalid results before returning them to the caller

diff --git a/src/compiler/commandLineParser.ts b/src/compiler/commandLineParser.ts
index 0ceebb399c..fac895281c 100644
--- a/src/compiler/commandLineParser.ts
+++ b/src/compiler/commandLineParser.ts
@@ -29,6 +29,7 @@ import {
     emptyArray,
     endsWith,
     ensureTrailingDirectorySeparator,
+    every,
     Expression,
     extend,
     Extension,
@@ -3080,8 +3081,8 @@ function parseJsonConfigFileContentWorker(
         if (hasProperty(raw, prop) && !isNullOrUndefined(raw[prop])) {
             const value = raw[prop];
             if (isArray(value)) {
-                const result = filter(value, validateElement);
-                if (!sourceFile && result.length !== value.length) {
+                const result = raw[prop] as T[];
+                if (!sourceFile && !every(result, validateElement)) {
                     errors.push(createCompilerDiagnostic(Diagnostics.Compiler_option_0_requires_a_value_of_type_1, prop, elementTypeName));
                 }
                 return result;

Do you want me to create a new PR with this change? I honestly don't care about it that much... but since I already have written this code it doesn't cost me much. The original motivation behind this PR was mostly "stylistic" since the old structure was a little bit confusing - it doesn't allow for a simple refactor like:

diff --git a/src/compiler/commandLineParser.ts b/src/compiler/commandLineParser.ts
index ab3909d8e2..d115281707 100644
--- a/src/compiler/commandLineParser.ts
+++ b/src/compiler/commandLineParser.ts
@@ -3074,7 +3074,8 @@ function parseJsonConfigFileContentWorker(
     }
 
     function getSpecsFromRaw(prop: "files" | "include" | "exclude"): PropOfRaw<string> {
-        return getPropFromRaw(prop, isString, "string");
+        const fromRaw = getPropFromRaw(prop, isString, "string");
+        return fromRaw;
     }
 
     function getPropFromRaw<T>(prop: "files" | "include" | "exclude" | "references", validateElement: (value: unknown) => boolean, elementTypeName: string): PropOfRaw<T> {

All in all, it doesn't really matter in the grand scheme of things - so it's up to you, just let me know what I should do.

[jakebailey]

The filter was the only reason this code became typesafe, so, I'm not sure about removing it if we can figure out why that broke the tests; was it even a break, or just a differing error?

[Andarist]

This doesn't make the code any safer than it currently is but I feel like it surfaces the "unsafe" part (the predicate function) to the reader and that code ends up being less surprising (previously it relied on the explicit type parameter and the contextual type provided through the return annotation: TS playground)

[jakebailey]

Uh oh, baseline difference: https://github.com/microsoft/TypeScript/actions/runs/4464265067/jobs/7840254168

[jakebailey]

I did a quick revert so main isn't broken. Oops. Weekend brain...