Skip to content

Fix super method call chain, remove ? from method, improve vary-by #35013

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

Merged
merged 2 commits into from
Nov 19, 2019
Merged
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
85 changes: 80 additions & 5 deletions src/harness/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1412,10 +1412,65 @@ namespace Harness {
[key: string]: string;
}

function splitVaryBySettingValue(text: string): string[] | undefined {
function splitVaryBySettingValue(text: string, varyBy: string): string[] | undefined {
if (!text) return undefined;
const entries = text.split(/,/).map(s => s.trim().toLowerCase()).filter(s => s.length > 0);
return entries && entries.length > 1 ? entries : undefined;

let star = false;
const includes: string[] = [];
const excludes: string[] = [];
for (let s of text.split(/,/g)) {
s = s.trim().toLowerCase();
if (s.length === 0) continue;
if (s === "*") {
star = true;
}
else if (ts.startsWith(s, "-") || ts.startsWith(s, "!")) {
excludes.push(s.slice(1));
}
else {
includes.push(s);
}
}

// do nothing if the setting has no variations
if (includes.length <= 1 && !star && excludes.length === 0) {
return undefined;
}

const variations: { key: string, value?: string | number }[] = [];
const values = getVaryByStarSettingValues(varyBy);

// add (and deduplicate) all included entries
for (const include of includes) {
const value = values?.get(include);
if (ts.findIndex(variations, v => v.key === include || value !== undefined && v.value === value) === -1) {
variations.push({ key: include, value });
}
}

if (star && values) {
// add all entries
for (const [key, value] of ts.arrayFrom(values.entries())) {
if (ts.findIndex(variations, v => v.key === key || v.value === value) === -1) {
variations.push({ key, value });
}
}
}

// remove all excluded entries
for (const exclude of excludes) {
const value = values?.get(exclude);
let index: number;
while ((index = ts.findIndex(variations, v => v.key === exclude || value !== undefined && v.value === value)) >= 0) {
ts.orderedRemoveItemAt(variations, index);
}
}

if (variations.length === 0) {
throw new Error(`Variations in test option '@${varyBy}' resulted in an empty set.`);
}

return ts.map(variations, v => v.key);
}

function computeFileBasedTestConfigurationVariations(configurations: FileBasedTestConfiguration[], variationState: FileBasedTestConfiguration, varyByEntries: [string, string[]][], offset: number) {
Expand All @@ -1433,17 +1488,37 @@ namespace Harness {
}
}

let booleanVaryByStarSettingValues: ts.Map<string | number> | undefined;

function getVaryByStarSettingValues(varyBy: string): ts.ReadonlyMap<string | number> | undefined {
const option = ts.forEach(ts.optionDeclarations, decl => ts.equateStringsCaseInsensitive(decl.name, varyBy) ? decl : undefined);
if (option) {
if (typeof option.type === "object") {
return option.type;
}
if (option.type === "boolean") {
return booleanVaryByStarSettingValues || (booleanVaryByStarSettingValues = ts.createMapFromTemplate({
true: 1,
false: 0
}));
}
}
}

/**
* Compute FileBasedTestConfiguration variations based on a supplied list of variable settings.
*/
export function getFileBasedTestConfigurations(settings: TestCaseParser.CompilerSettings, varyBy: string[]): FileBasedTestConfiguration[] | undefined {
export function getFileBasedTestConfigurations(settings: TestCaseParser.CompilerSettings, varyBy: readonly string[]): FileBasedTestConfiguration[] | undefined {
let varyByEntries: [string, string[]][] | undefined;
let variationCount = 1;
for (const varyByKey of varyBy) {
if (ts.hasProperty(settings, varyByKey)) {
// we only consider variations when there are 2 or more variable entries.
const entries = splitVaryBySettingValue(settings[varyByKey]);
const entries = splitVaryBySettingValue(settings[varyByKey], varyByKey);
if (entries) {
if (!varyByEntries) varyByEntries = [];
variationCount *= entries.length;
if (variationCount > 25) throw new Error(`Provided test options exceeded the maximum number of variations: ${varyBy.map(v => `'@${v}'`).join(", ")}`);
varyByEntries.push([varyByKey, entries]);
}
}
Expand Down
26 changes: 25 additions & 1 deletion src/testRunner/compilerRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,30 @@ class CompilerBaselineRunner extends RunnerBase {
}

class CompilerTest {
private static varyBy: readonly string[] = [
"module",
"target",
"jsx",
"removeComments",
"importHelpers",
"importHelpers",
"downlevelIteration",
"isolatedModules",
"strict",
"noImplicitAny",
"strictNullChecks",
"strictFunctionTypes",
"strictBindCallApply",
"strictPropertyInitialization",
"noImplicitThis",
"alwaysStrict",
"allowSyntheticDefaultImports",
"esModuleInterop",
"emitDecoratorMetadata",
"skipDefaultLibCheck",
"preserveConstEnums",
"skipLibCheck",
];
private fileName: string;
private justName: string;
private configuredName: string;
Expand Down Expand Up @@ -220,7 +244,7 @@ class CompilerTest {
// also see `parseCompilerTestConfigurations` in tests/webTestServer.ts
const content = Harness.IO.readFile(file)!;
const settings = Harness.TestCaseParser.extractCompilerSettings(content);
const configurations = Harness.getFileBasedTestConfigurations(settings, /*varyBy*/ ["module", "target"]);
const configurations = Harness.getFileBasedTestConfigurations(settings, CompilerTest.varyBy);
return { file, configurations, content };
}

Expand Down
18 changes: 18 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=es2016).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
// GH#34952
class Base {
method() { }
}
class Derived extends Base {
method1() { var _a; return (_a = super.method) === null || _a === void 0 ? void 0 : _a.call(this); }
method2() { var _a; return (_a = super["method"]) === null || _a === void 0 ? void 0 : _a.call(this); }
}
18 changes: 18 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=es2017).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
// GH#34952
class Base {
method() { }
}
class Derived extends Base {
method1() { var _a; return (_a = super.method) === null || _a === void 0 ? void 0 : _a.call(this); }
method2() { var _a; return (_a = super["method"]) === null || _a === void 0 ? void 0 : _a.call(this); }
}
18 changes: 18 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=es2018).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
// GH#34952
class Base {
method() { }
}
class Derived extends Base {
method1() { var _a; return (_a = super.method) === null || _a === void 0 ? void 0 : _a.call(this); }
method2() { var _a; return (_a = super["method"]) === null || _a === void 0 ? void 0 : _a.call(this); }
}
18 changes: 18 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=es2019).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
// GH#34952
class Base {
method() { }
}
class Derived extends Base {
method1() { var _a; return (_a = super.method) === null || _a === void 0 ? void 0 : _a.call(this); }
method2() { var _a; return (_a = super["method"]) === null || _a === void 0 ? void 0 : _a.call(this); }
}
18 changes: 18 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=es2020).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
// GH#34952
class Base {
method() { }
}
class Derived extends Base {
method1() { var _a; return (_a = super.method) === null || _a === void 0 ? void 0 : _a.call(this); }
method2() { var _a; return (_a = super["method"]) === null || _a === void 0 ? void 0 : _a.call(this); }
}
39 changes: 39 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=es5).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
var __extends = (this && this.__extends) || (function () {
var extendStatics = function (d, b) {
extendStatics = Object.setPrototypeOf ||
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
return extendStatics(d, b);
};
return function (d, b) {
extendStatics(d, b);
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
})();
// GH#34952
var Base = /** @class */ (function () {
function Base() {
}
Base.prototype.method = function () { };
return Base;
}());
var Derived = /** @class */ (function (_super) {
__extends(Derived, _super);
function Derived() {
return _super !== null && _super.apply(this, arguments) || this;
}
Derived.prototype.method1 = function () { var _a; return (_a = _super.prototype.method) === null || _a === void 0 ? void 0 : _a.call(this); };
Derived.prototype.method2 = function () { var _a; return (_a = _super.prototype["method"]) === null || _a === void 0 ? void 0 : _a.call(this); };
return Derived;
}(Base));
18 changes: 18 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=es6).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
// GH#34952
class Base {
method() { }
}
class Derived extends Base {
method1() { var _a; return (_a = super.method) === null || _a === void 0 ? void 0 : _a.call(this); }
method2() { var _a; return (_a = super["method"]) === null || _a === void 0 ? void 0 : _a.call(this); }
}
18 changes: 18 additions & 0 deletions tests/baselines/reference/callChainWithSuper(target=esnext).js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [callChainWithSuper.ts]
// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}

//// [callChainWithSuper.js]
"use strict";
// GH#34952
class Base {
method() { }
}
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//// [optionalMethodDeclarations.ts]
// https://github.com/microsoft/TypeScript/issues/34952#issuecomment-552025027
class C {
// ? should be removed in emit
method?() {}
}

//// [optionalMethodDeclarations.js]
// https://github.com/microsoft/TypeScript/issues/34952#issuecomment-552025027
class C {
// ? should be removed in emit
method() { }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//// [optionalMethodDeclarations.ts]
// https://github.com/microsoft/TypeScript/issues/34952#issuecomment-552025027
class C {
// ? should be removed in emit
method?() {}
}

//// [optionalMethodDeclarations.js]
// https://github.com/microsoft/TypeScript/issues/34952#issuecomment-552025027
class C {
// ? should be removed in emit
method() { }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// @target: esnext,es2016
Copy link
Member

Choose a reason for hiding this comment

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

isn't the ES5 emit interesting here too? I assume we could be putting a ? there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ES5 emit changes the entire declaration to a function expression, so it wouldn't be caught by this change.

// @noTypesAndSymbols: true

// https://github.com/microsoft/TypeScript/issues/34952#issuecomment-552025027
class C {
// ? should be removed in emit
method?() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// @target: *,-es3
// @strict: true
// @noTypesAndSymbols: true

// GH#34952
class Base { method?() {} }
class Derived extends Base {
method1() { return super.method?.(); }
method2() { return super["method"]?.(); }
}