JS: Separate JSDoc qualified names into individual identifiers #19077
Conversation
asgerf
force-pushed
the
js/jsdoc-name-tokens
branch
2 times, most recently
from
cabc847 to
b140e02
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the representation of JSDoc qualified names so that each identifier in a qualified name is represented as its own node instead of a single aggregated node. Key changes include:
- Introduction of a new QualifiedNameExpression class to represent qualified names.
- Replacement of the old NameExpression with Identifier and QualifiedNameExpression throughout the parser and extractor.
- Update of the extractor version to reflect these changes.
Reviewed Changes
Copilot reviewed 10 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| javascript/extractor/src/com/semmle/js/ast/jsdoc/QualifiedNameExpression.java | New class to represent qualified names by splitting identifiers |
| javascript/extractor/src/com/semmle/js/extractor/Main.java | Updated extractor version |
| javascript/extractor/src/com/semmle/js/parser/JSDocParser.java | Replaced NameExpression with Identifier and added support for qualified names |
| javascript/extractor/src/com/semmle/js/ast/jsdoc/Identifier.java | Renamed NameExpression to Identifier and updated documentation accordingly |
| javascript/extractor/src/com/semmle/js/extractor/JSDocExtractor.java | Updated type mappings and visitor methods to work with new Identifier and QualifiedNameExpression types |
| javascript/extractor/src/com/semmle/js/ast/jsdoc/Visitor.java | Updated visitor interface to reflect the new type names |
Files not reviewed (11)
- javascript/downgrades/ccefb5e2d49318eea4aeafd4c6ae2af9f94ac72a/upgrade.properties: Language not supported
- javascript/extractor/tests/comments/output/trap/jsdoc.js.trap: Language not supported
- javascript/ql/lib/semmle/javascript/DOM.qll: Language not supported
- javascript/ql/lib/semmle/javascript/Externs.qll: Language not supported
- javascript/ql/lib/semmle/javascript/JSDoc.qll: Language not supported
- javascript/ql/lib/semmlecode.javascript.dbscheme: Language not supported
- javascript/ql/lib/semmlecode.javascript.dbscheme.stats: Language not supported
- javascript/ql/lib/upgrades/5b5db607d20c7b449cef2d1c926b24d77c69bebb/upgrade.properties: Language not supported
- javascript/ql/test/library-tests/JSDoc/NameResolution/test.expected: Language not supported
- javascript/ql/test/library-tests/JSDoc/Nodes/tests.expected: Language not supported
- javascript/ql/test/library-tests/TypeAnnotations/JSDoc/JSDocTypeAnnotations.expected: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| description: split up qualified names in jsdoc type exprs | ||
| compatibility: backwards |
There was a problem hiding this comment.
So every existing jsdoc_named_type_expr gets converted to a jsdoc_identifier_type_expr?
Sounds fine to me.
And I suppose an upgrade script to split the identifiers is impossible.
There was a problem hiding this comment.
Changed the compatibility to partial to reflect that it won't be the same as a freshly built database but will mostly work
| description: split up qualified names in jsdoc type exprs | ||
| compatibility: backwards |
There was a problem hiding this comment.
So here I suppose all jsdoc_qualified_type_expr are discarded?
There was a problem hiding this comment.
Yeah that's not good as nodes in the middle of the tree will effectively disappear. I've pushed a proper downgrade script and tested it.
Test updates look OK. Some intermediate results are omitted but the qualified name of the final type names are still present.
The call to expect() below here updates 'token' and 'value' to that of the NEXT token (not the name). The code happened to work because the 'value' field is only updated if a token with a relevant value is found. E.g. if a name token could be followed by another name, then we would have seen the wrong name here.
asgerf
force-pushed
the
js/jsdoc-name-tokens
branch
from
2d0ef6a to
441ca1c
Compare
|
Sorry about the force-push; I had to rebase the downgrade script earlier in history to test it, and after I put it back to its original place the SHA of all the commits changed even though their contents are the same 🤷 |
Changes the underlying representation of
JSDocNamedTypeExprso each identifier in a qualified name has its own node. This aligns with how such types are represented in TypeScript type annotations.Previously
Foo.Barwas a single node, where as nowFooandBarare now identifier nodes wrapped in a qualified type name node.Before:
(JSDocNamedTypeExpr) Foo.BarAfter:
(JSDocQualifiedTypeExpr)getBase()(JSDocLocalTypeAccess) FoogetNameNode()(JSDocIdentifierTypeExpr) BarThe names
JSDocQualifiedTypeAccessandJSDocLocalTypeAccesswere chosen to align with theQualifiedTypeAccessandLocalTypeAccessclasses, which were themselves chosen to align withVarAccess.For context, the reason for this change is that we're moving name/type resolution out of the TypeScript extractor into plain QL. We already have a very basic name resolution for JSDoc types in QL and I'd like to ensure that the new implementation can replace the JSDoc name resolution so we have a shared implementation for JSDoc and TypeScript. This is easier to achieve if the representations are similar.