Skip to content
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

Adding Diagnostic message for missing ']' and ')' in Array literal and conditional statements #40884

Merged
merged 5 commits into from Mar 30, 2021

Conversation

keerthana1212
Copy link
Contributor

Fixes #35597

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Oct 1, 2020
@ghost
Copy link

ghost commented Oct 1, 2020

CLA assistant check
All CLA requirements met.

@keerthana1212 keerthana1212 changed the title Adding Diagnostic message for missing ']' in Array literal Adding Diagnostic message for missing ']' and ')' in Array literal and conditional statements Oct 2, 2020
Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋🏻 this is solid work @keerthana1212 - I think it's probably time to reduce the duplication though as this logic is repeated 5 times in the codebase (if merged) (the fifth is in #35586 )

I think you probably want a helper function like:

function parseExpectedWithRecovery(openKind: SyntaxKind, closeKind: SyntaxKind, openPosition: number) {
  if (!parseExpected(openKind)) {
    const lastError = lastOrUndefined(parseDiagnostics);
    if (lastError && lastError.code === Diagnostics._0_expected.code) {
      addRelatedInfo(
        lastError,
        createDetachedDiagnostic(fileName, openBracketPosition, 1, Diagnostics.The_parser_expected_to_find_a_1_to_match_the_0_token_here, tokenToString(openKind), tokenToString(closeKind))
      );
    }
    return false;
  }
  return true;
}

and to call that instead of parseExpected in the parseX functions

@sandersn sandersn added this to Not started in PR Backlog Oct 8, 2020
@sandersn sandersn moved this from Not started to Waiting on author in PR Backlog Oct 9, 2020
@sandersn
Copy link
Member

@keerthana1212 do you want to keep working on this? If not, one of us may finish it up.

@keerthana1212
Copy link
Contributor Author

keerthana1212 commented Mar 16, 2021 via email

@sandersn sandersn moved this from Waiting on author to Not started in PR Backlog Mar 16, 2021
@sandersn sandersn merged commit 555ef73 into microsoft:master Mar 30, 2021
PR Backlog automation moved this from Not started to Done Mar 30, 2021
@DanielRosenwasser
Copy link
Member

I think this needs a revert - see the explanation here:

#43005 (comment)

@sandersn
Copy link
Member

So, undo #35586 as well?

@sandersn
Copy link
Member

I looked closer at the code. There are 3 places that look at the last error on the stack.

  1. parseErrorAtPosition. This is a longstanding, context-free check to avoid many errors on the same location. It's fine, if ad-hoc. It's the one causing the other problems.
  2. parseExpectedMatchingBrackets. I think this has the same problem that you pointed out in Token hints for missing closing braces: classes, enums, jsx, modules, types #37497.
  3. parseTypedefTag. This probably has the same problem, but I'm not sure if I could create a repro for it. And it's @typedef, which is a syntactic dumpster fire anyway.

I don't think a revert is the best solution here.
There are couple of other fixes I thought of:

  1. Make parseErrorAtPosition and friends return DiagnosticWithDetactedLocation | true | undefined where true means a dupe and undefined means no error. This is the best fix as long as it isn't too slow.
  2. Make parseErrorAtPosition overwrite dupes so that it keeps the last instead of the first.

@sandersn
Copy link
Member

I tried (2) since it's a simple change and the errors are either the same or worse. They're worse in places where the parser hasn't gone too far off the rails, so it's not a good fix.

@sandersn
Copy link
Member

(1) makes a lot of improvements to baselines. I'll put it up and run the perf tests on it.

jessetrinity added a commit to jessetrinity/TypeScript that referenced this pull request May 19, 2021
…teral and conditional statements (microsoft#40884)"

This reverts commit 555ef73.
jessetrinity pushed a commit that referenced this pull request May 19, 2021
* Revert "Only issue matching token errors on non-dupe locations (#43460)"

This reverts commit 76a2ae3.

* Revert "Adding Diagnostic message for missing ']' and ')' in Array literal and conditional statements (#40884)"

This reverts commit 555ef73.

* re-add clobbered merge lines
jessetrinity pushed a commit to jessetrinity/TypeScript that referenced this pull request May 20, 2021
* Revert "Only issue matching token errors on non-dupe locations (microsoft#43460)"

This reverts commit 76a2ae3.

* Revert "Adding Diagnostic message for missing ']' and ')' in Array literal and conditional statements (microsoft#40884)"

This reverts commit 555ef73.

* re-add clobbered merge lines
DanielRosenwasser pushed a commit that referenced this pull request May 20, 2021
* Revert "Only issue matching token errors on non-dupe locations (#43460)"

This reverts commit 76a2ae3.

* Revert "Adding Diagnostic message for missing ']' and ')' in Array literal and conditional statements (#40884)"

This reverts commit 555ef73.

* re-add clobbered merge lines
jessetrinity added a commit to jessetrinity/TypeScript that referenced this pull request May 27, 2021
sandersn added a commit that referenced this pull request Mar 16, 2022
…brackets (#44158)

* Revert "Revert #43460 and #40884 (#44175)"

This reverts commit 5770434.

* fix missing opening brace match error

* refactor parseExpectedMatchingBrackets

* use getNodePos

* accept baselines

* delete mistakenly added files

* Revert getNodePos addition

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Generalize missing token hints to more constructs
6 participants