Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--experimental-transform-types leads to wrong lineNumbers in util.getCallSite for files with empty lines #55211

Closed
alexanderniebuhr opened this issue Oct 1, 2024 · 12 comments
Labels
duplicate Issues and PRs that are duplicates of other issues or PRs.

Comments

@alexanderniebuhr
Copy link

alexanderniebuhr commented Oct 1, 2024

Version

v22.9.0

Platform

Darwin MacBookPro 24.1.0 Darwin Kernel Version 24.1.0: Tue Sep 17 07:46:59 PDT 2024; root:xnu-11215.40.59~38/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

For the given file, the output is different for two different commands:

// @ts-expect-error
import { getCallSite } from "node:util";

console.log("should be", 4, "is", getCallSite()[0].lineNumber)

// A COMMENT
console.log("should be", 7, "is", getCallSite()[0].lineNumber)
console.log("should be", 8, "is", getCallSite()[0].lineNumber)



console.log("should be", 12, "is", getCallSite()[0].lineNumber)
$ node --experimental-strip-types src/getCallUtil.ts
(node:51623) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
should be 4 is 4
should be 7 is 7
should be 8 is 8
should be 12 is 12

$ node --experimental-strip-types --experimental-transform-types --enable-source-maps src/getCallUtil.ts
(node:51645) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
should be 4 is 3
should be 7 is 5
should be 8 is 6
should be 12 is 7

How often does it reproduce? Is there a required condition?

It only happens when using --experimental-transform-types

What is the expected behavior? Why is that the expected behavior?

The lineNumbers should be correctly regardless if --experimental-transform-types is used or not

What do you see instead?

I'm seeing the wrong line numbers based on the sum of all empty lines.

$ node --experimental-strip-types --experimental-transform-types --enable-source-maps src/getCallUtil.ts
(node:51645) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
should be 4 is 3 (one empty line so far)
should be 7 is 5 (two empty lines so far)
should be 8 is 6 (two empty lines so far)
should be 12 is 7 (five empty lines so far)

Additional information

No response

@RedYetiDev RedYetiDev closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
@RedYetiDev RedYetiDev added duplicate Issues and PRs that are duplicates of other issues or PRs. invalid Issues and PRs that are invalid. labels Oct 1, 2024
@RedYetiDev
Copy link
Member

If this even a bug, Duplicate of #55109


Transforming types modifies where each price of code is, so this is, IMO, expected.

@RedYetiDev RedYetiDev added the wontfix Issues that will not be fixed. label Oct 1, 2024
@alexanderniebuhr
Copy link
Author

alexanderniebuhr commented Oct 1, 2024

@RedYetiDev I don't think it's a duplicate of #55109, since I'm not using any third party lib like tsx or ts-node, so the "transpilation" is totally different in this case.

Additionally https://nodejs.org/api/cli.html#--experimental-transform-types states that --experimental-transform-types enables --enable-source-maps, which leads me to think that any stack trace should point to the right location, regardless if the code was modified or not, because it should use source maps to map the right location.

@RedYetiDev
Copy link
Member

IMHO the issues spawn from the same problem: source maps not being respected by getCallSite, so only one issue is needed to track it

@alexanderniebuhr
Copy link
Author

If the other issue is tracking that, and not only 3rd party libs, that's fine.

The other question I would have is, why does --experimental-transform-types changes code if there are not types to transform. E.g. why does it strip empty lines, when --experimental-strip-types does not. That seems not consistent, sure if enum is transformed the code changes, but for the given example there is no reason for the code to change.

@RedYetiDev
Copy link
Member

RedYetiDev commented Oct 1, 2024

--experimental-transform-types does transform the code no matter what, while strip only strips.

@RedYetiDev RedYetiDev removed invalid Issues and PRs that are invalid. wontfix Issues that will not be fixed. labels Oct 12, 2024
@marco-ippolito
Copy link
Member

@legendecas do you think its possible to reconstruct the callsite with sourcemaps?

@joyeecheung
Copy link
Member

That should be possible but will require a bit of work, for example currently the uncaught exception printing calls back to JS land to map the source locations for the exception source line:

MaybeLocal<Value> maybe_ret = env->get_source_map_error_source()->Call(

Which goes here

const errorSource = getErrorSource(sm, originalSource, originalLine, originalColumn);

and to here

`${originalSourcePathNoScheme}:${originalLine + 1}\n${line}\n${prefix}^\n\n`;

Something similar can be done for util.getCallsite() when the StackFrame::GetScriptSourceMappingURL() or StackFrame::GetScriptNameOrSourceURL() is returning something non-empty, just operating on the call site objects instead.

@legendecas
Copy link
Member

It is doable, and it is how internal stack trace translation works. I think for this issue and #55109, the question is whether util.getCallSite() should translate the information with source map or not. In userland, this can also be translated with public API module.findSourceMap() as well.

@joyeecheung
Copy link
Member

Yeah I imagine something like module.mapCallsite(util.getCallsite()) might be more versatile, and it can take duck-typed call site objects (or just options)

@cjihrig
Copy link
Contributor

cjihrig commented Oct 28, 2024

In case it helps anyone, here is where the test runner does this sort of thing with source maps.

@alexanderniebuhr
Copy link
Author

Do I understand correctly, that there might be a way to get it working with --experimental-transform-types by wrapping util.getCallSite() with some sort of custom sourcemap handling? If yes, do we a have a more abstracted example to look at, so I can implement in userland for the time-being, until further decisions about the API are made?

@legendecas
Copy link
Member

here is how the internal call site serialization does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Issues and PRs that are duplicates of other issues or PRs.
Projects
None yet
Development

No branches or pull requests

6 participants