Fix bugs: Replace SourceFile if '--noUnusedLabels' changed #27060

ghost · 2018-09-12T23:02:44Z

(or similar bindDiagnostics-affecting options)

sheetalkamat · 2018-09-12T23:40:07Z

src/compiler/utilities.ts

+    export function getSourceFileAffectingCompilerOptions(compilerOptions: CompilerOptions): CompilerOptions {
+        const res: CompilerOptions = {};
+        for (const option of optionDeclarations) {
+            if (option.name in compilerOptions && isSourceFileAffectingCompilerOption(option)) {


Like this! Can we instead put property on option declaration instead. Eg look at affectsSemanticDiagnostics in CommandLineOptionBase

Also noUnusedLabels shouldnt need the new sourceFile. Instead it needs to set affectsSemanticDiagnostics

It already had affectsSemanticDiagnostics, the problem is that the --noUnusedLabels error is attached to the SourceFile#bindDiagnostics directly during binding. While affectsSemanticDiagnostics seems to only affect semanticDiagnosticsPerFile.

sheetalkamat · 2018-09-13T23:20:30Z

src/compiler/commandLineParser.ts

@@ -441,6 +449,7 @@ namespace ts {
                type: "string",
                isFilePath: true
            },
+            affectsSourceFile: true,


Why ? RootDir and paths shouldn't affect syntaxTree or bindDiagnostics?
Same goes for lib, baseUrl, typeRoots, paths

Those all effect module resolution, and SourceFile stores resolvedModules.

sheetalkamat · 2018-09-13T23:21:50Z

src/compiler/program.ts

-            oldOptions.baseUrl !== newOptions.baseUrl ||
-            !equalOwnProperties(oldOptions.paths, newOptions.paths)
-        );
+        return !!oldOptions && !isJsonEqual(getSourceFileAffectingCompilerOptions(oldOptions), getSourceFileAffectingCompilerOptions(newOptions));


Also want to check if: getStrictOptionValue(opts, "alwaysStrict") (which depends on value of strict and alwaysStrict flag)

Handled this by adding affectsSourceFile to both "alwaysStrict" and "strict".

I don't think you need to create separate objects just for comparison, You should just iterate through options and achieve same thing.. Eg. compilerOptionsAffectSemanticDiagnostics

sheetalkamat · 2018-09-14T17:56:43Z

src/compiler/commandLineParser.ts

@@ -426,6 +434,7 @@ namespace ts {
            // use type = object to copy the value as-is
            name: "paths",
            type: "object",
+            affectsModuleResolution: true,


While you have added this, could you please change changesAffectModuleResolution as well to use this instead of manual checks.

sheetalkamat · 2018-09-14T17:57:29Z

src/compiler/program.ts

-            oldOptions.baseUrl !== newOptions.baseUrl ||
-            !equalOwnProperties(oldOptions.paths, newOptions.paths)
-        );
+        return !!oldOptions && !isJsonEqual(getSourceFileAffectingCompilerOptions(oldOptions), getSourceFileAffectingCompilerOptions(newOptions));


Btw you have option.type to determine if you want to do arrayIsEqualTo or equalOwnProperties or just direct equals check.

Here's how far I got:

export function areCompilerOptionsEqual(option: CommandLineOption, a: unknown, b: unknown) { switch (option.type) { case "boolean": case "number": case "string": return a === b; case "list": return arraysEqual(a as unknown[], b as unknown[], (aa, bb) => areCompilerOptionsEqual((option as CommandLineOptionOfListType).element, aa, bb)); case "object": return equalOwnProperties(a as MapLike<unknown>, b as MapLike<unknown>, isJsonEqual); default: something with maps; } }

Doesn't seem worth it since I need to call isJsonEqual in the nested case anyway, and I need some casts which might not be valid if someone put a wrong value in their tsconfig.

ghost · 2018-09-15T00:29:11Z

src/testRunner/unittests/reuseProgramStructure.ts

@@ -305,10 +305,10 @@ namespace ts {
            assert.equal(program1.structureIsReused, StructureIsReused.Not);
        });

-        it("fails if rootdir changes", () => {
+        it("succeeds if rootdir changes", () => {


I think this change is correct? rootDir doesn't affect module resolution, it affects emit.

ghost · 2018-09-15T00:30:04Z

Note: Filed issue #27108

Fix bugs: Replace SourceFile if '--noUnusedLabels' changed

bed6722

ghost requested a review from sheetalkamat September 12, 2018 23:02

sheetalkamat requested changes Sep 12, 2018

View reviewed changes

Use properties on CommandLineOptionBase

6890ad2

ghost force-pushed the unused_label_config branch from 6722236 to 6890ad2 Compare September 13, 2018 22:48

sheetalkamat reviewed Sep 13, 2018

View reviewed changes

Andy Hanson added 2 commits September 13, 2018 16:38

Handle "alwaysStrict" and better categorize options

f31cb59

Properly handle "strict"

a9aaed5

sheetalkamat reviewed Sep 14, 2018

View reviewed changes

ghost force-pushed the unused_label_config branch from 1a3f969 to 09e065e Compare September 14, 2018 19:49

ghost mentioned this pull request Sep 14, 2018

maxNodeModuleJsDepth should have affectsModuleResolution: true #27108

Closed

Code review

433e9b4

ghost force-pushed the unused_label_config branch from 09e065e to 433e9b4 Compare September 14, 2018 23:32

fix test

ae51afa

ghost commented Sep 15, 2018

View reviewed changes

sheetalkamat approved these changes Sep 17, 2018

View reviewed changes

ghost merged commit a57467a into master Sep 17, 2018

ghost deleted the unused_label_config branch September 17, 2018 17:53

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix bugs: Replace SourceFile if '--noUnusedLabels' changed #27060

Fix bugs: Replace SourceFile if '--noUnusedLabels' changed #27060

Uh oh!

ghost commented Sep 12, 2018

Uh oh!

sheetalkamat Sep 12, 2018

Uh oh!

sheetalkamat Sep 12, 2018

Uh oh!

ghost Sep 13, 2018

Uh oh!

sheetalkamat Sep 13, 2018

Uh oh!

ghost Sep 13, 2018

Uh oh!

sheetalkamat Sep 13, 2018

Uh oh!

ghost Sep 13, 2018

Uh oh!

sheetalkamat Sep 14, 2018

Uh oh!

sheetalkamat Sep 14, 2018

Uh oh!

ghost Sep 14, 2018

Uh oh!

sheetalkamat Sep 14, 2018

Uh oh!

ghost Sep 14, 2018

Uh oh!

ghost Sep 15, 2018 •

edited by ghost

Loading

Uh oh!

ghost commented Sep 15, 2018

Uh oh!

Uh oh!

Fix bugs: Replace SourceFile if '--noUnusedLabels' changed #27060

Fix bugs: Replace SourceFile if '--noUnusedLabels' changed #27060

Uh oh!

Conversation

ghost commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Sep 15, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 15, 2018

Uh oh!

Uh oh!

ghost Sep 15, 2018 •

edited by ghost

Loading