Add sourcemap generation to JavaScript codegen target #3675

AlisCode · 2024-10-06T22:25:42Z

Here goes!

Resolves Sourcemaps #2249
Partially adresses Code Generation: Include file and line attribute #3149 - i don't know anything about Erlang so generating sourcemaps is out of my zone of comfort.

The PR is still kind of WIP - mainly, I'd like to talk about how to approach testing (and actually add way more snapshot tests before this gets a chance to be merged).

I also have 2 expects on calls to sourcemap - I'm sure this is not the right way to do it, so I'm wondering what you guys think is the best approach ? Is it fine to have a "sourcemap error" variant in the big Gleam Error enum ?

Context

I highly recommend watching the following talk if you're unfamiliar with Sourcemaps : https://www.youtube.com/watch?v=6LI0BJIiamg

Long story short, a sourcemap is a file that defines mappings from specific places in a generated file (the generated javascript code - which needs to be referring to its sourcemap file) to a source file (the gleam module).

It's used for example :

By the browser to be able to display the actual line of the Gleam source instead of the generated JS code
In tools like Sentry (I'm not affiliated, but they're huge in the industry) to be able to point to the actual source instead of the generated Javascript code
By debuggers such as vscode-js-debug to be able to provide debugging straight from your Gleam sources : this PR enables using a debugger on Gleam, which I'm sure some people are excited about (I am! 🎉).

Content

This PR adds sourcemap generation for the JavaScript codegen target.
Sourcemap generation is disabled by default, and needs to be manually enabled using the following config in the gleam.toml manifest :

[javascript]
sourcemaps = true

This is done out of symmetry with other languages and tools - for example TypeScript does not automatically generate them. I imagine as your codebase grows large, it can end up being quite a heavy process. Also I'm assuming if this ever gets released, it should maybe be marked experimental.

AlisCode · 2024-10-06T22:35:04Z

compiler-core/src/codegen.rs

+                sourcemap.to_writer(&mut output).expect("Failed to write sourcemap to memory. This is a bug in sourcemap, please report.");
+                let output = String::from_utf8(output).expect("Sourcemap did not generate valid UTF-8. This is a bug in sourcemap, please report.");


Really not a fan of those expect - opened to any suggestion here

Please remove the bit about opening a bug ticket, all panics have this information printed for them

AlisCode · 2024-10-06T22:37:03Z

compiler-core/src/io.rs

+///
+/// Used to produce SourceMaps.
+#[derive(Debug)]
+pub struct CursorPositionWriter<'a, W> {


Or move this to compiler-core/src/io/cursor_position_writer.rs ?
Also not a fan of the name

lpil

Thank you! I've left a bunch of notes inline

lpil · 2024-10-07T18:12:26Z

compiler-core/src/config.rs

@@ -662,6 +662,8 @@ pub struct ErlangConfig {
 pub struct JavaScriptConfig {
    #[serde(default)]
    pub typescript_declarations: bool,
+    #[serde(default)]
+    pub sourcemap: bool,


Suggested change

pub sourcemap: bool,

pub sourcemaps: bool,

lpil · 2024-10-07T18:14:51Z

compiler-core/src/javascript.rs

+                type_reference,
+                "export {}",
+                line(),
+                sourcemap_reference,


How come this is not with the type reference at the top?

The declaration of the SourceMap URL does not have to be at a specific location, it can be anywhere in the JS code. It is something that's separate from the type reference (which is just TS-specific in my understanding), so I think it's best to keep them as separate declaration blocks for symmetry.

I take it from this comment that you'd rather have the sourcemap declaration at the top, so I moved it :)

lpil · 2024-10-07T18:18:11Z

compiler-core/src/pretty.rs

+    WithSourceMapLocation {
+        document: Box<Self>,
+        start: LineColumn,
+    },


This module should not know anything about code generation or Gleam, but here it knows about JavaScript specific implementation details. Any changes to this module should not be specific to anything other than pretty printing.

When building a sourcemap, for each mapping we want to add, we need two things :

The line+column of the mapping in the generated (JS) file

The line+column inside the source (Gleam) file it maps to.

In my implementation, the field start in this enum variant stores the information of where the Document is in the source file (a SrcSpan passed through the LineNumbers util). The only place we know the line+column on the generated JS file is when the pretty printer is writing the Document inside a file - then we know as we're writing "The document is being printed at the line x, column y", and this info is lost afterwards.

I agree that conceptually the pretty module shouldn't know anything about JS, but given that the JS files that Gleam produces are pretty-printed, I don't see how it's possible to generate sourcemaps without hooking into the pretty-printer itself. Any idea I could explore ?

lpil · 2024-10-07T18:18:56Z

compiler-core/src/sourcemap.rs

+        match self {
+            SourceMapEmitter::Null => (),
+            SourceMapEmitter::Emit(source_map) => {
+                tracing::debug!("emitting one sourcemap entry");


Remove this please

lpil · 2024-10-07T18:19:08Z

compiler-core/src/sourcemap.rs

+}
+
+impl SourceMapEmitter {
+    pub fn add_mapping(&mut self, dst_line: u32, dst_col: u32, src_location: LineColumn) {


No abbreviations please

lpil · 2024-10-07T18:25:03Z

compiler-core/src/format.rs

+    writer: &mut impl Utf8Writer,
+    src: &EcoString,
+    path: &Utf8Path,
+    source_map_emitter: &mut SourceMapEmitter,


Formatting never uses a source map emitter, there should be no change to this API.

lpil · 2024-10-07T18:32:31Z

compiler-core/src/io.rs

+
+impl<'a, W: Utf8Writer + std::fmt::Write> Utf8Writer for CursorPositionWriter<'a, W> {
+    /// A wrapper around `fmt::Write` that has Gleam's error handling.
+    fn str_write(&mut self, str: &str) -> Result<()> {


This iterates the string twice. Can we do it once instead?

The pretty printer also iterates over and counts the characters, can that work be reused?

I've reused LineColumn inside of this now. As long as LineNumber does not change behavior (the TODOs mention graphemes which confuses me a bit), then this should work.

Whenever the behavior of LineColumn changes, the test should break :)

lpil · 2024-10-07T18:33:31Z

compiler-core/src/io.rs

+        self.line += newline_count;
+        if newline_count > 0 {
+            let lastline = str.lines().last().expect("Should have at least one line");
+            self.column = lastline.len();


This is number of bytes and not unicode graphemes or characters. Is what sourcemaps expect?

A sourcemap works with byte indices, in my understanding this is correct. I'll add a test with emojis inside the code, see if that breaks anything.

lpil · 2024-10-07T18:36:59Z

compiler-core/src/javascript/tests/sourcemaps.rs

+    assert_sourcemap!(
+        "fn add_2(x) {
+  x + 2
+}"


Is this single case enough to test all the given functionality? I presume it is not only implemented for addition.

No it isn't. I have tested the sourcemaps with all examples from the Gleam tour on a personal project, I'm going to add those test cases in this PR, but I wanted to get some feedback on my snapshot testing before doing that. Indeed your comment below indicates that it's not clear how to test this for now.

Before I add more test cases, let's agree on how to test this feature

lpil · 2024-10-07T18:37:46Z

...ests/snapshots/gleam_core__javascript__tests__sourcemaps__sourcemap_function_definition.snap

+source: compiler-core/src/javascript/tests/sourcemaps.rs
+expression: sourcemap_viz
+---
+note: 


This snapshot is rather verbose, to the extent of being hard to read. A more concise format would be very helpful

I completely agree, but discussions with Gleam folks on discord didn't really lead to a precise conclusion. What exactly would you be happy with?

Maybe each test could be "what is the Gleam location of this specific location in the JS file" - then the snapshot would contain the whole JS + Gleam code on top of each other, with the one position you're trying to assert?

As an example, say you write a test case that asks "What is the position in the Gleam code of the character 1 line 1 in the generated JS file ?", the snapshot would look like this:

┌─ original.gleam:1:1 │ 1 │ fn add_2(x) { │ ^ This code │ x + 2 │ } │ ┌─ generated.js:2:1 │ 2 │ function add_2(x) { │ ^ Gets mapped to this │ return x + 2; │ } }

lpil · 2024-10-21T13:24:57Z

When you are ready for a review please un-draft this PR. Thank you!

…ignments

…codebase

…he sourcemap module

AlisCode · 2024-10-24T13:49:44Z

I think I need your opinion on stuff before I can write a bunch of unit tests or refactor the PR, so I'm marking this as ready.

lpil · 2024-12-02T15:35:41Z

Hello! Are you waiting on something from me? I can't see anything here but wasn't sure if I missed anything.

AlisCode · 2024-12-03T08:35:55Z

Hi @lpil,

this PR got a bit old and so it has a few merging conflicts that I'll resolve today after work.

There are 2 points which needs clarification before I proceed to add more unit tests :

#3675 (comment)
About this comment : The approach I took in this PR is that we're adding a variant to the pretty.rs module, in order for the information on Line + Column to stay relevant even after a prettification pass. Indeed, sourcemaps work by referencing columns+lines in generated code, so if the pretty module is not aware of sourcemaps, I don't see how this is implementable since to the best of my knowledge it's the only place where tracking the info "where is this code in the generated JS source" makes sense.

#3675 (comment)
About this comment : Is this approach good enough ? If it is, then I'll add some more test cases :)

cdaringe · 2024-12-07T20:34:16Z

I noticed that if i already have a compiled build cache, then i switch to this compiler with sourcemaps turned on, maps arent emitted. i have to run a clean first, presumably to bypass a cache hit that does not consider this flag as input to the cache?

AlisCode commented Oct 6, 2024

View reviewed changes

lpil reviewed Oct 7, 2024

View reviewed changes

lpil marked this pull request as draft October 7, 2024 20:14

AlisCode added 20 commits October 24, 2024 14:44

Basic support for generating SourceMaps on Typescript target

1801b7e

Produce Sourcemap for case

0123bf8

Add sourcemap for todo and panic instructions

a6c4073

Add sourcemap support for type declaration

612f592

Support sourcemap for assert

93d6e3f

Attach sourcemap location to more expressions

c9f99c9

Attach sourcemap location for inputs

cbee6b3

Cleanup sourcemap generation by not generating sourcemaps for ALL ass…

02c04bb

…ignments

Attach sourcemap location for pipeline and more expressions

424c7bc

Attach more sourcemap location

e3a31c5

Attach sourcemap location to tail-call-optimized functions

be4870a

Attach sourcemap location to external functions

ce1f0b2

Simplify sourcemap attachment process and harmonize usage across the …

44401c4

…codebase

Setup test infra for sourcemaps

fbcc362

Cleanup implementation details to prepare for a PR

6edde81

Rename sourcemap-generation functionality sourcemap->sourcemaps

fb0cf2e

Correct minor comments

5083d17

Reduce the size of the SourceMapEmitter by boxing the SourceMapBuilder

b2bd538

Remove SourceMap information from formatting API

9ed2600

Move sourcemap content generation to the sourcemap module, document t…

2c5e729

…he sourcemap module

AlisCode force-pushed the feat/sourcemap-javascript branch from e76e450 to 2c5e729 Compare October 24, 2024 13:02

AlisCode added 2 commits October 24, 2024 15:29

Reuse LineNumbers inside CursorPositionWriter

60f2ca7

Move sourcemap declaration at the top

bc4a2d5

AlisCode marked this pull request as ready for review October 24, 2024 13:48

AlisCode requested a review from lpil October 28, 2024 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sourcemap generation to JavaScript codegen target #3675

Add sourcemap generation to JavaScript codegen target #3675

AlisCode commented Oct 6, 2024 •

edited

Loading

AlisCode Oct 6, 2024

lpil Oct 7, 2024

AlisCode Oct 6, 2024

lpil left a comment

lpil Oct 7, 2024

lpil Oct 7, 2024

AlisCode Oct 24, 2024

lpil Oct 7, 2024

AlisCode Oct 9, 2024

lpil Oct 7, 2024

lpil Oct 7, 2024

lpil Oct 7, 2024

lpil Oct 7, 2024

AlisCode Oct 24, 2024

lpil Oct 7, 2024

AlisCode Oct 9, 2024

lpil Oct 7, 2024

AlisCode Oct 7, 2024

AlisCode Oct 24, 2024

lpil Oct 7, 2024

AlisCode Oct 24, 2024 •

edited

Loading

lpil commented Oct 21, 2024

AlisCode commented Oct 24, 2024

lpil commented Dec 2, 2024

AlisCode commented Dec 3, 2024 •

edited

Loading

cdaringe commented Dec 7, 2024 •

edited

Loading

		sourcemap.to_writer(&mut output).expect("Failed to write sourcemap to memory. This is a bug in sourcemap, please report.");
		let output = String::from_utf8(output).expect("Sourcemap did not generate valid UTF-8. This is a bug in sourcemap, please report.");

Add sourcemap generation to JavaScript codegen target #3675

Are you sure you want to change the base?

Add sourcemap generation to JavaScript codegen target #3675

Conversation

AlisCode commented Oct 6, 2024 • edited Loading

Context

Content

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlisCode Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

lpil commented Oct 21, 2024

AlisCode commented Oct 24, 2024

lpil commented Dec 2, 2024

AlisCode commented Dec 3, 2024 • edited Loading

cdaringe commented Dec 7, 2024 • edited Loading

AlisCode commented Oct 6, 2024 •

edited

Loading

AlisCode Oct 24, 2024 •

edited

Loading

AlisCode commented Dec 3, 2024 •

edited

Loading

cdaringe commented Dec 7, 2024 •

edited

Loading