getJSSyntacticDiagnosticsForFile: minor cleanup #29208
Conversation
|
What's the originating issue here? |
|
This just removes some unnecessary code I discovered while reading through the implementation. |
|
@sandersn please verify |
| for (const modifier of modifiers) { | ||
| switch (modifier.kind) { | ||
| case SyntaxKind.ConstKeyword: | ||
| if (isConstValid) { |
There was a problem hiding this comment.
where is this checked if not here? I assume we're supposed to get an error for const on method declarations in both Javascript and Typescript.
There was a problem hiding this comment.
Why is const special?
As I see it this function reports Typescript syntax that is not valid in JS files and there's nothing TS specific about const.
What you really want is all grammar errors in TS and JS files. Hence I opened #29213 to propose exactly that.
There was a problem hiding this comment.
So the const modifier check is otherwise handled in the checker's grammar errors?
I read #29213, and it makes sense, although syntactic errors block semantic errors (or did at one time), so we'd need to test that the editor experience doesn't get worse.
However, removing this check here would then remove the error in non-ts-tcheck JS files until #29213 is fixed, right? If that's the case we should wait until that is done before making this change.
There was a problem hiding this comment.
I think at one point the logic of having grammar errors in semantic scenario was to be able to reuse the tree in incremental parsing and avoid extra pass as part of syntax diagnostics query. We would need to investigate if and what all changes are needed to even make sense if #29213 should be fixed or not.
|
This PR hasn't seen any activity for quite a while and is blocked on #29213, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion there. |
ConstKeywordis not specific to TypeScript syntax, therefore it doesn't need to be checked there.This function doesn't abort if cancellation is requested, so it doesn't need to be wrapped in
runWithCancellationToken. And even if it used the cancellation token, the checker wouldn't be in an invalid state.This change is best viewed without whitespace changes: https://github.com/Microsoft/TypeScript/pull/29208/files?w=1