Improve null safety #996

josephjunker · 2023-12-23T17:00:19Z

This adds a second tsconfig which can be used to view null safety errors, and fixes a handful of them. Nothing here should change runtime behavior and I've aimed to make the changes as noninvasive as possible.

I've been loose with test files, preferring unsafe assertions to code changes in them
When possible, I've added annotations rather than modify runtime code

I've added explicit type annotations and intermediate interfaces for the return types of some functions and methods which were previously implicit. In some cases this helps inference go through without changes and in other cases it just made it easier for me to determine what exactly was undefined in a nested object.

I've skipped a couple of the harder errors. Most notably, there seems to be an implicit distinction between an "input BsConfig" and an "initialized BsConfig", in that many fields in the BsConfig class are optional but assumed to be present during actual processing. The potentially pedantic way to address this would be to make a second InitializedBsConfig type which is used internally and is produced by parsing a BsConfig, but this would be significantly more invasive than what I am aiming for here.

Please let me know if you would prefer any modifications to my approach.

src/LanguageServer.ts

src/parser/AstNode.ts

src/util.ts

josephjunker · 2023-12-23T18:53:52Z

src/util.ts

+            //if we don't have a rightmost range, use the leftmost range for both the start and end
+            return this.createRangeFromPositions(
+                leftmostRange.start,
+                rightmostRange ? rightmostRange.end : leftmostRange.end);


This is changed behavior; previously an error would have been thrown in this case. I don't know whether this case would occur in practice.

josephjunker · 2023-12-23T18:56:24Z

src/util.ts

+                    } else {
+                        //this should never happen; somehow an invalid plugin has made it into here
+                        throw new Error(`TILT: Encountered an invalid plugin: ${String(plugin)}`);


I tend to sprinkle these assertions into my code, but they are often considered unidiomatic. The alternative here would be to remove the if condition from the else if above. Removing the condition could allow bad data to get farther into the program before it crashes, but so long as the type of theExport (CompilerPlugin | CompilerPluginFactory) is correct, no if is needed in the else.

src/SymbolTable.ts

src/CommentFlagProcessor.ts

src/DependencyGraph.ts

src/DiagnosticFilterer.ts

src/DiagnosticSeverityAdjuster.ts

src/LanguageServer.ts

src/Logger.ts

src/astUtils/reflection.ts

josephjunker · 2023-12-23T19:34:27Z

src/parser/Expression.ts

@@ -130,7 +130,7 @@ export class FunctionExpression extends Expression implements TypedefProvider {
        super();
        if (this.returnTypeToken) {
            this.returnType = util.tokenToBscType(this.returnTypeToken);
-        } else if (this.functionType.text.toLowerCase() === 'sub') {
+        } else if (this.functionType && this.functionType.text.toLowerCase() === 'sub') {


Another case of "would previously throw an error in an edge case which will likely never happen"

Can we use the optional chaining operator here instead?

Suggested change

} else if (this.functionType && this.functionType.text.toLowerCase() === 'sub') {

} else if (this.functionType?.text.toLowerCase() === 'sub') {

src/parser/TranspileState.ts

src/util.ts

josephjunker · 2023-12-23T19:38:30Z

src/util.ts

@@ -510,7 +512,7 @@ export class Util {
     * |  bbb | bb   |  bbb |  b   | bbb  |    bb |  bb   |     b | a     |
     * ```
     */
-    public rangesIntersect(a: Range, b: Range) {
+    public rangesIntersect(a: Range | undefined, b: Range | undefined) {


This and rangesIntersectOrTouch are called with potentially undefined values elsewhere but handle undefinedness gracefully internally, so I've loosened their type signatures.

josephjunker · 2023-12-23T19:39:00Z

src/util.ts

@@ -732,7 +734,7 @@ export class Util {
            //this diagnostic is affected by this flag
            if (diagnostic.range && this.rangeContains(flag.affectedRange, diagnostic.range.start)) {
                //if the flag acts upon this diagnostic's code
-                if (flag.codes === null || flag.codes.includes(diagnosticCode)) {
+                if (flag.codes === null || (diagnosticCode !== undefined && flag.codes.includes(diagnosticCode))) {


Not a behavioral change, just satisfies the typechecker

josephjunker · 2023-12-23T19:41:31Z

src/util.ts

@@ -1477,7 +1489,7 @@ export class Util {

    public validateTooDeepFile(file: (BrsFile | XmlFile)) {
        //find any files nested too deep
-        let pkgPath = file.pkgPath ?? file.pkgPath.toString();
+        let pkgPath = file.pkgPath ?? (file.pkgPath as any).toString();


The type of file.pkgPath is string, so unless something unexpected has happened this toString() call isn't necessary. Since it seems to only be relevant to cases where the types are wrong I don't think that an any cast will hurt anything.

TwitchBronBron

Looking great! I found a few things that need addressed, but overall I'm happy with these changes! Thanks a bunch.

src/bscPlugin/hover/HoverProcessor.ts

src/bscPlugin/validation/BrsFileValidator.spec.ts

src/parser/AstNode.ts

TwitchBronBron · 2024-01-02T13:31:38Z

src/parser/Expression.ts

@@ -130,7 +130,7 @@ export class FunctionExpression extends Expression implements TypedefProvider {
        super();
        if (this.returnTypeToken) {
            this.returnType = util.tokenToBscType(this.returnTypeToken);
-        } else if (this.functionType.text.toLowerCase() === 'sub') {
+        } else if (this.functionType && this.functionType.text.toLowerCase() === 'sub') {


Can we use the optional chaining operator here instead?

Suggested change

} else if (this.functionType && this.functionType.text.toLowerCase() === 'sub') {

} else if (this.functionType?.text.toLowerCase() === 'sub') {

src/LanguageServer.ts

src/Logger.ts

TwitchBronBron · 2024-01-02T16:07:56Z

src/Program.spec.ts

+            const expected: BsDiagnostic[] = [{
                message: 'message',
                file: undefined,
                range: undefined
-            }];
+            }] as any;


Couldn't you do it like this instead?

Suggested change

const expected: BsDiagnostic[] = [{

message: 'message',

file: undefined,

range: undefined

}];

}] as any;

const expected = [{

message: 'message',

file: undefined,

range: undefined

}] as BsDiagnostic[]

I've changed it to:

const expected = [{ message: 'message', file: undefined, range: undefined }] as any as BsDiagnostic[]

There has to be an any somewhere. I could also do

const expected = [{ message: 'message', file: undefined as any, range: undefined as any }] as BsDiagnostic[];

as an alternative

tsconfig-null-safe.json

TwitchBronBron

Looking great! I found a few things that need addressed, but overall I'm happy with these changes! Thanks a bunch.

src/preprocessor/PreprocessorParser.ts

TwitchBronBron

Looks great! One question, and then I think this is ready to go!

TwitchBronBron · 2024-01-04T01:44:39Z

Looks great! Thanks for this.