Ensure that JSDoc parsing happens within a ParsingContext #52710
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Milestone Bug
jakebailey
commented
Feb 10, 2023
| @@ -25,7 +25,7 @@ | |||
| // zee(''); | |||
| // ^ | |||
| // | ---------------------------------------------------------------------- | |||
| // | zee(**arg0: any**, arg1: any, arg2: any): any | |||
| // | zee(**arg0: any**, arg1: any): any | |||
There was a problem hiding this comment.
This function is:
/** @type {function(module:xxxx, module:xxxx): module:xxxxx} */
function zee() { }
Clearly, this has two parameters (not three!), and now we get that right.
jakebailey
commented
Feb 10, 2023
| /a.js(1,14): error TS1139: Type parameter declaration expected. | ||
| /a.js(1,17): error TS1003: Identifier expected. | ||
| /a.js(1,17): error TS1110: Type expected. | ||
| /a.js(1,17): error TS8024: JSDoc '@param' tag has name '', but there is no parameter with that name. |
There was a problem hiding this comment.
This function is:
/** @param {<} x */
function f(x) {}
Clearly this has a parameter named x, and now we no longer spuriously complain about that.
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at bd85412. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at bd85412. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at bd85412. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at bd85412. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at bd85412. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at bd85412. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at bd85412. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailsaxios-src
|
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
jakebailey
commented
Feb 10, 2023
| @@ -1,6 +1,6 @@ | |||
| === /a.js === | |||
| /** @param {<} x */ | |||
| function f(x) {} | |||
| >f : (x: any) => void | |||
There was a problem hiding this comment.
IDK if this is bad or not; this is clearly malformed JSDoc but before we just got any. Now we think we're starting type parameters (which is normal for parsing a type) and then assume it's probably a function signature which is the only thing that can start with < like this.
jakebailey
commented
Feb 10, 2023
Comment on lines
+7224
to
+7225
| // TODO(jakebailey): this is totally wrong; `parsingContext` is the result of ORing a bunch of `1 << ParsingContext.XYZ`. | ||
| // The enum should really be a bunch of flags. |
There was a problem hiding this comment.
All of the other uses get this right, at least, so node reuse has feasibly only been broken for ambient declarations.
There was a problem hiding this comment.
You're right - what happens if you pass in 0b111111111? Which tests break? My hope is that something tested node reuse in an ambient context.
There was a problem hiding this comment.
Passing in 0b111111111 won't change anything because it uses equality checks, so they'll all just end up being false and no node reuse will occur.
There was a problem hiding this comment.
won't change anything because it uses equality checks, so they'll all just end up being false and no node reuse will occur.
But we have tests that ensure that some node reuse occurs, don't we?
There was a problem hiding this comment.
All fourslash server tests which edit their files are incremental, which covers things in some aspect, but the incrementalParser.ts unittest file doesn't appear to test ambient declarations too much.
There was a problem hiding this comment.
Can you add an incremental test with an ambient context where node reuse should happen, and which fails when you pass in 0b11111111?
There was a problem hiding this comment.
In trying to make a test case for this, I've found that this can only be a problem if there's an ambient declaration inside of something else, like:
{
declare module "foo" {
// ...
}
}This is technically illegal code, and this bug here means we never use any nodes from this tree.
Ambients are only being reused now because they can only be a child of the SourceFile itself, and its ParsingContext happens to be ParsingContext.SourceElements = 0. Then, the top level variable has value 1 << 0 or 1, which is ParsingContext.BlockStatements which just so happens to also be reusable, so it works.
There was a problem hiding this comment.
So, I could fix the bug, but it'd only fix broken trees that probably never happen. But it's still totally wrong and only works because of a fluke!
There was a problem hiding this comment.
Okay, I'm wrong, the "ambient in a block" also works because of a fluke; you get parsingContext 0b11 which equals ParsingContext.SwitchClauseStatements and that is also reusable, so the code passes. 🙃
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsakveo/ngx-admin
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsconwnet/github1s
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsjhipster/generator-jhipster
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsnestjs/nest
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailspnpm/pnpm
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsremix-run/react-router
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsvercel/swr
|
|
@jakebailey Here they are:
CompilerComparison Report - main..52710
System
Hosts
Scenarios
TSServerComparison Report - main..52710
System
Hosts
Scenarios
StartupComparison Report - main..52710
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -3337,6 +3347,7 @@ namespace Parser { | |||
| case ParsingContext.JsxAttributes: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); | |||
| case ParsingContext.JsxChildren: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); | |||
| case ParsingContext.AssertEntries: return parseErrorAtCurrentToken(Diagnostics.Identifier_or_string_literal_expected); // AssertionKey. | |||
| case ParsingContext.JSDocComment: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); | |||
There was a problem hiding this comment.
What does it mean to provide an error in a JSDoc parsing comment? Where would we issue this?
There was a problem hiding this comment.
This is part of recovery in parseList/parseDelimitedList and feasibly only only happens when a list ends prematurely.
There was a problem hiding this comment.
You probably don't need an error but it's fine. In a follow-up PR, I'd be curious to see what happens if it Debug.fail()s.
There was a problem hiding this comment.
Huh, yeah, you can just Debug.fail() and this PR also works. Maybe I'll just do that.
But, I am probably close to eliminating this entire parse mode entirely too.
There was a problem hiding this comment.
Probably this doesn't break because the tests aren't good enough.
|
I would rename badJSDocNoCrash.ts - maybe just Also, this is only repro-able in the language service? Not the standard compiler? |
Sure, yeah. That name is a relic of it being a crash to start until I found the real problem.
It's not reproable in the compiler because the compiler only ever parses source files. In services, we re-parse just the JSDoc comment via But, honestly, that might be wrong too; we should have already parsed the JSDoc, so why are we doing it again? Maybe that's the real problem? |
|
See |
|
(I'm OOF starting in like half a hour so probably won't screw with this much until next week.) |
|
Okay before I head out I was able to get most things working by eliminating this extra parse and instead just using the JSDoc node already, so I think that might actually be a fix here. |
|
I've sorta gotten the "don't reparse JSDoc" version of this working, though not completely. However, since this is for the "old" classifier and not v2020, should I even go through with that effort? The code change would be entirely constrained to that classifier code that I don't even know people use anymore. |
|
I sent #52795 for the "use the already-parsed JSDoc" version, though, I'm not 100% sure if that one's good or not. This PR RE: ParsingContext does expose some real bugs, but the one that this bug fixes regarding not having a context is rendered moot in #52795 because the code is never actually run anymore. And, nether PR fixes the node reuse bug to do with contexts, which I need to address separately anyway. |
sandersn
approved these changes
May 4, 2023
| case ParsingContext.Count: | ||
| return Debug.fail("ParsingContext.Count used as a context"); // Not a real context, only a marker. | ||
| default: | ||
| Debug.assertNever(parsingContext, "Non-exhaustive case in 'isListElement'."); |
There was a problem hiding this comment.
how did this (mostly) work before? Do we never callisListElement inside jsdoc? (Unlikely). Did currentNode(parsingContext) always return a value, so the switch never executed? Was there some other incorrect parsing context set during jsdoc parsing, except for the bug case?
There was a problem hiding this comment.
This code was actually never hit for JSDoc, because the loop in isInSomeParsingContext checks each bit and then uses it. Unlike source files (which are always inside of a statement list), the standalone JSDoc parser was not, and so its parsing context was zero, and skipped all of this code (which was the bug).
There was a problem hiding this comment.
Another way to think about this particular block is that it's checking equiality on the context, which are values like { 1, 2, 3, 4 ... }... but the context actually stored in the parser are actually 1 << { 1, 2, 3, 4 ... }, so anyone trying to use this code has to un-flag-ify the value before using it, and that's the other broken thing I found in this code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #47537
aaaaaaaaa
Basically, I noticed that we parse out the JSDoc tag twice (????), once at the initial parse, once later at JSDoc parse during classification.
The first time, parsing fails because we're looking for a type literal and we have
{dot|fulltext}which is missing a colon after the name "dot". Cool, that makes sense.The second time, parsing succeeds somehow??? Whuhhh?
It succeeds because now we're parsing via the JSDoc parser, and so
parsingContextis zero, so we don't know that we shouldn't be eating random tokens and do weird things. It's that eating of random tokens that causes the debug assert because we're skipping stuff and it's going into trivia (then we decide to parsefulltextas a signature (????), even though that's not how it parses normally. "Normally" we are coming fromparseSourceFileand the first thing it does is enter into theSourceElementscontext, so it's only JSDoc that doesn't have a context.So, I forced
parsingContextto be nonzero so that we are always in some parsing context. Sure, that's fine I guess.And surprise, we have a few good baseline changes because of this. Very weird.
But, then as I was inspecting
parsingContext, I noticed that its type is a total lie. It's actually holding1 << ParsingContext.Foo | 1 << ParsingContext.bar | ..., not actualParsingContextvalues. But, we have some code that assumes that it doesn't do that! So, there are some random TODOs because there must be more broken here but that's probably another PR. Probably.