Much better comment preservation #22141
Conversation
…ommentsOfPosition
When the transform was trivial it worked, but was unneeded, but when it was complex, it was brittle - best leave source mapping up to the transformers
|
(Psst, @evmar) |
src/compiler/emitter.ts
Outdated
| @@ -2399,7 +2399,7 @@ namespace ts { | |||
|
|
|||
| function emitEnumMember(node: EnumMember) { | |||
| emit(node.name); | |||
| emitInitializer(node.initializer); | |||
| emitInitializer(node.initializer, node.name.end + 1, node); | |||
There was a problem hiding this comment.
Why are these all plus one? Shouldn't the start position be the end of the name?
There was a problem hiding this comment.
Yep. This one (and like one other?) were the first ones I wrote and for some reason thought that I needed to skip the length the token in question. Thanks for reminding me that I still hadn't gone back and changed them.
There was a problem hiding this comment.
Maybe that's a good indicator that we need tests for cases where trivia comes immediately after:
enum E {
A/*what sick person even writes comments like this?*/=0
}
|
Have you run a benchmark for these changes against master? I want to be sure these changes don't cause a dramatic slowdown. |
|
|
src/compiler/emitter.ts
Outdated
| } | ||
|
|
||
| function emitCaseOrDefaultClauseStatements(parentNode: Node, statements: NodeArray<Statement>) { | ||
| function emitCaseOrDefaultClauseStatements(parentNode: Node, statements: NodeArray<Statement>, colonPos: number) { | ||
| // This also handles the colon token from the preceeding clause, as weather comments must be emitted for it depends on |
There was a problem hiding this comment.
nit: This comment is hard to read and has misspellings ('whether' not 'weather'), can you rephrase it?
There was a problem hiding this comment.
You can probably just remove the comment entirely if you rename the function to emitCaseOrDefaultClauseRest (as it no longer would imply only emitting the statements).
|
Can you address the merge conflict? |
This clears out some large swaths of #17689 (which is now much more precise now that I've taken an initial look on what should/should not be functioning right now), specifically:
YieldExpressionAwaitExpressionDeleteExpressionTypeOfExpressionVoidExpressionNewExpressionElementAccessExpressionPropertyAccessExpressionVariableDeclarationshould now all preserve any comments of any kind between any of their tokens, regardless of if the token is actually in the AST, so long as the node is sufficiently similar to its original form.
Just a reminder: Comment preservation is important for tools based on TS (like our own quickfixes) which expect to be able to extract, transform, and insert arbitrary ranges without modification. You can actually see improvements in our own
organizeImportsbaseline. 😄Things that might still not preserve comments correctly include any multitoken type nodes, and some TS-specific constructs (like namespaces and casts). I'm choosing to tackle those later (if at all) in a separate PR, since they need to be tested in a separate harness (as we have no transform-less emit option). Additionally, I am aware of a bug preventing multiline object literals from preserving comments after non-final commas (#10385). I'm leaving a fix for that out of this PR, since fixing it may end up requiring reworking
emitLista bit more than I want to when there's already a lot else going on.I don't think I have an exhaustive list, but I know this:
Fixes #11755
Fixes #2546 (all cases mentioned in the thread)
Fixes #17606
Fixes #6069 probably (provided I understand the original issue correctly)
Fixes #9873 (Which is actually a collection of issues)
Side note on why the changeset is so big: The sourcemap changes should be from changing
emitTokenWithCommentto no longer add an extra sourcemap entry for the token we're emitting comments for. Baselines were captured when this was used forreturnstatements, and while the baselines were technically "more precise" insofar as there were more mapping entries, typically the extra entries were for equal width spans (as a token always mapped to itself) making the extra entry rather pointless. That alone wouldn't usually be enough for me to care, but we actually have some places where the token does map to more text (when you consider non-returntokens), and the sourcemaps for those locations were always inaccurate (as the transformers hadn't setup corrected ones). So in lieu of updating the transformers to add mappings for all these tokens, I'm just leaving them all mapped as they are today, and revertingreturnto how it was more simply mapped in the past. If it'd help, I can extract that change into a separate PR so this one's easier to review once it's merged.