Fixed accidentally reused comments between files in the emitter #61261
Conversation
| return factory.updateImportTypeNode( | ||
| node, | ||
| factory.updateLiteralTypeNode(node.argument, rewriteModuleSpecifier(node, node.argument.literal)), |
There was a problem hiding this comment.
The issue was caused by factory.updateLiteralTypeNode calling its setTextRange. This one is specifically different from setTextRange used by reuseNode (check the comment above setTextRange in the checker.ts).
But it also means that likely all factory.update* methods used in this file here are susceptible for similar bugs. At least some of them are using the internal update function that calls setTextRange. There is a chance though this bug was extra tricky because LiteralType and StringLiteral here have the very same positions and only StringLiteral was correctly stripped from its positions before. So maybe the other update methods are safe-ish.
There was a problem hiding this comment.
The factories doing anything with locations sounds very very cursed; but I can very much see update being used all over the place inside of the factory. So, not sure what to think there. I'd personally prefer if we didn't set ranges on nodes that didn't come out of real parsed source.
|
|
||
| //// [a.d.ts] | ||
| export declare const _: { | ||
| foo: import("./id").Id<{}>; |
There was a problem hiding this comment.
I'm slightly surprised this one gets rewritten to "./id" (this test is using rewriteRelativeImportExtensions). IIRC I couldn't make it to be rewritten to anything else with any module-related options but maybe I have not tried hard enough. I guess it might behave differently from what rewriteRelativeImportExtensions promises us normally because it's in a type node.
|
Is it correct to say that the only harm done was to have extra comments that shouldn't have been there in declaration files? |
|
Yeah, I think so. cc @MichaelMitchell-at to confirm |
Unfortunately the comments could be inserted in between tokens such that it produced invalid syntax, similar to the previous issue from last year. |
|
Thanks for the quick turnaround @Andarist! I'll check if this resolves the more complex scenario in our code base. |
|
Do you have an example of this occurring? The example you gave just inserted some extra block comments between nodes, so didn't seem harmful. |
It happens in our internal code, but I wasn't able to quite isolate a reproduction that could demonstrate that behavior too. I figured that getting a repro that demonstrated that there was some issue would hopefully suffice in producing a fix for our case too. |
|
I would still like to come up with a test that shows it, then, given that sort of impacts what we think is a backportable bugfix (into say, the yet-to-be-released 5.8) or not. |
I'll give another shot at producing one today |
|
Ok! I managed to reproduce it: // @strict: true
// @declaration: true
// @showEmit
// @showEmittedFile: a.d.ts
// @filename: a.ts
import { object } from "./obj";
import { id } from "./id";
export const _ = object;
/**
*/
// @filename: obj.d.ts
import { id } from "./id";
// ----
export declare const object: id.A<{
foo: id.A<1>
}>;
// @filename: id.d.ts
export declare namespace id {
type A<T> = T;
} |
|
@Andarist added a test case on top of your PR here: Andarist#2 |
fixes #61239