Add the 5 modifier jsdoc tags#927
Conversation
`@public @Private @Protected @readonly @override` Three notes: - I did not add support for BinaryExpressions yet because they don't have a Modifiers list. I think the right thing is to add it (along with a Type node), but I want to bring it up at a design meeting first. - I dropped the error messages with tag-specific wording for `@override`. This seems unnecessary to me -- people who use JS as TS are aware that's what they're doing. - I added a setModifiers method on nodeData and exposed it via a public AsMutable method on Node. I don't know if that's exactly the right way to do it.
internal/parser/reparser.go
Outdated
| if declaration.AsVariableDeclaration().Type == nil { | ||
| declaration.AsVariableDeclaration().Type = p.makeNewType(tag.AsJSDocTypeTag().TypeExpression, declaration) | ||
| break | ||
| switch parent.Kind { |
There was a problem hiding this comment.
this is a drive-by reformat. As usual, best reviewed ignoring whitespace.
There was a problem hiding this comment.
Pull Request Overview
Adds support for five new JSDoc tags by reparsing comments into actual TypeScript modifiers.
- Parses
@public,@private,@protected,@readonly, and@overridetags and injects corresponding modifiers into the AST. - Adjusts grammar checks to recognize modifiers inserted by the reparser and avoid spurious ordering errors.
- Extends the AST API to allow mutation of modifier lists on nodes.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/parser/reparser.go | Add JSDoc‐tag cases to insert matching modifiers on declarations |
| internal/checker/grammarchecks.go | Skip ordering errors for modifiers added via JSDoc reparse |
| internal/ast/ast.go | Introduce AsMutable() + setModifiers() for AST nodes |
| testdata/baselines/reference/submoduleAccepted/conformance/jsdocReadonly.errors.txt.diff | Remove obsolete readonly‐property error baseline |
| testdata/baselines/reference/submoduleAccepted/conformance/jsdocOverrideTag1.errors.txt.diff | Update override‐tag1 error expectations |
| testdata/baselines/reference/submoduleAccepted/conformance/jsdocAccessibilityTags.errors.txt.diff | Clean up accessibility‐tag error baselines |
| testdata/baselines/reference/submodule/conformance/uniqueSymbolsDeclarationsInJsErrors.types | Expect unique symbol for readonly‐unique symbol props |
| testdata/baselines/reference/submodule/conformance/uniqueSymbolsDeclarationsInJsErrors.errors.txt | Drop duplicate unique‐symbol error |
| testdata/baselines/reference/submodule/conformance/uniqueSymbolsDeclarationsInJs.types | Reflect unique symbol type in all readonly symbol cases |
| testdata/baselines/reference/submodule/conformance/uniqueSymbolsDeclarationsInJs.errors.txt | Remove outdated unique‐symbol error baseline |
| testdata/baselines/reference/submodule/conformance/privateNamesIncompatibleModifiersJs.errors.txt | Add tests for private‐identifier modifier conflicts |
| testdata/baselines/reference/submodule/conformance/override_js4.errors.txt | New test for typo suggestion in override error |
| testdata/baselines/reference/submodule/conformance/override_js3.errors.txt | Remove obsolete JS3 override baseline |
| testdata/baselines/reference/submodule/conformance/override_js2.errors.txt | Expand override tests in JS2 |
| testdata/baselines/reference/submodule/conformance/override_js1.errors.txt | Expand override tests in JS1 |
| testdata/baselines/reference/submodule/conformance/jsdocReadonlyDeclarations.types | Update declaration‐site readonly types to literal values |
| testdata/baselines/reference/submodule/conformance/jsdocReadonly.types | Update readonly property types and instance assignment types |
| testdata/baselines/reference/submodule/conformance/jsdocReadonly.errors.txt | Add readonly‐assignment error baseline |
| testdata/baselines/reference/submodule/conformance/jsdocOverrideTag1.errors.txt | Update override‐tag1 error baselines |
| testdata/baselines/reference/submodule/conformance/jsdocAccessibilityTags.errors.txt | Add accessibility‐tag error baseline |
internal/parser/reparser.go
Outdated
| case ast.KindJSDocReadonlyTag, ast.KindJSDocPrivateTag, ast.KindJSDocPublicTag, ast.KindJSDocProtectedTag, ast.KindJSDocOverrideTag: | ||
| switch parent.Kind { | ||
| case ast.KindPropertyDeclaration, ast.KindMethodDeclaration, ast.KindGetAccessor, ast.KindSetAccessor: | ||
| // !!! BinaryExpression (this.p assignments) |
There was a problem hiding this comment.
[nitpick] Consider implementing handling for BinaryExpression parent nodes (e.g. this.property = ... inside constructors) so that JSDoc modifiers on assignments are consistently applied, or remove the stray comment if unneeded.
| // !!! BinaryExpression (this.p assignments) | |
| // Handle JSDoc modifiers for declarations |
There was a problem hiding this comment.
I can't implement handling until I decide how to store modifiers for this.p=. Waiting on a decision at a design meeting.
| l.y = 12 | ||
| >l.y = 12 : 12 | ||
| >l.y : number | ||
| >l.y : any | ||
| >l : LOL | ||
| >y : number | ||
| >y : any | ||
| >12 : 12 |
There was a problem hiding this comment.
The type of l.y has been widened to any after applying @readonly. It should remain number (or the literal type) to preserve type safety and avoid losing information.
There was a problem hiding this comment.
No, it's because y is readonly now, so assignments to it are not allowed.
internal/parser/reparser.go
Outdated
| nodes = append(parent.Modifiers().Nodes, modifier) | ||
| loc = parent.Modifiers().Loc | ||
| } | ||
| parent.AsMutable().SetModifiers(p.newModifierList(loc, nodes)) |
There was a problem hiding this comment.
Does this ever happen on any parent kind, or is there a specific subset of nodes that do this? I'm not entirely certain how I feel about the AsMutable pattern, so curious if it's just shorthand for a couple of types.
There was a problem hiding this comment.
It is the 5 node kinds you see here (including BinaryExpression), unless I missed 1 or 2. Would you prefer a specific SetModifiers on those types?
There was a problem hiding this comment.
In a way I think it'd be fine to just have a top level ast package function with a switch case in it; just wasn't sure if it was that simple or if the method dispatch here was simplifying things.
There was a problem hiding this comment.
It is the 5 you see here: class members and expando class members. Based on experience, I predict were going to need this to generalise someday, but definitely not today.
2 participants
@public @private @protected @readonly @overrideThree notes:
@override. This seems unnecessary to me -- people who use JS as TS are aware that's what they're doing.to do it.