Use regex+getTokenAtPosition to find dynamic import #28104
Conversation
Instead of walking the entire tree. This stack overflows for large trees. Still need to adapt a test.
| function collectDynamicImportOrRequireCallsForEachChild(node: Node) { | ||
| forEachChild(node, collectDynamicImportOrRequireCalls); | ||
| /** Returns a token if position is in [start-of-leading-trivia, end) */ | ||
| function getTokenAtPosition(sourceFile: SourceFile, position: number): Node { |
There was a problem hiding this comment.
does this differ from getTokenAtPosition in services/utitilities.ts? if not, it could be moved to compiler/utilities.ts and also used for services
There was a problem hiding this comment.
Yes, it uses Node.pos and Node.end instead of the much-more-complicated Node.getStart() and Node.getFullStart() that are present in the language service. The original getTokenAtPosition handles a lot more cases as well; I'd rather wait until something else in the compiler proper needs this code to move it into utilities and harden it.
There was a problem hiding this comment.
I see. In addition it doesn't use node.getChildren to get the actual token. So at least it should be renamed to getNodeAtPosition to avoid confusion.
| if (hasJSDocNodes(node)) { | ||
| forEach(node.jsDoc, collectDynamicImportOrRequireCallsForEachChild); | ||
| function collectDynamicImportOrRequireCalls(file: SourceFile) { | ||
| const r = /import|require/g; |
There was a problem hiding this comment.
you could tweak the regex to avoid false positives. though you need to adjust the loop a bit to save the result of r.exec to a variable.
/\b(?:import|require)\s*[(/<]/gThe above will not match require.resolve for example as it requires an opening parenthesis or a slash (typically the start of a comment) after it.
There was a problem hiding this comment.
I don't think we'd save much in the majority of files, and I prefer to have the simplest possible regex.
There was a problem hiding this comment.
Since this PR is about performance, I'd rather we are careful to not regress performance where possible.
There is require.resolve, import.meta, in a project of mine there's a lot requiresTypeInformation and in addition these are regular verbs that could occur in a lot of comments.
Each time you encounter something that contains one of these words, you need to recursively look up the node in the AST. That can potentially be very slow for large files with deep nesting.
There was a problem hiding this comment.
Well, the new collector is m log(n) where m is the number of import|require and n is the number of nodes. The old collector is n log(n), and I think m will only approach n in pathological cases.
Also, the compiler is going to crash on very deep ASTs in the binder anyway. I tried to fix that by adding a flat OperatorListExpression to replace deep BinaryExpression trees, but gave up (for now) because of the amount of work needed to update all our BinaryExpression-based code.
| } | ||
| }; | ||
| while (true) { | ||
| const child = hasJSDocNodes(current) && forEach(current.jsDoc, getContainingChild) || forEachChild(current, getContainingChild); |
There was a problem hiding this comment.
probably unrelated to this change: IMO it should only look at JSDoc in JS files as ImportTypeNodes in JSDoc of TS files have no effect.
Use regex+getTokenAtPosition to find dynamic imports (and requires) instead of walking the entire tree. A tree walk overflows the stack for very deep trees. I assume, though I have not measured it, that this method is much faster for JS files, which may not have any occurrences of the words
importorrequire, but would previously be walked.All existing tests pass, and the stack overflow in #27433 no longer repros. However, a test based on the large JS file in that repo now fails in the binder. So I'll leave #27433 open for now even though technically this change makes antd-admin compile successfully.