(feat) control flow for stores (#719)

#493
$store --> (__svelte_store_get(store), $store)
By using the comma operand we allow TypeScript's control flow to work. Before that, it was $store --> __svelte_store_get(store) which loses control flow info every time.

findReferences and getDefinitions return one additional false positive reference (the svelte2tsx-added declaration) which is a TODO for later

Co-authored-by: Simon Holthausen <simon.holthausen@accso.de>
Co-authored-by: GrzegorzKazana <kazana.grzegorz@gmail.com>

package.json

@@ -16,7 +16,7 @@
         "lint": "prettier --check . && eslint \"packages/**/*.{ts,js}\""
     },
     "dependencies": {
-        "typescript": "^4.1.3"
+        "typescript": "^4.2.2"
     },
     "devDependencies": {
         "@sveltejs/eslint-config": "github:sveltejs/eslint-config#v5.2.0",

packages/language-server/src/plugins/typescript/DocumentSnapshot.ts

@@ -229,6 +229,11 @@ export class SvelteDocumentSnapshot implements DocumentSnapshot {
         return positionAt(offset, this.text);
     }
 
+    getLineContainingOffset(offset: number) {
+        const chunks = this.getText(0, offset).split('\n');
+        return chunks[chunks.length - 1];
+    }
+
     hasProp(name: string): boolean {
         return this.exportedNames.has(name);
     }

packages/language-server/src/plugins/typescript/features/DiagnosticsProvider.ts

@@ -50,7 +50,7 @@ export class DiagnosticsProviderImpl implements DiagnosticsProvider {
             }))
             .map((diagnostic) => mapObjWithRangeToOriginal(fragment, diagnostic))
             .filter(hasNoNegativeLines)
-            .filter(isNoFalsePositive(document.getText(), tsDoc))
+            .filter(isNoFalsePositive(document.getText(), tsDoc, diagnostics))
             .map(enhanceIfNecessary)
             .map(swapRangeStartEndIfNecessary);
     }
@@ -80,16 +80,40 @@ function hasNoNegativeLines(diagnostic: Diagnostic): boolean {
     return diagnostic.range.start.line >= 0 && diagnostic.range.end.line >= 0;
 }
 
-function isNoFalsePositive(text: string, tsDoc: SvelteDocumentSnapshot) {
-    return (diagnostic: Diagnostic) => {
+function isNoFalsePositive(
+    text: string,
+    tsDoc: SvelteDocumentSnapshot,
+    rawTsDiagnostics: ts.Diagnostic[]
+) {
+    return (diagnostic: Diagnostic, idx: number) => {
         return (
             isNoJsxCannotHaveMultipleAttrsError(diagnostic) &&
             isNoUnusedLabelWarningForReactiveStatement(diagnostic) &&
-            isNoUsedBeforeAssigned(diagnostic, text, tsDoc)
+            isNoUsedBeforeAssigned(diagnostic, text, tsDoc) &&
+            isNotHiddenStoreValueDeclaration(diagnostic, tsDoc, rawTsDiagnostics[idx])
         );
     };
 }
 
+/**
+ * During compilation to tsx, for each store we create an additional variable
+ * called `$<store-name>` which contains the store value.
+ * This variable declaration does not show up in the sourcemaps.
+ * We have to ignore the error if the variable prefixed by `$` was not a store.
+ */
+function isNotHiddenStoreValueDeclaration(
+    diagnostic: Diagnostic,
+    tsDoc: SvelteDocumentSnapshot,
+    rawTsDiagnostic: ts.Diagnostic
+): boolean {
+    if (diagnostic.code !== 2345 || !rawTsDiagnostic.start) return true;
+
+    const affectedLine = tsDoc.getLineContainingOffset(rawTsDiagnostic.start);
+    const hasStoreValueDefinition = /let \$[\w$]+ = __sveltets_store_get\(/.test(affectedLine);
+
+    return !hasStoreValueDefinition;
+}
+
 /**
  * Variable used before being assigned, can happen when  you do `export let x`
  * without assigning a value in strict mode. Should not throw an error here

packages/language-server/src/plugins/typescript/features/RenameProvider.ts

@@ -327,8 +327,11 @@ export class RenameProviderImpl implements RenameProvider {
             // When the user renames a Svelte component, ts will also want to rename
             // `__sveltets_instanceOf(TheComponentToRename)` or
             // `__sveltets_ensureType(TheComponentToRename,..`. Prevent that.
+            // Additionally, we cannot rename the hidden variable containing the store value
             return (
-                notPrecededBy('__sveltets_instanceOf(') && notPrecededBy('__sveltets_ensureType(')
+                notPrecededBy('__sveltets_instanceOf(') &&
+                notPrecededBy('__sveltets_ensureType(') &&
+                notPrecededBy('= __sveltets_store_get(')
             );
 
             function notPrecededBy(str: string) {

packages/language-server/test/plugins/typescript/features/DiagnosticsProvider.test.ts

@@ -7,7 +7,7 @@ import { DiagnosticsProviderImpl } from '../../../../src/plugins/typescript/feat
 import { LSAndTSDocResolver } from '../../../../src/plugins/typescript/LSAndTSDocResolver';
 import { pathToUrl } from '../../../../src/utils';
 
-const testDir = path.join(__dirname, '..', 'testfiles');
+const testDir = path.join(__dirname, '..', 'testfiles', 'diagnostics');
 
 describe('DiagnosticsProvider', () => {
     function setup(filename: string) {
@@ -234,7 +234,7 @@ describe('DiagnosticsProvider', () => {
         assert.deepStrictEqual(diagnostics, [
             {
                 code: 6385,
-                message: "'a' is deprecated",
+                message: "'a' is deprecated.",
                 range: {
                     end: {
                         character: 5,
@@ -399,4 +399,82 @@ describe('DiagnosticsProvider', () => {
             }
         ]);
     });
+
+    it('$store control flow', async () => {
+        const { plugin, document } = setup('diagnostics-$store-control-flow.svelte');
+        const diagnostics = await plugin.getDiagnostics(document);
+
+        assert.deepStrictEqual(diagnostics, [
+            {
+                code: 2367,
+                message:
+                    "This condition will always return 'false' since the types 'string' and 'boolean' have no overlap.",
+                range: {
+                    start: {
+                        line: 9,
+                        character: 40
+                    },
+                    end: {
+                        line: 9,
+                        character: 57
+                    }
+                },
+                severity: 1,
+                source: 'ts',
+                tags: []
+            },
+            {
+                code: 2322,
+                message: "Type 'string' is not assignable to type 'boolean'.",
+                range: {
+                    start: {
+                        line: 15,
+                        character: 12
+                    },
+                    end: {
+                        line: 15,
+                        character: 16
+                    }
+                },
+                severity: 1,
+                source: 'ts',
+                tags: []
+            },
+            {
+                code: 2367,
+                message:
+                    "This condition will always return 'false' since the types 'string' and 'boolean' have no overlap.",
+                range: {
+                    start: {
+                        line: 23,
+                        character: 41
+                    },
+                    end: {
+                        line: 23,
+                        character: 58
+                    }
+                },
+                severity: 1,
+                source: 'ts',
+                tags: []
+            },
+            {
+                code: 2322,
+                message: "Type 'string' is not assignable to type 'boolean'.",
+                range: {
+                    start: {
+                        line: 28,
+                        character: 13
+                    },
+                    end: {
+                        line: 28,
+                        character: 17
+                    }
+                },
+                severity: 1,
+                source: 'ts',
+                tags: []
+            }
+        ]);
+    });
 });

packages/language-server/test/plugins/typescript/features/FindReferencesProvider.test.ts

@@ -80,4 +80,80 @@ describe('FindReferencesProvider', () => {
     it('finds references (not searching from declaration)', async () => {
         await test(Position.create(2, 8), true);
     });
+
+    it('finds references for $store', async () => {
+        const { provider, document } = setup('find-references-$store.svelte');
+
+        const results = await provider.findReferences(document, Position.create(2, 10), {
+            includeDeclaration: true
+        });
+        assert.deepStrictEqual(results, [
+            {
+                range: {
+                    end: {
+                        character: 16,
+                        line: 1
+                    },
+                    start: {
+                        character: 10,
+                        line: 1
+                    }
+                },
+                uri: getUri('find-references-$store.svelte')
+            },
+            // TODO this one should be filtered out
+            {
+                range: {
+                    end: {
+                        character: 30,
+                        line: 1
+                    },
+                    start: {
+                        character: 30,
+                        line: 1
+                    }
+                },
+                uri: getUri('find-references-$store.svelte')
+            },
+            {
+                range: {
+                    end: {
+                        character: 15,
+                        line: 2
+                    },
+                    start: {
+                        character: 9,
+                        line: 2
+                    }
+                },
+                uri: getUri('find-references-$store.svelte')
+            },
+            {
+                range: {
+                    end: {
+                        character: 15,
+                        line: 3
+                    },
+                    start: {
+                        character: 9,
+                        line: 3
+                    }
+                },
+                uri: getUri('find-references-$store.svelte')
+            },
+            {
+                range: {
+                    end: {
+                        character: 8,
+                        line: 7
+                    },
+                    start: {
+                        character: 2,
+                        line: 7
+                    }
+                },
+                uri: getUri('find-references-$store.svelte')
+            }
+        ]);
+    });
 });

packages/language-server/test/plugins/typescript/features/HoverProvider.test.ts

@@ -87,7 +87,7 @@ describe('HoverProvider', () => {
         const { provider, document } = setup('hoverinfo.svelte');
 
         assert.deepStrictEqual(await provider.doHover(document, Position.create(9, 10)), <Hover>{
-            contents: '```typescript\nconst withJsDocTag: true\n```\n---\n\n\n*@author* — foo',
+            contents: '```typescript\nconst withJsDocTag: true\n```\n---\n\n\n*@author* — foo ',
             range: {
                 start: {
                     character: 10,

packages/language-server/test/plugins/typescript/features/UpdateImportsProvider.test.ts

@@ -46,14 +46,14 @@ describe('UpdateImportsProviderImpl', () => {
 
         const workspaceEdit = await updateImportsProvider.updateImports({
             // imported files both old and new have to actually exist, so we just use some other test files
-            oldUri: pathToUrl(join(testFilesDir, 'diagnostics.svelte')),
+            oldUri: pathToUrl(join(testFilesDir, 'diagnostics', 'diagnostics.svelte')),
             newUri: pathToUrl(join(testFilesDir, 'documentation.svelte'))
         });
 
         assert.deepStrictEqual(workspaceEdit?.documentChanges, [
             TextDocumentEdit.create(VersionedTextDocumentIdentifier.create(fileUri, 0), [
                 TextEdit.replace(
-                    Range.create(Position.create(1, 17), Position.create(1, 37)),
+                    Range.create(Position.create(1, 17), Position.create(1, 49)),
                     './documentation.svelte'
                 )
             ])

packages/language-server/test/plugins/typescript/testfiles/diagnostics/diagnostics-$store-control-flow.svelte

@@ -0,0 +1,32 @@
+<script lang="ts">
+import { writable } from 'svelte/store';
+const store = writable<undefined | { a: string | { b: string | boolean }}>({a: 'hi'});
+function isBoolean(t: string | boolean): t is boolean {
+    return !!t;
+}
+let test: boolean;
+if ($store) {
+    if (typeof $store.a === 'string') {
+        test = $store.a === 'string' || $store.a === true;
+    } else {
+        if (isBoolean($store.a.b)) {
+            test = $store.a.b;
+            test;
+        } else {
+            test = $store.a.b;
+        }
+    }
+}
+</script>
+
+{#if $store}
+    {#if typeof $store.a === 'string'}
+        {test = $store.a === 'string' || $store.a === true}
+    {:else}
+        {#if isBoolean($store.a.b)}
+            {test = $store.a.b}
+        {:else}
+            {test = $store.a.b}
+        {/if}
+    {/if}
+{/if}

packages/language-server/test/plugins/typescript/testfiles/diagnostics/tsconfig.json

@@ -0,0 +1,10 @@
+{
+    "compilerOptions": {
+        "strict": true,
+        /**
+          This is actually not needed, but makes the tests faster
+          because TS does not look up other types.
+        */
+        "types": ["svelte"]
+    }
+}

packages/language-server/test/plugins/typescript/testfiles/find-references-$store.svelte

@@ -0,0 +1,8 @@
+<script>
+    const findMe = writable('');
+    if ($findMe) {
+        $findMe;
+    }
+</script>
+
+{$findMe}

packages/language-server/test/plugins/typescript/testfiles/updateimports.svelte

@@ -1,3 +1,3 @@
 <script>
-import Bla from './diagnostics.svelte';
+import Bla from './diagnostics/diagnostics.svelte';
 </script>

packages/svelte2tsx/package.json

@@ -35,7 +35,7 @@
         "svelte": "~3.32.1",
         "tiny-glob": "^0.2.6",
         "tslib": "^1.10.0",
-        "typescript": "^4.1.3"
+        "typescript": "^4.2.2"
     },
     "peerDependencies": {
         "svelte": "^3.24",