Support strict mode #505

kitsonk · 2018-08-10T19:34:17Z

Resolves #504

This PR sets --strict when compiling the internal TypeScript for Deno and makes the necessary changes to support that. There were a couple issues around strict null checking that were fixed, but most were cases where TypeScript could not determine that a value was defined, but it was known to be defined. In those cases I used the not null assertion operator (!) and inserted a comment. Not all of my assumptions might of been accurate though.

kitsonk · 2018-08-12T18:00:50Z

Under strict mode, while the TypeScript compiler seems to handle it fine, returning null or undefined for host.resolveModuleNames() is not allow, in addition, these lines of code:

deno/js/runtime.ts

Lines 182 to 184 in 4772c14

 if (sourceCode == null || sourceCode.length === 0) { 

 return null; 

 }

Were causing null to to be returned for the resolved module, which in turn returned an undefined. Therefore instead of just passing "" for msg_generated.d.ts to pass " ".

The long term fix is to improve the resilience of the code in runtime.ts as well as generate unit tests for it, as well as compile and include msg_generated.d.ts properly in assets.ts as it is actually required from timers.ts to have a proper interface.

ry · 2018-08-13T15:58:09Z

js/runtime.ts

- const depModule = FileModule.load(resolved);
- depModule.compileAndRun();
- return depModule.exports;
+ util.assert(resolved != null);


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

@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.

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

ry

Thanks! Just a few comments.

ry · 2018-08-13T15:59:20Z

js/os.ts

 }
 assert(fbs.Any.ReadFileSyncRes === baseRes.msgType());
 const res = new fbs.ReadFileSyncRes();
 assert(baseRes.msg(res) != null);
- return new Uint8Array(res.dataArray());
+ // TypeScript cannot track assertion above, therefore not null assertion


I think this comment is wrong - because res.dataArray() is not checked as null?

ry · 2018-08-13T15:59:52Z

js/os.ts

- 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!));


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

ry · 2018-08-13T16:00:09Z

js/os.ts

- 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!));


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

ry · 2018-08-13T16:00:15Z

js/main.ts

- mod.compileAndRun();
+ assert(mod != null);
+ // TypeScript does not track assert, therefore not null assertion
+ mod!.compileAndRun();


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

@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

@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.

@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 😅

ry · 2018-08-13T16:01:59Z

js/runtime.ts

+ // 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


Yes, I agree

Plan to deal with this under #509

ry · 2018-08-13T16:02:52Z

js/types.d.ts

+ moduleName: string | null;
+ filename: string | null;
+ sourceCode: string | null;
+ outputCode: string | null;


Is this not the same thing?

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.

ok - makes sense

ry · 2018-08-13T16:05:13Z

js/assets.ts

@@ -96,5 +96,5 @@ export const assetSourceCode: { [key: string]: string } = {
 "types.d.ts": typesDts,

 // TODO(ry) Remove the following when possible. It's a workaround.
- "msg_generated.d.ts": "",
+ "msg_generated.d.ts": " ",


Needs to be rebased

ry

LGTM - great clean up - thank you

Bumped versions for 0.256.0 Co-authored-by: littledivy <littledivy@users.noreply.github.com>

kitsonk force-pushed the strict branch from 2fe89cb to dec0a9d Compare August 12, 2018 17:56

kitsonk changed the title ~~[WIP] Support strict mode~~ Support strict mode Aug 12, 2018

ry reviewed Aug 13, 2018

View reviewed changes

kitsonk added 4 commits August 14, 2018 11:08

Support strict mode

c8e203b

Lint fixes and format

2a7e551

Workound issue with TypeScriptHost.resolveModuleNames

ca5a94b

Properly assert in readFileSync in js/os.ts

b0ef38e

kitsonk force-pushed the strict branch from dec0a9d to b0ef38e Compare August 14, 2018 18:27

kitsonk mentioned this pull request Aug 14, 2018

Move from runtime.ts to compiler.ts #518

Closed

ry approved these changes Aug 15, 2018

View reviewed changes

ry merged commit 168d92f into denoland:master Aug 15, 2018

kitsonk deleted the strict branch August 2, 2022 04:41

hardfist pushed a commit to hardfist/deno that referenced this pull request Aug 7, 2024

0.256.0 (denoland#505)

ca6f3d1

Bumped versions for 0.256.0 Co-authored-by: littledivy <littledivy@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support strict mode #505

Support strict mode #505

kitsonk commented Aug 10, 2018

kitsonk commented Aug 12, 2018

ry Aug 13, 2018

kitsonk Aug 14, 2018

ry Aug 14, 2018

ry left a comment

ry Aug 13, 2018

ry Aug 13, 2018

ry Aug 13, 2018

ry Aug 13, 2018

styfle Aug 17, 2018

kitsonk Aug 17, 2018

styfle Aug 17, 2018 •

edited

Loading

ry Aug 13, 2018

kitsonk Aug 14, 2018

ry Aug 13, 2018

kitsonk Aug 14, 2018

ry Aug 14, 2018

ry Aug 13, 2018

ry left a comment

Support strict mode #505

Support strict mode #505

Conversation

kitsonk commented Aug 10, 2018

kitsonk commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry 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

styfle Aug 17, 2018 • edited Loading

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

ry left a comment

Choose a reason for hiding this comment

styfle Aug 17, 2018 •

edited

Loading