Skip to content
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

Support strict mode #505

Merged
merged 4 commits into from
Aug 15, 2018
Merged

Support strict mode #505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8e203b
Support strict mode
kitsonk Aug 10, 2018
2a7e551
Lint fixes and format
kitsonk Aug 10, 2018
ca5a94b
Workound issue with TypeScriptHost.resolveModuleNames
kitsonk Aug 12, 2018
b0ef38e
Properly assert in readFileSync in js/os.ts
kitsonk Aug 14, 2018
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
4 changes: 3 additions & 1 deletion js/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,7 @@ export default function denoMain() {

const inputFn = argv[1];
const mod = runtime.resolveModule(inputFn, `${cwd}/`);
mod.compileAndRun();
assert(mod != null);
// TypeScript does not track assert, therefore not null assertion
mod!.compileAndRun();
Copy link
Member

Choose a reason for hiding this comment

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

These null assert checks are redundant when using the bang operator. Suggest removing the assert.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ry @kitsonk FYI, you can avoid the bang if you create a new assertNotNull function like so:

function assertNotNull<T>(val: T | null | undefined, msg = "assert not null"): val is T {
  if (val === null || val === undefined) {
    throw Error(msg);
  }
  return true;
}

See this question on stackoverflow for more info: https://stackoverflow.com/a/46700791/266535

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@styfle that doesn't work the way you think it would work, because TypeScript does not know that the code past that point is narrowed without some block scoping. For example, if this is run under --strictNullChecks:

function assertNotNull<T>(val: T | null | undefined, msg = "assert not null"): val is T {
  if (val === null || val === undefined) {
    throw Error(msg);
  }
  return true;
}

declare const foo: string | undefined;
declare function bar(val: string): void;

foo; // string | undefined
assertNotNull(foo);
foo; // string | undefined
bar(foo); // maybe undefined

Also see microsoft/TypeScript#8655 which explains that problem is not currently supported by TypeScript.

Copy link
Contributor

@styfle styfle Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@kitsonk Oh right, you would need an if statement for the assert for it to narrow the type as not null.
See this playground.
But it's still safer than using ! in my opinion 😅

}
25 changes: 18 additions & 7 deletions js/os.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,14 @@ export function codeFetch(
fbs.Base.addMsgType(builder, fbs.Any.CodeFetch);
builder.finish(fbs.Base.endBase(builder));
const resBuf = libdeno.send(builder.asUint8Array());
assert(resBuf != null);
// Process CodeFetchRes
const bb = new flatbuffers.ByteBuffer(new Uint8Array(resBuf));
// TypeScript does not track `assert` from a CFA perspective, therefore not
// null assertion `!`
const bb = new flatbuffers.ByteBuffer(new Uint8Array(resBuf!));
Copy link
Member

Choose a reason for hiding this comment

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

These null assert checks are redundant when using the bang operator. Suggest removing the assert.

const baseRes = fbs.Base.getRootAsBase(bb);
if (fbs.Any.NONE === baseRes.msgType()) {
throw Error(baseRes.error());
throw Error(baseRes.error()!);
}
assert(fbs.Any.CodeFetchRes === baseRes.msgType());
const codeFetchRes = new fbs.CodeFetchRes();
Expand Down Expand Up @@ -80,7 +83,9 @@ export function codeCache(
const bb = new flatbuffers.ByteBuffer(new Uint8Array(resBuf));
const baseRes = fbs.Base.getRootAsBase(bb);
assert(fbs.Any.NONE === baseRes.msgType());
throw Error(baseRes.error());
// undefined and null are incompatible in strict mode, but at runtime
// a null value is fine, therefore not null assertion
throw Error(baseRes.error()!);
}
}

Expand All @@ -103,16 +108,22 @@ export function readFileSync(filename: string): Uint8Array {
builder.finish(fbs.Base.endBase(builder));
const resBuf = libdeno.send(builder.asUint8Array());
assert(resBuf != null);

const bb = new flatbuffers.ByteBuffer(new Uint8Array(resBuf));
// TypeScript does not track `assert` from a CFA perspective, therefore not
// null assertion `!`
const bb = new flatbuffers.ByteBuffer(new Uint8Array(resBuf!));
Copy link
Member

Choose a reason for hiding this comment

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

These null assert checks are redundant when using the bang operator. Suggest removing the assert.

const baseRes = fbs.Base.getRootAsBase(bb);
if (fbs.Any.NONE === baseRes.msgType()) {
throw Error(baseRes.error());
// undefined and null are incompatible in strict mode, but at runtime
// a null value is fine, therefore not null assertion
throw Error(baseRes.error()!);
}
assert(fbs.Any.ReadFileSyncRes === baseRes.msgType());
const res = new fbs.ReadFileSyncRes();
assert(baseRes.msg(res) != null);
return new Uint8Array(res.dataArray());
const dataArray = res.dataArray();
assert(dataArray != null);
// TypeScript cannot track assertion above, therefore not null assertion
return new Uint8Array(dataArray!);
}

export function writeFileSync(
Expand Down
87 changes: 53 additions & 34 deletions js/runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export function setup(): void {
const mod = FileModule.load(filename);
if (!mod) {
util.log("getGeneratedContents cannot find", filename);
return null;
return "";
}
return mod.outputCode;
}
Expand All @@ -71,7 +71,7 @@ export function setup(): void {
// FileModule.load(). FileModules are NOT executed upon first load, only when
// compileAndRun is called.
export class FileModule {
scriptVersion: string;
scriptVersion = "";
readonly exports = {};

private static readonly map = new Map<string, FileModule>();
Expand Down Expand Up @@ -105,15 +105,15 @@ export class FileModule {
execute(this.fileName, this.outputCode);
}

static load(fileName: string): FileModule {
static load(fileName: string): FileModule | undefined {
return this.map.get(fileName);
}

static getScriptsWithSourceCode(): string[] {
const out = [];
for (const fn of this.map.keys()) {
const m = this.map.get(fn);
if (m.sourceCode) {
if (m && m.sourceCode) {
out.push(fn);
}
}
Expand All @@ -127,7 +127,8 @@ export function makeDefine(fileName: string): AmdDefine {
log("localRequire", x);
};
const currentModule = FileModule.load(fileName);
const localExports = currentModule.exports;
util.assert(currentModule != null);
const localExports = currentModule!.exports;
log("localDefine", fileName, deps, localExports);
const args = deps.map(dep => {
if (dep === "require") {
Expand All @@ -140,9 +141,13 @@ export function makeDefine(fileName: string): AmdDefine {
return deno;
} else {
const resolved = resolveModuleName(dep, fileName);
const depModule = FileModule.load(resolved);
depModule.compileAndRun();
return depModule.exports;
util.assert(resolved != null);
Copy link
Member

Choose a reason for hiding this comment

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

These null assert checks are redundant when using the bang operator. Suggest removing the assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ry just to make it clear, the not null assertion operator (!) in TypeScript only says "I know what I am doing" and does not actually change the runtime behaviour in any sense. Given that, should we still remove it, or is it more valuable to actually put a comment in the assert to make the potential throw more logical.

As a side note, the TypeScript team recently closed the issue that would allow assert like calls to affect CFA analysis (microsoft/TypeScript#8655) in lieu of microsoft/TypeScript#10421 which might allow authoring of some sort of function(s) that would allow this pattern to affect CFA and therefore the type past this point in the block.

Copy link
Member

Choose a reason for hiding this comment

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

@kitsonk Oh sorry - I thought it threw an exception. In that case ignore my comments and keep the asserts.

const depModule = FileModule.load(resolved!);
if (depModule) {
depModule.compileAndRun();
return depModule.exports;
}
return undefined;
}
});
factory(...args);
Expand All @@ -156,10 +161,14 @@ export function resolveModule(
): null | FileModule {
util.log("resolveModule", { moduleSpecifier, containingFile });
util.assert(moduleSpecifier != null && moduleSpecifier.length > 0);
let filename: string, sourceCode: string, outputCode: string;
let filename: string | null;
let sourceCode: string | null;
let outputCode: string | null;
if (moduleSpecifier.startsWith(ASSETS) || containingFile.startsWith(ASSETS)) {
// Assets are compiled into the runtime javascript bundle.
const moduleId = moduleSpecifier.split("/").pop();
// we _know_ `.pop()` will return a string, but TypeScript doesn't so
// not null assertion
const moduleId = moduleSpecifier.split("/").pop()!;
const assetName = moduleId.includes(".") ? moduleId : `${moduleId}.d.ts`;
util.assert(assetName in assetSourceCode, `No such asset "${assetName}"`);
sourceCode = assetSourceCode[assetName];
Expand All @@ -179,15 +188,17 @@ export function resolveModule(
sourceCode = fetchResponse.sourceCode;
outputCode = fetchResponse.outputCode;
}
if (sourceCode == null || sourceCode.length === 0) {
if (sourceCode == null || sourceCode.length === 0 || filename == null) {
return null;
}
util.log("resolveModule sourceCode length ", sourceCode.length);
const m = FileModule.load(filename);
if (m != null) {
return m;
} else {
return new FileModule(filename, sourceCode, outputCode);
// null and undefined are incompatible in strict mode, but outputCode being
// null here has no runtime behavior impact, therefore not null assertion
return new FileModule(filename, sourceCode, outputCode!);
}
}

Expand All @@ -204,7 +215,7 @@ function resolveModuleName(
}

function execute(fileName: string, outputCode: string): void {
util.assert(outputCode && outputCode.length > 0);
util.assert(outputCode != null && outputCode.length > 0);
window["define"] = makeDefine(fileName);
outputCode += `\n//# sourceURL=${fileName}`;
globalEval(outputCode);
Expand Down Expand Up @@ -278,7 +289,7 @@ class TypeScriptHost implements ts.LanguageServiceHost {
getScriptVersion(fileName: string): string {
util.log("getScriptVersion", fileName);
const m = FileModule.load(fileName);
return m.scriptVersion;
return (m && m.scriptVersion) || "";
}

getScriptSnapshot(fileName: string): ts.IScriptSnapshot | undefined {
Expand Down Expand Up @@ -323,32 +334,40 @@ class TypeScriptHost implements ts.LanguageServiceHost {
const fn = "lib.globals.d.ts"; // ts.getDefaultLibFileName(options);
util.log("getDefaultLibFileName", fn);
const m = resolveModule(fn, ASSETS);
return m.fileName;
util.assert(m != null);
// TypeScript cannot track assertions, therefore not null assertion
return m!.fileName;
}

resolveModuleNames(
moduleNames: string[],
containingFile: string,
reusedNames?: string[]
): Array<ts.ResolvedModule | undefined> {
containingFile: string
): ts.ResolvedModule[] {
//util.log("resolveModuleNames", { moduleNames, reusedNames });
return moduleNames.map((name: string) => {
let resolvedFileName;
if (name === "deno") {
resolvedFileName = resolveModuleName("deno.d.ts", ASSETS);
} else if (name === "typescript") {
resolvedFileName = resolveModuleName("typescript.d.ts", ASSETS);
} else {
resolvedFileName = resolveModuleName(name, containingFile);
if (resolvedFileName == null) {
return undefined;
return moduleNames
.map(name => {
let resolvedFileName;
if (name === "deno") {
resolvedFileName = resolveModuleName("deno.d.ts", ASSETS);
} else if (name === "typescript") {
resolvedFileName = resolveModuleName("typescript.d.ts", ASSETS);
} else {
resolvedFileName = resolveModuleName(name, containingFile);
}
}
// This flags to the compiler to not go looking to transpile functional
// code, anything that is in `/$asset$/` is just library code
const isExternalLibraryImport = resolvedFileName.startsWith(ASSETS);
return { resolvedFileName, isExternalLibraryImport };
});
// According to the interface we shouldn't return `undefined` but if we
// fail to return the same length of modules to those we cannot resolve
// then TypeScript fails on an assertion that the lengths can't be
// different, so we have to return an "empty" resolved module
// TODO: all this does is push the problem downstream, and TypeScript
// will complain it can't identify the type of the file and throw
// a runtime exception, so we need to handle missing modules better
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plan to deal with this under #509

resolvedFileName = resolvedFileName || "";
// This flags to the compiler to not go looking to transpile functional
// code, anything that is in `/$asset$/` is just library code
const isExternalLibraryImport = resolvedFileName.startsWith(ASSETS);
// TODO: we should be returning a ts.ResolveModuleFull
return { resolvedFileName, isExternalLibraryImport };
});
}
}

Expand Down
8 changes: 4 additions & 4 deletions js/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
export type TypedArray = Uint8Array | Float32Array | Int32Array;

export interface ModuleInfo {
moduleName?: string;
filename?: string;
sourceCode?: string;
outputCode?: string;
moduleName: string | null;
filename: string | null;
sourceCode: string | null;
outputCode: string | null;
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 not the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under strict mode, undefined and null are incompatible. Because when we create the object literal, we are using functions that return null but always defining the properties. We could retain that optional properties, but if we use null for a not present value, we have to include it as a potential type.

Copy link
Member

Choose a reason for hiding this comment

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

ok - makes sense

}

// Following definitions adapted from:
Expand Down
11 changes: 8 additions & 3 deletions js/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,20 @@ export function arrayToStr(ui8: Uint8Array): string {
// produces broken code when targeting ES5 code.
// See https://github.com/Microsoft/TypeScript/issues/15202
// At the time of writing, the github issue is closed but the problem remains.
export interface Resolvable<T> extends Promise<T> {
export interface ResolvableMethods<T> {
resolve: (value?: T | PromiseLike<T>) => void;
// tslint:disable-next-line:no-any
reject: (reason?: any) => void;
}

type Resolvable<T> = Promise<T> & ResolvableMethods<T>;

export function createResolvable<T>(): Resolvable<T> {
let methods;
let methods: ResolvableMethods<T>;
const promise = new Promise<T>((resolve, reject) => {
methods = { resolve, reject };
});
return Object.assign(promise, methods) as Resolvable<T>;
// TypeScript doesn't know that the Promise callback occurs synchronously
// therefore use of not null assertion (`!`)
return Object.assign(promise, methods!) as Resolvable<T>;
}
16 changes: 8 additions & 8 deletions js/v8_source_maps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export function prepareStackTraceWrapper(
try {
return prepareStackTrace(error, stack);
} catch (prepareStackError) {
Error.prepareStackTrace = null;
Error.prepareStackTrace = undefined;
console.log("=====Error inside of prepareStackTrace====");
console.log(prepareStackError.stack.toString());
console.log("=====Original error=======================");
Expand All @@ -66,8 +66,8 @@ export function wrapCallSite(frame: CallSite): CallSite {
const source = frame.getFileName() || frame.getScriptNameOrSourceURL();

if (source) {
const line = frame.getLineNumber();
const column = frame.getColumnNumber() - 1;
const line = frame.getLineNumber() || 0;
const column = (frame.getColumnNumber() || 1) - 1;
const position = mapSourcePosition({ source, line, column });
frame = cloneCallSite(frame);
frame.getFileName = () => position.source;
Expand All @@ -79,7 +79,7 @@ export function wrapCallSite(frame: CallSite): CallSite {
}

// Code called using eval() needs special handling
let origin = frame.isEval() && frame.getEvalOrigin();
let origin = (frame.isEval() && frame.getEvalOrigin()) || undefined;
if (origin) {
origin = mapEvalOrigin(origin);
frame = cloneCallSite(frame);
Expand Down Expand Up @@ -115,7 +115,7 @@ function CallSiteToString(frame: CallSite): string {
} else {
fileName = frame.getScriptNameOrSourceURL();
if (!fileName && frame.isEval()) {
fileLocation = frame.getEvalOrigin();
fileLocation = frame.getEvalOrigin() || "";
fileLocation += ", "; // Expecting source position to follow.
}

Expand Down Expand Up @@ -162,7 +162,7 @@ function CallSiteToString(frame: CallSite): string {
line += ` [as ${methodName} ]`;
}
} else {
line += typeName + "." + (methodName || "<anonymous>");
line += `${typeName}.${methodName || "<anonymous>"}`;
}
} else if (isConstructor) {
line += "new " + (functionName || "<anonymous>");
Expand All @@ -181,7 +181,7 @@ function CallSiteToString(frame: CallSite): string {
// Regex for detecting source maps
const reSourceMap = /^data:application\/json[^,]+base64,/;

function loadConsumer(source: string): SourceMapConsumer {
function loadConsumer(source: string): SourceMapConsumer | null {
let consumer = consumers.get(source);
if (consumer == null) {
const code = getGeneratedContents(source);
Expand Down Expand Up @@ -221,7 +221,7 @@ function loadConsumer(source: string): SourceMapConsumer {
return consumer;
}

function retrieveSourceMapURL(fileData: string): string {
function retrieveSourceMapURL(fileData: string): string | null {
// Get the URL of the source map
// tslint:disable-next-line:max-line-length
const re = /(?:\/\/[@#][ \t]+sourceMappingURL=([^\s'"]+?)[ \t]*$)|(?:\/\*[@#][ \t]+sourceMappingURL=([^\*]+?)[ \t]*(?:\*\/)[ \t]*$)/gm;
Expand Down
3 changes: 2 additions & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,18 @@
"baseUrl": ".",
"module": "esnext",
"moduleResolution": "node",
"noImplicitAny": true,
"noImplicitReturns": true,
"noFallthroughCasesInSwitch": true,
"noLib": true,
"noUnusedLocals": true,
"paths": {
"*": ["*", "out/debug/*", "out/default/*", "out/release/*"]
},
"preserveConstEnums": true,
"pretty": true,
"removeComments": true,
"sourceMap": true,
"strict": true,
"target": "esnext",
"types": []
},
Expand Down