improve stripInternal with inline comments #23611
Conversation
|
@weswigham can you please review. |
src/compiler/utilities.ts
Outdated
| @@ -758,6 +758,10 @@ namespace ts { | |||
| return node.kind !== SyntaxKind.JsxText ? getLeadingCommentRanges(sourceFileOfNode.text, node.pos) : undefined; | |||
| } | |||
|
|
|||
| export function getTrailingCommentRangesOfNode(node: Node, sourceFileOfNode: SourceFile) { | |||
There was a problem hiding this comment.
This is misleadingly named - the trailing comment ranges of the node would be the trailing comment ranges of the end of the node - this is the trailing comment ranges working backwards from the beginning of the node - which are actually the trailing comment ranges of the end of the prior token. Also, consider the quite crazily formatted:
class A {
constructor(
public foo: string, /* @internal */
// This next one
public bar: number,
) {}
}I don't think that should count as an internal annotation for the second parameter declaration, due to the intervening single-line comment. (On a related note, I'd love to see a few more tests for some odd internal comment locations around parameter properties (esp. mixed with single-line comments) with this change).
|
need some help: |
|
Looks like our declaration file output has errors in it (we use |
Kingwl
force-pushed
the
fix-stripInternal
branch
from
64185f3 to
5addfab
Compare
src/compiler/types.ts
Outdated
| // A name that supports late-binding (used in checker) | ||
| /* @internal */ |
There was a problem hiding this comment.
On second thought, changing the logic such that the @internal annotation must be immediately preceding the node isn't a change we probably want to make lightly; I'd keep the old behavior if you're not looking for an inline annotation.
src/compiler/scanner.ts
Outdated
| let pendingPos: number; | ||
| let pendingEnd: number; | ||
| let pendingKind: CommentKind; | ||
| let pendingHasTrailingNewLine: boolean; | ||
| let hasPendingCommentRange = false; | ||
| let collecting = trailing || pos === 0; | ||
| let collecting = inline || trailing || pos === 0; |
There was a problem hiding this comment.
Don't change this. I assure you, though the terminology is confusing, it is at least consistent. What you want to change instead is the position you begin scanning from, waaaaay up the callstack closer to shouldStripInternal. Rather than looking for the node's leading or trailing comments, you want the trailing comments of the first non-trivia position before the node. For example, in:
(/* whatever*/ Foo);The /*whatever*/ comment is neither a leading nor trailing comment of the Foo identifier. Nor is is a leading or trailing comment of the parenthesized expression/argument list. What it is is a trailing comment of the ( - the OpenParen token. This token doesn't actually exist in our AST, so recovering comments on it can be difficult. But in this case it's not hard - you just want the trailing comments of the argument list start for the first parameter, or the trailing comments of the end of the previous parameter + skip trivia length + 1 if it's not the first param. Our emitter has a bunch of code that does similar stuff to preserve comments on these tokens - checkout the emitTrailingCommentsOfPosition and emitLeadingCommentsOfPosition calls in emitNodeList in emitter.ts.
| @@ -7658,7 +7658,6 @@ declare namespace ts.server { | |||
| */ | |||
| type ProjectRoot = ScriptInfo | NormalizedPath; | |||
| abstract class Project implements LanguageServiceHost, ModuleResolutionHost { | |||
| readonly projectName: string; | |||
There was a problem hiding this comment.
seems something has been lgnored
There was a problem hiding this comment.
Yeah, looks exactly like an instance of the bug you're fixing that we never noticed :D
|
⬆️ |
|
@weswigham & @rbuckton 👍 / 👎 ? |
Fixes #23589