Fix JSON.stringify return type (#18879), reduce usage of any in JSON.parse #50242

clarfonthey · 2022-08-09T23:06:45Z

This reflects the types of the actual JSON.stringify function, which may return string | undefined. This approach chooses to add extra overloads specific to the JSON-valid types (specifically boolean | number | string | object) to allow the caller to ensure that the value is definitely a string. To make these overloads effective, the input that may potentially return undefined is marked with unknown, not any.

This also modifies the replacer to JSON.parse to follow the same format as the new replace for stringify, which now accepts unknown instead of any. To minimise the cascading of changes made, however, parse still returns any instead of unknown.

… in JSON.parse

ghost · 2022-08-09T23:06:56Z

All CLA requirements met.

clarfonthey · 2022-08-10T23:16:57Z

I'm going to try one more time, but I genuinely can't reproduce the errors happening on CI, locally. I rebased on main multiple times and ran multiple variations of the test commands, including literally copying the same stuff CI does, and it just did not result in the same errors locally. Everything passes on my end. node --version is 18.7.0 and it doesn't matter whether I do a regular npm install or npm ci; it just doesn't work.

src/lib/es5.d.ts

clarfonthey · 2022-08-17T06:12:34Z

Note to any TypeScript maintainers reading this: the fact that baselines hard-code the line and column of items in the library source code makes this very difficult to properly maintain. Given how changes seem to be merged daily that change these files, this makes library improvements additionally painful, even if it's for a very important change.

I don't blame the actual team members responsible for reviewing PRs, but would like to specifically call out Microsoft, a several-billion-dollar company, for not allocating the resources to the TS team to adequately review PRs to what has become a critical piece of infrastructure for many companies. This is not a volunteer project, but something being developed by a for-profit corporation with control of much of the Node ecosystem.

I'm willing to continue to put up with merging in the main branch for another week or so, but beyond that, I'm going to just give up and face the fact that this isn't going to be merged. I'm almost certain this is the reason why this bug, which has had multiple fix PRs opened since it was initially reported, has never been fixed-- the project changes rapidly, is fragile to these sorts of changes, and there's inadequate staff allocated to actually helping contributors merge changes in a manner that reflects that.

Doubt anyone's actually going to be able to take action on this, but figured I'd post a comment for those burned by this bug to read and sympathise with. Hopefully, this doesn't matter too much and this actually does get merged.

RyanCavanaugh · 2022-08-17T21:20:31Z

The CI runs a second pass to ensure that TypeScript can still build itself after any proposed change. You introduced build errors into shims.ts, as shown on the PR page itself:

You can reproduce this locally by running

gulp lkg
gulp local

I haven't seen a PR break the build in this particular way before, but we should probably update the CONTRIBUTING.md instructions to address this edge case.

…ange was merged that conflicts with your change

clarfonthey · 2022-08-17T21:29:12Z

So I should add, this particular case came up after the latest change, but before I was getting other baseline diffs showing up that I could not actually get to generate locally, no matter how hard I tried; it just said that the files were generated. This change is actually very clear in the diffs that run in CI.

I'll see if I can add these steps to CONTRIBUTING.md. Thank you for helping out. ❤️

For the specific build I'm referencing: https://github.com/microsoft/TypeScript/runs/7757158610

…best workaround I know to make this work. I'm continuing out of sheer spite. This isn't even done yet.

clarfonthey · 2022-08-18T05:16:12Z

src/lib/es5.d.ts

+    T extends object & { then(onfulfilled: infer F, ...args: infer _): any } ? // `await` only unwraps object types with a callable `then`. Non-object types are not unwrapped
+    F extends ((value: infer V, ...args: infer _) => any) ? // if the argument to `then` is callable, extracts the first argument
+    Awaited<V> : // recursively unwrap the value
+    never : // the argument to `then` was not callable
+    T; // non-object or non-thenable


This is how typescript-language-server auto-formats this file -- I don't prefer it, but the best way I have at the moment to make the changes not look horrendous as I'm modifying them is to format-on-save and then run eslint --fix.

I would recommend disabling formatting altogether on this repo; we don't use any particular style besides the lines, so it's just not a good time for those who use format on save (which I normally do, except here). I'm hoping that can change one day, but checker is just a huge beast.

clarfonthey · 2022-08-18T17:53:24Z

So I don't understand why CI is passing now -- I didn't finish fixing all the errors last night and they're still showing up for me locally, so, I'm planning to do more work later on it.

jakebailey · 2022-08-19T07:21:47Z

src/harness/harnessUtils.ts

@@ -148,7 +148,7 @@ namespace Utils {
        }
    }

-    function isNodeOrArray(a: any): boolean {
+    function isNodeOrArray(a: any): a is ts.Node {


Is this accurate? I'd think this should mention NodeArray, which is not a Node.

Will double-check.

jakebailey · 2022-08-19T07:22:59Z

src/testRunner/unittests/tsbuildWatch/projectsBuilding.ts

@@ -11,29 +11,31 @@ namespace ts.tscWatch {
            return { path: `./pkg${index}` };
        }
        function pkgFiles(index: number): File[] {
+            const tsconfig = {
+                compilerOptions: { composite: true },


Were these changes necessary to make the code compile?

is it because of contextual typing? or maybe object literal freshness?

sandersn

this parameter type seems wrong
I am curious about the number of objects that had to be extracted to a variable.
I would like the whitespace changes to be reverted.

sandersn · 2022-09-01T23:10:56Z

src/harness/fourslashImpl.ts

@@ -1341,7 +1341,7 @@ namespace FourSlash {
            }));
        }

-        public verifyQuickInfoAt(markerName: string | Range, expectedText: string, expectedDocumentation?: string, expectedTags?: {name: string; text: string;}[]) {
+        public verifyQuickInfoAt(markerName: string | Range, expectedText: string, expectedDocumentation?: string, expectedTags?: { name: string; text: string; }[]) {


please undo these formatting changes. We'll create a giant formatting PR when we (someday, maybe) start using a formatter.

Okay. Keep in mind that this is just the formatting the langserver did automatically, and not a special formatter.

In the final PR I can remove it, I was just tired of constantly removing it when it was done automatically, when my main concern was getting tests to pass.

sandersn · 2022-09-01T23:11:48Z

src/harness/fourslashImpl.ts

-    function stringify(data: any, replacer?: (key: string, value: any) => any): string {
-        return JSON.stringify(data, replacer, 2);
+    function stringify(data: unknown, replacer?: (key: string, value: unknown) => unknown): string {
+        return JSON.stringify(data, replacer, 2) ?? "undefined";


this would have been equivalent to ... ?? "" before, right? Do any baselines change?

Actually, the previous code would have returned undefined (not string), which is why I was conservative and returned the string "undefined" since that's what would happen when it's cast to a string.

Essentially: the code would have either crashed at runtime or returned that string, and it's why I was conservative and chose it.

sandersn · 2022-09-01T23:27:53Z

src/lib/es5.d.ts

+     * @param replacer A function that transforms the results.
+     * @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
+     */
+    stringify(value: unknown, replacer?: (this: unknown, key: string, value: unknown) => unknown, space?: string | number): string | undefined;


this: unknown requires this to be unknown or a supertype of unknown if it's specified. That doesn't sound right to me. What types are supposed to allowed and disallowed for this here?

this is always set to the value being replaced, which doesn't have any distinct type. It could be a symbol or function or some other value added to the standard in the distant future, which is why I was conservative.

When you say "supertype of unknown"... I thought that unknown was the maximal type?

sandersn · 2022-09-01T23:43:11Z

src/testRunner/unittests/tsbuildWatch/projectsBuilding.ts

@@ -11,29 +11,31 @@ namespace ts.tscWatch {
            return { path: `./pkg${index}` };
        }
        function pkgFiles(index: number): File[] {
+            const tsconfig = {
+                compilerOptions: { composite: true },


is it because of contextual typing? or maybe object literal freshness?

clarfonthey · 2022-09-02T12:27:39Z

I've been mostly unable to work on this this week due to real-life moving hell, but was planning to continue this either next week or the week after.

In terms of the object literal extraction, to my understanding, it's because the type checking literally differs between lvalues and rvalues, or literals and bindings, or whatever pair of terms you want to use. It prefers to treat the value as unknown rather than directly inferring object.

I chose specifically object instead of some other type like a Record since tsc infers this directly when checking typeof, and it's the type that most closely matches the spec. But it's unhappy using this type when directly comparing object literals for some unknown reason. If you can figure out the cause of this, I'm fine undoing my changes.

I was testing via npx gulp lkg && npx gulp tests; I didn't want to run in parallel since I wanted to do other things while it was running.

clarfonthey · 2022-12-03T03:09:38Z

Yeah I, lost all desire to work on this, so, I'm just going to close this PR so people don't think I'm going to try this again.

Fix JSON.stringify return type (microsoft#18879), reduce usage of any…

dd2f40e

… in JSON.parse

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 9, 2022

clarfonthey added 3 commits August 9, 2022 19:21

Forgot a baseline

ba28be6

Merge remote-tracking branch 'u/main' into stringify-is-wrong

1022de1

Merge remote-tracking branch 'u/main' into stringify-is-wrong

46fdcfd

Josh-Cena reviewed Aug 13, 2022

View reviewed changes

src/lib/es5.d.ts Outdated Show resolved Hide resolved

clarfonthey added 2 commits August 17, 2022 01:46

JSON value => JSON-representable value

03e442b

Merge remote-tracking branch 'u/main' into stringify-is-wrong

e763780

The irony of having to fix a bug that your change solves because a ch…

e3428ad

…ange was merged that conflicts with your change

clarfonthey added 3 commits August 17, 2022 18:46

Use exclude for override, update CONTRIBUTING

2642797

Actually, exclude doesn't make any difference

6d29511

Expressions are typed differently than declarations, and this is the …

c5af390

…best workaround I know to make this work. I'm continuing out of sheer spite. This isn't even done yet.

clarfonthey commented Aug 18, 2022

View reviewed changes

jakebailey reviewed Aug 19, 2022

View reviewed changes

sandersn requested changes Sep 1, 2022

View reviewed changes

sandersn self-assigned this Sep 1, 2022

clarfonthey closed this Dec 3, 2022

clarfonthey deleted the stringify-is-wrong branch December 3, 2022 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix JSON.stringify return type (#18879), reduce usage of any in JSON.parse #50242

Fix JSON.stringify return type (#18879), reduce usage of any in JSON.parse #50242

clarfonthey commented Aug 9, 2022

ghost commented Aug 9, 2022 •

edited by ghost

Loading

clarfonthey commented Aug 10, 2022

clarfonthey commented Aug 17, 2022

RyanCavanaugh commented Aug 17, 2022

clarfonthey commented Aug 17, 2022 •

edited

Loading

clarfonthey Aug 18, 2022

jakebailey Aug 19, 2022

clarfonthey commented Aug 18, 2022

jakebailey Aug 19, 2022

clarfonthey Aug 20, 2022

jakebailey Aug 19, 2022

clarfonthey Aug 20, 2022

sandersn Sep 1, 2022

sandersn left a comment

sandersn Sep 1, 2022

clarfonthey Sep 2, 2022

clarfonthey Sep 2, 2022

sandersn Sep 1, 2022

clarfonthey Sep 2, 2022

clarfonthey Sep 2, 2022

sandersn Sep 1, 2022

clarfonthey Sep 2, 2022

sandersn Sep 1, 2022

clarfonthey commented Sep 2, 2022 •

edited

Loading

clarfonthey commented Dec 3, 2022

Fix JSON.stringify return type (#18879), reduce usage of any in JSON.parse #50242

Fix JSON.stringify return type (#18879), reduce usage of any in JSON.parse #50242

Conversation

clarfonthey commented Aug 9, 2022

ghost commented Aug 9, 2022 • edited by ghost Loading

clarfonthey commented Aug 10, 2022

clarfonthey commented Aug 17, 2022

RyanCavanaugh commented Aug 17, 2022

clarfonthey commented Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey commented Aug 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey commented Sep 2, 2022 • edited Loading

clarfonthey commented Dec 3, 2022

ghost commented Aug 9, 2022 •

edited by ghost

Loading

clarfonthey commented Aug 17, 2022 •

edited

Loading

clarfonthey commented Sep 2, 2022 •

edited

Loading