Add suggestion diagnostics for unused label and unreachable code (#24261)

* Add suggestion diagnostics for unused label and unreachable code

* Always error on unused left hand side of comma

Author: Andy

src/compiler/binder.ts

@@ -1208,7 +1208,7 @@ namespace ts {
             bind(node.statement);
             popActiveLabel();
             if (!activeLabel.referenced && !options.allowUnusedLabels) {
-                file.bindDiagnostics.push(createDiagnosticForNode(node.label, Diagnostics.Unused_label));
+                errorOrSuggestionOnFirstToken(unusedLabelIsError(options), node, Diagnostics.Unused_label);
             }
             if (!node.statement || node.statement.kind !== SyntaxKind.DoStatement) {
                 // do statement sets current flow inside bindDoStatement
@@ -1914,6 +1914,17 @@ namespace ts {
             file.bindDiagnostics.push(createFileDiagnostic(file, span.start, span.length, message, arg0, arg1, arg2));
         }
 
+        function errorOrSuggestionOnFirstToken(isError: boolean, node: Node, message: DiagnosticMessage, arg0?: any, arg1?: any, arg2?: any) {
+            const span = getSpanOfTokenAtPosition(file, node.pos);
+            const diag = createFileDiagnostic(file, span.start, span.length, message, arg0, arg1, arg2);
+            if (isError) {
+                file.bindDiagnostics.push(diag);
+            }
+            else {
+                file.bindSuggestionDiagnostics = append(file.bindSuggestionDiagnostics, { ...diag, category: DiagnosticCategory.Suggestion });
+            }
+        }
+
         function bind(node: Node): void {
             if (!node) {
                 return;
@@ -2730,26 +2741,26 @@ namespace ts {
                 if (reportError) {
                     currentFlow = reportedUnreachableFlow;
 
-                    // unreachable code is reported if
-                    // - user has explicitly asked about it AND
-                    // - statement is in not ambient context (statements in ambient context is already an error
-                    //   so we should not report extras) AND
-                    //   - node is not variable statement OR
-                    //   - node is block scoped variable statement OR
-                    //   - node is not block scoped variable statement and at least one variable declaration has initializer
-                    //   Rationale: we don't want to report errors on non-initialized var's since they are hoisted
-                    //   On the other side we do want to report errors on non-initialized 'lets' because of TDZ
-                    const reportUnreachableCode =
-                        !options.allowUnreachableCode &&
-                        !(node.flags & NodeFlags.Ambient) &&
-                        (
-                            node.kind !== SyntaxKind.VariableStatement ||
-                            getCombinedNodeFlags((<VariableStatement>node).declarationList) & NodeFlags.BlockScoped ||
-                            forEach((<VariableStatement>node).declarationList.declarations, d => d.initializer)
-                        );
-
-                    if (reportUnreachableCode) {
-                        errorOnFirstToken(node, Diagnostics.Unreachable_code_detected);
+                    if (!options.allowUnreachableCode) {
+                        // unreachable code is reported if
+                        // - user has explicitly asked about it AND
+                        // - statement is in not ambient context (statements in ambient context is already an error
+                        //   so we should not report extras) AND
+                        //   - node is not variable statement OR
+                        //   - node is block scoped variable statement OR
+                        //   - node is not block scoped variable statement and at least one variable declaration has initializer
+                        //   Rationale: we don't want to report errors on non-initialized var's since they are hoisted
+                        //   On the other side we do want to report errors on non-initialized 'lets' because of TDZ
+                        const isError =
+                            unreachableCodeIsError(options) &&
+                            !(node.flags & NodeFlags.Ambient) &&
+                            (
+                                !isVariableStatement(node) ||
+                                !!(getCombinedNodeFlags(node.declarationList) & NodeFlags.BlockScoped) ||
+                                node.declarationList.declarations.some(d => !!d.initializer)
+                            );
+
+                        errorOrSuggestionOnFirstToken(isError, node, Diagnostics.Unreachable_code_detected);
                     }
                 }
             }

src/compiler/core.ts

@@ -1991,6 +1991,14 @@ namespace ts {
         return moduleResolution;
     }
 
+    export function unreachableCodeIsError(options: CompilerOptions): boolean {
+        return options.allowUnreachableCode === false;
+    }
+
+    export function unusedLabelIsError(options: CompilerOptions): boolean {
+        return options.allowUnusedLabels === false;
+    }
+
     export function getAreDeclarationMapsEnabled(options: CompilerOptions) {
         return !!(options.declaration && options.declarationMap);
     }

src/compiler/diagnosticMessages.json

@@ -2238,7 +2238,8 @@
     },
     "Left side of comma operator is unused and has no side effects.": {
         "category": "Error",
-        "code": 2695
+        "code": 2695,
+        "reportsUnnecessary": true
     },
     "The 'Object' type is assignable to very few other types. Did you mean to use the 'any' type instead?": {
         "category": "Error",
@@ -3673,7 +3674,8 @@
     },
     "Unreachable code detected.": {
         "category": "Error",
-        "code": 7027
+        "code": 7027,
+        "reportsUnnecessary": true
     },
     "Unused label.": {
         "category": "Error",

src/compiler/factory.ts

@@ -2463,6 +2463,7 @@ namespace ts {
             if (node.symbolCount !== undefined) updated.symbolCount = node.symbolCount;
             if (node.parseDiagnostics !== undefined) updated.parseDiagnostics = node.parseDiagnostics;
             if (node.bindDiagnostics !== undefined) updated.bindDiagnostics = node.bindDiagnostics;
+            if (node.bindSuggestionDiagnostics !== undefined) updated.bindSuggestionDiagnostics = node.bindSuggestionDiagnostics;
             if (node.lineMap !== undefined) updated.lineMap = node.lineMap;
             if (node.classifiableNames !== undefined) updated.classifiableNames = node.classifiableNames;
             if (node.resolvedModules !== undefined) updated.resolvedModules = node.resolvedModules;

src/compiler/parser.ts

@@ -912,6 +912,7 @@ namespace ts {
 
             sourceFile.text = sourceText;
             sourceFile.bindDiagnostics = [];
+            sourceFile.bindSuggestionDiagnostics = undefined;
             sourceFile.languageVersion = languageVersion;
             sourceFile.fileName = normalizePath(fileName);
             sourceFile.languageVariant = getLanguageVariant(scriptKind);

src/compiler/types.ts

@@ -2603,6 +2603,7 @@ namespace ts {
 
         // File-level diagnostics reported by the binder.
         /* @internal */ bindDiagnostics: Diagnostic[];
+        /* @internal */ bindSuggestionDiagnostics?: Diagnostic[];
 
         // File-level JSDoc diagnostics reported by the JSDoc parser
         /* @internal */ jsDocDiagnostics?: Diagnostic[];

src/harness/fourslash.ts

@@ -1317,8 +1317,13 @@ Actual: ${stringify(fullActual)}`);
         }
 
         private testDiagnostics(expected: ReadonlyArray<FourSlashInterface.Diagnostic>, diagnostics: ReadonlyArray<ts.Diagnostic>, category: string) {
-            assert.deepEqual(ts.realizeDiagnostics(diagnostics, ts.newLineCharacter), expected.map<ts.RealizedDiagnostic>(e => (
-                { message: e.message, category, code: e.code, ...ts.createTextSpanFromRange(e.range || this.getRanges()[0]) })));
+            assert.deepEqual(ts.realizeDiagnostics(diagnostics, ts.newLineCharacter), expected.map((e): ts.RealizedDiagnostic => ({
+                message: e.message,
+                category,
+                code: e.code,
+                ...ts.createTextSpanFromRange(e.range || this.getRanges()[0]),
+                reportsUnnecessary: e.reportsUnnecessary,
+            })));
         }
 
         public verifyQuickInfoAt(markerName: string, expectedText: string, expectedDocumentation?: string) {
@@ -4422,15 +4427,15 @@ namespace FourSlashInterface {
             this.state.verifyQuickInfoDisplayParts(kind, kindModifiers, textSpan, displayParts, documentation, tags);
         }
 
-        public getSyntacticDiagnostics(expected: ReadonlyArray<ts.RealizedDiagnostic>) {
+        public getSyntacticDiagnostics(expected: ReadonlyArray<Diagnostic>) {
             this.state.getSyntacticDiagnostics(expected);
         }
 
-        public getSemanticDiagnostics(expected: ReadonlyArray<ts.RealizedDiagnostic>) {
+        public getSemanticDiagnostics(expected: ReadonlyArray<Diagnostic>) {
             this.state.getSemanticDiagnostics(expected);
         }
 
-        public getSuggestionDiagnostics(expected: ReadonlyArray<ts.RealizedDiagnostic>) {
+        public getSuggestionDiagnostics(expected: ReadonlyArray<Diagnostic>) {
             this.state.getSuggestionDiagnostics(expected);
         }
 
@@ -4837,6 +4842,7 @@ namespace FourSlashInterface {
         message: string;
         range?: FourSlash.Range;
         code: number;
+        reportsUnnecessary?: true;
     }
 
     export interface GetEditsForFileRenameOptions {

src/services/services.ts

@@ -548,6 +548,7 @@ namespace ts {
         public syntacticDiagnostics: Diagnostic[];
         public parseDiagnostics: Diagnostic[];
         public bindDiagnostics: Diagnostic[];
+        public bindSuggestionDiagnostics?: Diagnostic[];
 
         public isDeclarationFile: boolean;
         public isDefaultLib: boolean;

src/services/shims.ts

@@ -586,7 +586,7 @@ namespace ts {
         length: number;
         category: string;
         code: number;
-        unused?: {};
+        reportsUnnecessary?: {};
     }
     export function realizeDiagnostics(diagnostics: ReadonlyArray<Diagnostic>, newLine: string): RealizedDiagnostic[] {
         return diagnostics.map(d => realizeDiagnostic(d, newLine));
@@ -598,7 +598,8 @@ namespace ts {
             start: diagnostic.start,
             length: diagnostic.length,
             category: diagnosticCategoryName(diagnostic),
-            code: diagnostic.code
+            code: diagnostic.code,
+            reportsUnnecessary: diagnostic.reportsUnnecessary,
         };
     }
 

src/services/suggestionDiagnostics.ts

@@ -60,6 +60,7 @@ namespace ts {
             }
         }
 
+        addRange(diags, sourceFile.bindSuggestionDiagnostics);
         return diags.concat(checker.getSuggestionDiagnostics(sourceFile)).sort((d1, d2) => d1.start - d2.start);
     }