Token hints for missing closing braces: classes, enums, jsx, modules, types #37497
Conversation
|
cc @sheonhan |
|
edit: done |
|
@andrewbranch @DanielRosenwasser is there any additional action I could take to get a review on this? |
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at f2d57da. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at f2d57da. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at f2d57da. You can monitor the build here. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
… types
This commit adds token hints for missing close braces in
- Class definitions
- Enum definitions
- JSX expressions
- Module definitions (this includes augmentations and namespaces)
- Type member lists (this includes interfaces)
The token hint refers to the opening brace for which the parser expected
a corresponding closing brace.
Note that this commit *does not* update all occurences of an expectation
for a close brace to provide token hints. Constructs for which this
token hint is not provided include:
- Mapped types
- Switch statements
- Object binding patterns
- JSX spread attributes (`<div {...props}></div>`)
- JSDoc implements/augments tags (`/* @implements {SomeInterface} */`)
Part of microsoft#35597.
There was a problem hiding this comment.
@DanielRosenwasser I guess the extended tests didn’t have any unbalanced braces. Code and baselines look good to me. Anyone else want to review first?
I figured, just wanted to make sure that we weren't introducing any weird behavioral regressions |
|
@weswigham perf tests seem to have had an issue. Any clue?
Do we just need to pass |
|
I pinged @rbuckton in one of my PRs about the same thing; I assumed it's because the version of |
There was a problem hiding this comment.
As it should've been obvious, the previous approach is too hacky and mucks with non-local state that it can't be sure it created.
Consider making parseErrorAtPosition return the diagnostic it creates if it can. Then wire up parseExpectedCloseToken to use that, directly or indirectly so that it doesn't have to reach into the diagnostics list.
| @@ -14,6 +14,7 @@ tests/cases/compiler/classMemberWithMissingIdentifier2.ts(3,1): error TS1128: De | |||
| !!! error TS1146: Declaration expected. | |||
| ~ | |||
| !!! error TS1005: ';' expected. | |||
| !!! related TS1007 tests/cases/compiler/classMemberWithMissingIdentifier2.ts:1:9: The parser expected to find a '}' to match the '{' token here. | |||
There was a problem hiding this comment.
So it looks like the heuristic on this is actually...not working correctly given
TypeScript/src/compiler/parser.ts
Line 1136 in 7f59949
|
@DanielRosenwasser I initially had the same thought about looking at |
Then I think those functions are also bad 🙂 |
|
The perf test issue should be resolved, I think. |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 35cd0e4. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - master..37497
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Just to be specific, the reason I think so is that
is actually untrue! See the reference over at #37497 (comment). We'll return TypeScript/src/compiler/parser.ts Lines 1135 to 1140 in 7f59949 I believe the de-duping logic means that you really can never rely on the most recent error in the list, which was always a super sketchy approach anyway. |
|
Ahh, I wasn’t looking all the way into
I do too, I just try to avoid refactoring code that’s ostensibly been working fine for a long time. But now it definitely looks like it’s not working fine. My bad. Carry on. |
|
@DanielRosenwasser @andrewbranch I can't tell what the state of this PR is. Is it the right approach? Does it need to be reworked entirely? If we're waiting for changes from @ayazhafiz, what are the changes? The long comment history made it too hard for me to figure any of this out. |
|
It needs a refactor in the parser which, on its face, would appear unrelated to the actual goal here, and I’m unsure how big of a refactor that is. Daniel described what needs to happen here, and his review points out an incorrect test baseline that exhibits why the current approach is unreliable. |
|
@ayazhafiz Would you like to attempt the refactor? I'll close this PR soon if not. |
|
No, I do not have time to do this presently. |
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
This commit adds token hints for missing close braces in
The token hint refers to the opening brace for which the parser expected
a corresponding closing brace.
Note that this commit does not update all occurences of an expectation
for a close brace to provide token hints. Constructs for which this
token hint is not provided include:
<div {...props}></div>)/* @implements {SomeInterface} */)Part of #35597.