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.
There was a problem hiding this comment.
We could setCommentRange instead of setTextRange, but setTextRange also gets the sourcemap locations right for declaration maps, in addition to comments, so is actually what we want. The complexities around when we can and can't actually copy the range are why setTextRange is overridden within the nodebuilder nowadays. (Specifically, we can't copy the location if it's from another file than the one for the tree it's being copied into.) It's definitely unfortunate that some uses in expressionToTypeNode fell through the cracks when it got pulled out, though.
|
|
||
| //// [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 |
Co-authored-by: MichaelMitchell-at <=>
|
@MichaelMitchell-at could you check out if the current state of this PR fixes your codebase? |
| setTextRange(context, updated, node); | ||
| } | ||
| return updated; | ||
| return setTextRange(context, updated, node); |
There was a problem hiding this comment.
The containing function was only cloning the leftmost identifier in a qualified name, leaving the other ones intact - but they could also be from a different source file than the target file. So when the leftmost one gets updated, the right ones need to be (potentially) cloned too.
But even the node path containing the leftmost identifier wasn't handled quite right here because that the return value of setTextRange was ignored.
Taking a look! |
|
Maybe a dumb question, but did that last change fix everything else such that the rest could be undone? |
|
Confirmed that the specific case in our codebase is resolved. Just kicked off a full CI build to verify there isn't any visible regression elsewhere. Will follow-up shortly. |
|
Looks good after typechecking our codebase! |
No. The first change specifically targets import type nodes, the second one specifically targets type reference nodes |
There was a problem hiding this comment.
Thanks for tracking this down, and thanks @MichaelMitchell-at for the repro! This looks great!
|
@typescript-bot cherry-pick this to release-5.8 |
Co-authored-by: michaelm <michael.mitchell@airtable.com>
|
Hey, @jakebailey! I've created #61290 for you. |
fixes #61239