-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Increase span of unreachable code error #25388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5d67cdc
to
01cf8b0
Compare
src/compiler/binder.ts
Outdated
@@ -1211,7 +1211,7 @@ namespace ts { | |||
bind(node.statement); | |||
popActiveLabel(); | |||
if (!activeLabel.referenced && !options.allowUnusedLabels) { | |||
errorOrSuggestionOnFirstToken(unusedLabelIsError(options), node, Diagnostics.Unused_label); | |||
errorOrSuggestion(unusedLabelIsError(options), node.label, Diagnostics.Unused_label); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will mark function declarations as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The quick fix will skip them, but the error message will be a single span rather than several spans separated by functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it is worth changing it to multiple spans?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this would be ugly for a pattern like:
return { fn1, fn2, fn3, <<etc>> };
doSometing();
function fn1() { ... }
function fn2() { ... }
<<etc>>
that would show hundreds of lines as unreachable when only one line is.
The problem is that our current interface is:
export interface Diagnostic extends DiagnosticRelatedInformation {
/** May store more in future. For now, this will simply be `true` to indicate when a diagnostic is an unused-identifier diagnostic. */
reportsUnnecessary?: {};
source?: string;
relatedInformation?: DiagnosticRelatedInformation[];
}
export interface DiagnosticRelatedInformation {
category: DiagnosticCategory;
code: number;
file: SourceFile | undefined;
start: number | undefined;
length: number | undefined;
messageText: string | DiagnosticMessageChain;
}
export interface DiagnosticWithLocation extends Diagnostic {
file: SourceFile;
start: number;
length: number;
}
So each additional span would need a new messageText
. But it's unreachable for the same reason the first line of code was unreachable, so it doesn't feel right to add an additional span for that.
We could just add a new diagnostic for each group of unreachable statements separated by functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of multiple diagnostics, one for each contiguous span..
01cf8b0
to
96db135
Compare
* Increase span of unreachable code error * Add a new diagnostic for each range of unreachable statements * Update baselines
Fixes #25384