Skip to content

Rewrite getPropFromRaw slightly to surface T in the parameters #53301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 20, 2023
Merged

Rewrite getPropFromRaw slightly to surface T in the parameters #53301

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdd47ef
Rewrite getPropFromRaw slightly to surface T in the parameters
Andarist Mar 16, 2023
29e6c48
Make getPropFromRaw even safer
Andarist Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/compiler/commandLineParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
emptyArray,
endsWith,
ensureTrailingDirectorySeparator,
every,
Expression,
extend,
Extension,
Expand Down Expand Up @@ -2974,7 +2973,7 @@ function parseJsonConfigFileContentWorker(
};

function getConfigFileSpecs(): ConfigFileSpecs {
const referencesOfRaw = getPropFromRaw<ProjectReference>("references", element => typeof element === "object", "object");
const referencesOfRaw = getPropFromRaw("references", (element): element is ProjectReference => !!element && typeof element === "object", "object");
const filesSpecs = toPropValue(getSpecsFromRaw("files"));
if (filesSpecs) {
const hasZeroOrNoReferences = referencesOfRaw === "no-prop" || isArray(referencesOfRaw) && referencesOfRaw.length === 0;
Expand Down Expand Up @@ -3049,7 +3048,7 @@ function parseJsonConfigFileContentWorker(

function getProjectReferences(basePath: string): readonly ProjectReference[] | undefined {
let projectReferences: ProjectReference[] | undefined;
const referencesOfRaw = getPropFromRaw<ProjectReference>("references", element => typeof element === "object", "object");
const referencesOfRaw = getPropFromRaw("references", (element): element is ProjectReference => !!element && typeof element === "object", "object");
if (isArray(referencesOfRaw)) {
for (const ref of referencesOfRaw) {
if (typeof ref.path !== "string") {
Expand Down Expand Up @@ -3077,11 +3076,12 @@ function parseJsonConfigFileContentWorker(
return getPropFromRaw(prop, isString, "string");
}

function getPropFromRaw<T>(prop: "files" | "include" | "exclude" | "references", validateElement: (value: unknown) => boolean, elementTypeName: string): PropOfRaw<T> {
function getPropFromRaw<T>(prop: "files" | "include" | "exclude" | "references", validateElement: (value: unknown) => value is T, elementTypeName: string): PropOfRaw<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (hasProperty(raw, prop) && !isNullOrUndefined(raw[prop])) {
if (isArray(raw[prop])) {
const result = raw[prop] as T[];
if (!sourceFile && !every(result, validateElement)) {
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;
Expand Down