Skip to content

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

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.

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

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ gulp

The files in `lib/` are used to bootstrap compilation and usually **should not** be updated unless publishing a new version or updating the LKG.

NOTE: Although LKG isn't updated on every change, CI will still verify that changes work if LKG is regenerated. So, you may want to run `gulp lkg` and `gulp local` as part of testing to uncover potential bugs.

### Modifying generated library files

The files `src/lib/dom.generated.d.ts` and `src/lib/webworker.generated.d.ts` both represent type declarations for the DOM and are auto-generated. To make any modifications to them, you will have to direct changes to https://github.com/Microsoft/TSJS-lib-generator
Expand Down
40 changes: 20 additions & 20 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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; }[]) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

if (typeof markerName === "string") this.goToMarker(markerName);
else this.goToRangeStart(markerName);

Expand Down Expand Up @@ -2622,7 +2622,7 @@ namespace FourSlash {
Harness.IO.log(this.spanInfoToString(this.getNameOrDottedNameSpan(pos)!, "**"));
}

private classificationToIdentifier(classification: number){
private classificationToIdentifier(classification: number) {

const tokenTypes: string[] = [];
tokenTypes[ts.classifier.v2020.TokenType.class] = "class";
Expand Down Expand Up @@ -2664,7 +2664,7 @@ namespace FourSlash {
return [tokenTypes[typeIdx], ...tokenModifiers.filter((_, i) => modSet & 1 << i)].join(".");
}

private verifyClassifications(expected: { classificationType: string | number, text?: string; textSpan?: TextSpan }[], actual: (ts.ClassifiedSpan | ts.ClassifiedSpan2020)[] , sourceFileText: string) {
private verifyClassifications(expected: { classificationType: string | number, text?: string; textSpan?: TextSpan }[], actual: (ts.ClassifiedSpan | ts.ClassifiedSpan2020)[], sourceFileText: string) {
if (actual.length !== expected.length) {
this.raiseError("verifyClassifications failed - expected total classifications to be " + expected.length +
", but was " + actual.length +
Expand All @@ -2673,7 +2673,7 @@ namespace FourSlash {

ts.zipWith(expected, actual, (expectedClassification, actualClassification) => {
const expectedType = expectedClassification.classificationType;
const actualType = typeof actualClassification.classificationType === "number" ? this.classificationToIdentifier(actualClassification.classificationType) : actualClassification.classificationType;
const actualType = typeof actualClassification.classificationType === "number" ? this.classificationToIdentifier(actualClassification.classificationType) : actualClassification.classificationType;

if (expectedType !== actualType) {
this.raiseError("verifyClassifications failed - expected classifications type to be " +
Expand Down Expand Up @@ -2732,7 +2732,7 @@ namespace FourSlash {
public replaceWithSemanticClassifications(format: ts.SemanticClassificationFormat.TwentyTwenty) {
const actual = this.languageService.getSemanticClassifications(this.activeFile.fileName,
ts.createTextSpan(0, this.activeFile.content.length), format);
const replacement = [`const c2 = classification("2020");`,`verify.semanticClassificationsAre("2020",`];
const replacement = [`const c2 = classification("2020");`, `verify.semanticClassificationsAre("2020",`];
for (const a of actual) {
const identifier = this.classificationToIdentifier(a.classificationType as number);
const text = this.activeFile.content.slice(a.textSpan.start, a.textSpan.start + a.textSpan.length);
Expand Down Expand Up @@ -2786,7 +2786,7 @@ namespace FourSlash {

const reverseSpans = allSpanInsets.sort((l, r) => r.pos - l.pos);
ts.forEach(reverseSpans, span => {
annotated = annotated.slice(0, span.pos) + span.text + annotated.slice(span.pos);
annotated = annotated.slice(0, span.pos) + span.text + annotated.slice(span.pos);
});
Harness.IO.log(`\nMockup:\n${annotated}`);
}
Expand Down Expand Up @@ -3088,9 +3088,9 @@ namespace FourSlash {

// Undo changes to perform next fix
const span = change.textChanges[0].span;
const deletedText = originalContent.substr(span.start, change.textChanges[0].span.length);
const insertedText = change.textChanges[0].newText;
this.editScriptAndUpdateMarkers(fileName, span.start, span.start + insertedText.length, deletedText);
const deletedText = originalContent.substr(span.start, change.textChanges[0].span.length);
const insertedText = change.textChanges[0].newText;
this.editScriptAndUpdateMarkers(fileName, span.start, span.start + insertedText.length, deletedText);
}
if (expectedTextArray.length !== actualTextArray.length) {
this.raiseError(`Expected ${expectedTextArray.length} import fixes, got ${actualTextArray.length}:\n\n${actualTextArray.join("\n\n" + "-".repeat(20) + "\n\n")}`);
Expand Down Expand Up @@ -4504,8 +4504,8 @@ namespace FourSlash {
};
}

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";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

}

/** Collects an array of unique outputs. */
Expand Down Expand Up @@ -4575,13 +4575,13 @@ namespace FourSlash {
if (doesAddToIndex) {
closestIndex.length = closestIndex.length + 1;
}
else {
ranges.push({ start: index - 1, length: 1 });
}
}
else {
ranges.push({ start: index - 1, length: 1 });
}
}
else {
ranges.push({ start: index - 1, length: 1 });
}
};

for (let index = 0; index < Math.max(source.length, target.length); index++) {
Expand All @@ -4591,10 +4591,10 @@ namespace FourSlash {
}

return ranges;
}
}

// Adds an _ when the source string and the target string have a whitespace difference
function highlightDifferenceBetweenStrings(source: string, target: string) {
// Adds an _ when the source string and the target string have a whitespace difference
function highlightDifferenceBetweenStrings(source: string, target: string) {
const ranges = rangesOfDiffBetweenTwoStrings(source, target);
let emTarget = target;
ranges.forEach((range, index) => {
Expand All @@ -4606,9 +4606,9 @@ namespace FourSlash {
range.start + 1 + additionalOffset,
range.start + range.length + 1 + additionalOffset
);
const after = emTarget.slice(range.start + range.length + 1 + additionalOffset, emTarget.length);
const after = emTarget.slice(range.start + range.length + 1 + additionalOffset, emTarget.length);
emTarget = before + lhs + between + rhs + after;
});
return emTarget;
}
}
}
2 changes: 1 addition & 1 deletion src/harness/harnessUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ namespace Utils {
}
}

function isNodeOrArray(a: any): boolean {
function isNodeOrArray(a: any): a is ts.Node {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Will double-check.

return a !== undefined && typeof a.pos === "number";
}

Expand Down
40 changes: 27 additions & 13 deletions src/lib/es5.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ interface ObjectConstructor {
* Prevents the modification of existing property attributes and values, and prevents the addition of new properties.
* @param o Object on which to lock the attributes.
*/
freeze<T extends {[idx: string]: U | null | undefined | object}, U extends string | bigint | number | boolean | symbol>(o: T): Readonly<T>;
freeze<T extends { [idx: string]: U | null | undefined | object }, U extends string | bigint | number | boolean | symbol>(o: T): Readonly<T>;

/**
* Prevents the modification of existing property attributes and values, and prevents the addition of new properties.
Expand Down Expand Up @@ -1111,21 +1111,35 @@ interface JSON {
* @param reviver A function that transforms the results. This function is called for each member of the object.
* If a member contains nested objects, the nested objects are transformed before the parent object is.
*/
parse(text: string, reviver?: (this: any, key: string, value: any) => any): any;
parse(text: string, reviver?: (this: unknown, key: string, value: unknown) => unknown): any;
/**
* Converts a JavaScript value to a JavaScript Object Notation (JSON) string.
* @param value A JavaScript value, usually an object or array, to be converted.
* Converts a JSON-representable value to a JavaScript Object Notation (JSON) string.
* @param value A JSON-representable value, usually an object or array, to be converted.
* @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: any, replacer?: (this: any, key: string, value: any) => any, space?: string | number): string;
stringify(value: boolean | number | string | object, replacer?: (this: unknown, key: string, value: unknown) => unknown, space?: string | number): string;
/**
* Converts a JavaScript value to a JavaScript Object Notation (JSON) string.
* @param value A JavaScript value, usually an object or array, to be converted.
* Converts a JSON-representable value to a JavaScript Object Notation (JSON) string.
* @param value A JSON-representable value, usually an object or array, to be converted.
* @param replacer An array of strings and numbers that acts as an approved list for selecting the object properties that will be stringified.
* @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
*/
stringify(value: any, replacer?: (number | string)[] | null, space?: string | number): string;
stringify(value: boolean | number | string | object, replacer?: (number | string)[] | null, space?: string | number): string;
/**
* Converts a JavaScript value to a JavaScript Object Notation (JSON) string or undefined.
* @param value A JavaScript value to be converted, which could potentially be a non-JSON-representable value.
* @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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

/**
* Converts a JavaScript value to a JavaScript Object Notation (JSON) string or undefined.
* @param value A JavaScript value to be converted, which could potentially be a non-JSON-representable value.
* @param replacer An array of strings and numbers that acts as an approved list for selecting the object properties that will be stringified.
* @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?: (number | string)[] | null, space?: string | number): string | undefined;
}

/**
Expand Down Expand Up @@ -1520,11 +1534,11 @@ interface Promise<T> {
*/
type Awaited<T> =
T extends null | undefined ? T : // special case for `null | undefined` when not in `--strictNullChecks` mode
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
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
Comment on lines +1537 to +1541
Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


interface ArrayLike<T> {
readonly length: number;
Expand Down
11 changes: 7 additions & 4 deletions src/services/shims.ts
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ namespace ts {
this.shimHost.useCaseSensitiveFileNames!(), this.shimHost.getCurrentDirectory()); // TODO: GH#18217
return JSON.parse(this.shimHost.readDirectory(
path,
JSON.stringify(extensions),
JSON.stringify(extensions ?? []),
JSON.stringify(pattern.basePaths),
pattern.excludePattern,
pattern.includeFilePattern,
Expand Down Expand Up @@ -585,15 +585,18 @@ namespace ts {
function forwardCall<T>(logger: Logger, actionDescription: string, returnJson: boolean, action: () => T, logPerformance: boolean): T | string {
try {
const result = simpleForwardCall(logger, actionDescription, action, logPerformance);
return returnJson ? JSON.stringify({ result }) : result as T;
const resultObj = { result };
return returnJson ? JSON.stringify(resultObj) : result as T;
}
catch (err) {
if (err instanceof OperationCanceledException) {
return JSON.stringify({ canceled: true });
const canceledObj = { canceled: true };
return JSON.stringify(canceledObj);
}
logInternalError(logger, err);
err.description = actionDescription;
return JSON.stringify({ error: err });
const errorObj = { error: err };
return JSON.stringify(errorObj);
}
}

Expand Down
23 changes: 12 additions & 11 deletions src/testRunner/unittests/config/convertCompilerOptionsFromJson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ namespace ts {
const { options: actualCompilerOptions, errors: actualErrors } = convertCompilerOptionsFromJson(json.compilerOptions, "/apath/", configFileName);

const parsedCompilerOptions = JSON.stringify(actualCompilerOptions);
const expectedCompilerOptions = JSON.stringify({ ...expectedResult.compilerOptions, configFilePath: configFileName });
const expectedCompilerOptionsObj = { ...expectedResult.compilerOptions, configFilePath: configFileName };
const expectedCompilerOptions = JSON.stringify(expectedCompilerOptionsObj);
assert.equal(parsedCompilerOptions, expectedCompilerOptions);

verifyErrors(actualErrors, expectedResult.errors, /*ignoreLocation*/ true);
}

function assertCompilerOptionsWithJsonNode(json: any, configFileName: string, expectedResult: ExpectedResultWithParsingSuccess) {
assertCompilerOptionsWithJsonText(JSON.stringify(json), configFileName, expectedResult);
assertCompilerOptionsWithJsonText(JSON.stringify(json as object), configFileName, expectedResult);
}

function assertCompilerOptionsWithJsonText(fileText: string, configFileName: string, expectedResult: ExpectedResult) {
Expand Down Expand Up @@ -657,15 +658,15 @@ namespace ts {
}
}
`,
"tsconfig.json",
{
compilerOptions: {
target: undefined,
module: ModuleKind.ESNext,
experimentalDecorators: true,
},
hasParseErrors: true
});
"tsconfig.json",
{
compilerOptions: {
target: undefined,
module: ModuleKind.ESNext,
experimentalDecorators: true,
},
hasParseErrors: true
});
});

it("Convert a tsconfig file with stray trailing characters", () => {
Expand Down
27 changes: 15 additions & 12 deletions src/testRunner/unittests/tsbuildWatch/programUpdates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,12 +182,13 @@ export class someClass2 { }`),
"with circular project reference",
() => {
const [coreTsconfig, ...otherCoreFiles] = core;
const circularCoreObj = {
compilerOptions: { composite: true, declaration: true },
references: [{ path: "../tests", circular: true }]
};
const circularCoreConfig: File = {
path: coreTsconfig.path,
content: JSON.stringify({
compilerOptions: { composite: true, declaration: true },
references: [{ path: "../tests", circular: true }]
})
content: JSON.stringify(circularCoreObj)
};
return [libFile, circularCoreConfig, ...otherCoreFiles, ...logic, ...tests];
}
Expand Down Expand Up @@ -240,18 +241,20 @@ export class someClass2 { }`),
subScenario: "when referenced using prepend builds referencing project even for non local change",
commandLineArgs: ["-b", "-w", `sample1/${SubProject.logic}`],
sys: () => {
const coreTsConfigObj = {
compilerOptions: { composite: true, declaration: true, outFile: "index.js" }
};
const coreTsConfig: File = {
path: core[0].path,
content: JSON.stringify({
compilerOptions: { composite: true, declaration: true, outFile: "index.js" }
})
content: JSON.stringify(coreTsConfigObj)
};
const logicTsConfigObj = {
compilerOptions: { composite: true, declaration: true, outFile: "index.js" },
references: [{ path: "../core", prepend: true }]
};
const logicTsConfig: File = {
path: logic[0].path,
content: JSON.stringify({
compilerOptions: { composite: true, declaration: true, outFile: "index.js" },
references: [{ path: "../core", prepend: true }]
})
content: JSON.stringify(logicTsConfigObj)
};
const logicIndex: File = {
path: logic[1].path,
Expand Down Expand Up @@ -743,4 +746,4 @@ export function someFn() { }`),
changes: emptyArray
});
});
}
}
Loading