Skip to content

Sourcemap: stop producing sourcemap mappings with negative lines and columns #1445

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Sourcemap: stop producing sourcemap mappings with negative lines and columns #1445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
## Bug fixes
* Compiler: put custom header at the top of the output file (fix #1441)
* Compiler (js parser): fix parsing of js labels (fix #1440)
* Sourcemap: stop producing sourcemaps mappings with negative lines or columns

# 5.1.1 (2023-03-15) - Lille
## Bug fixes
Expand Down
24 changes: 9 additions & 15 deletions compiler/lib/js_output.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,13 @@ struct
| U | Pi { Parse_info.src = None | Some ""; _ } ->
push_mapping
(PP.pos f)
{ Source_map.gen_line = -1
; gen_col = -1
; ori_source = -1
; ori_line = -1
; ori_col = -1
; ori_name = None
}
{ Source_map.gen_line = -1; gen_col = -1; ori_location = None }
| Pi { Parse_info.src = Some file; line; col; _ } ->
push_mapping
(PP.pos f)
{ Source_map.gen_line = -1
; gen_col = -1
; ori_source = get_file_index file
; ori_line = line
; ori_col = col
; ori_name = None
; ori_location = Some { source = get_file_index file; line; col; name = None }
}

let output_debug_info_ident f nm loc =
Expand All @@ -115,10 +106,13 @@ struct
(PP.pos f)
{ Source_map.gen_line = -1
; gen_col = -1
; ori_source = get_file_index file
; ori_line = line
; ori_col = col
; ori_name = Some (get_name_index nm)
; ori_location =
Some
{ source = get_file_index file
; line
; col
; name = Some (get_name_index nm)
}
}

let ident f = function
Expand Down
106 changes: 52 additions & 54 deletions compiler/lib/source_map.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@

open! Stdlib

type ori_location =
{ source : int
; line : int
; col : int
; name : int option
}

type map =
{ gen_line : int
; gen_col : int
; ori_source : int
; ori_line : int
; ori_col : int
; ori_name : int option
; ori_location : ori_location option
}

type mapping = map list
Expand All @@ -52,7 +56,11 @@ let empty ~filename =

let map_line_number ~f =
let f i = if i < 0 then i else f i in
fun m -> { m with ori_line = f m.ori_line; gen_line = f m.gen_line }
fun m ->
{ m with
gen_line = f m.gen_line
; ori_location = Option.map m.ori_location ~f:(fun l -> { l with line = f l.line })
}

let string_of_mapping mapping =
let mapping =
Expand All @@ -79,15 +87,7 @@ let string_of_mapping mapping =
if i < len
then
let c = a.(i) in
if prev >= 0
&& c.ori_source = a.(prev).ori_source
&& c.ori_line = a.(prev).ori_line
&& c.ori_col = a.(prev).ori_col
then (* We already are at this location *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this logic removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it because it didn't look like it was necessary, but I'm not at all confident about that. If I put it back, then we have to decide what happens in the other three cases besides Some, Some. Do you know what situation causes this case to be hit?

loop prev (i + 1)
else if i + 1 < len
&& c.gen_line = a.(i + 1).gen_line
&& c.gen_col = a.(i + 1).gen_col
if i + 1 < len && c.gen_line = a.(i + 1).gen_line && c.gen_col = a.(i + 1).gen_col
then (* Only keep one source location per generated location *)
loop prev (i + 1)
else (
Expand All @@ -104,26 +104,25 @@ let string_of_mapping mapping =
let l =
(c.gen_col - !gen_col)
::
(if c.ori_source = -1
then []
else
(c.ori_source - !ori_source)
:: (c.ori_line - !ori_line)
:: (c.ori_col - !ori_col)
::
(match c.ori_name with
| None -> []
| Some n ->
let n' = !ori_name in
ori_name := n;
[ n - n' ]))
(match c.ori_location with
| None -> []
| Some ori ->
(ori.source - !ori_source)
:: (ori.line - !ori_line)
:: (ori.col - !ori_col)
::
(match ori.name with
| None -> []
| Some n ->
let n' = !ori_name in
ori_name := n;
[ n - n' ]))
in
gen_col := c.gen_col;
if c.ori_source <> -1
then (
ori_source := c.ori_source;
ori_line := c.ori_line;
ori_col := c.ori_col);
Option.iter c.ori_location ~f:(fun ori ->
ori_source := ori.source;
ori_line := ori.line;
ori_col := ori.col);
Vlq64.encode_l buf l;
loop i (i + 1))
in
Expand Down Expand Up @@ -158,24 +157,21 @@ let mapping_of_string str =
match v with
| [ g ] ->
gen_col := !gen_col + g;
{ gen_line = line
; gen_col = !gen_col
; ori_source = -1
; ori_line = -1
; ori_col = -1
; ori_name = None
}
{ gen_line = line; gen_col = !gen_col; ori_location = None }
| [ g; os; ol; oc ] ->
gen_col := !gen_col + g;
ori_source := !ori_source + os;
ori_line := !ori_line + ol;
ori_col := !ori_col + oc;
{ gen_line = line
; gen_col = !gen_col
; ori_source = !ori_source
; ori_line = !ori_line
; ori_col = !ori_col
; ori_name = None
; ori_location =
Some
{ source = !ori_source
; line = !ori_line
; col = !ori_col
; name = None
}
}
| [ g; os; ol; oc; on ] ->
gen_col := !gen_col + g;
Expand All @@ -185,10 +181,13 @@ let mapping_of_string str =
ori_name := !ori_name + on;
{ gen_line = line
; gen_col = !gen_col
; ori_source = !ori_source
; ori_line = !ori_line
; ori_col = !ori_col
; ori_name = Some !ori_name
; ori_location =
Some
{ source = !ori_source
; line = !ori_line
; col = !ori_col
; name = Some !ori_name
}
}
| _ -> invalid_arg "Source_map.mapping_of_string"
in
Expand All @@ -198,14 +197,13 @@ let mapping_of_string str =
readline 0 0 []

let maps ~sources_offset ~names_offset x =
let gen_line = x.gen_line in
let ori_source = x.ori_source + sources_offset in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this line was turning no source into an invalid source.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I believe that's correct. Turning the type into an option pointed this position out, but if we want to make this patch is minimal as possible, maybe this would be the only line that has to change. Let me know if you think this is what we should try to do.

let ori_name =
match x.ori_name with
| None -> None
| Some ori_name -> Some (ori_name + names_offset)
let ori_location =
Option.map x.ori_location ~f:(fun ori ->
let source = ori.source + sources_offset in
let name = Option.map ori.name ~f:(fun name -> name + names_offset) in
{ ori with source; name })
in
{ x with gen_line; ori_source; ori_name }
{ x with ori_location }

let filter_map sm ~f =
let a = Array.of_list sm.mappings in
Expand Down
12 changes: 8 additions & 4 deletions compiler/lib/source_map.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,17 @@
* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
*)

type ori_location =
{ source : int
; line : int
; col : int
; name : int option
}

type map =
{ gen_line : int
; gen_col : int
; ori_source : int
; ori_line : int
; ori_col : int
; ori_name : int option
; ori_location : ori_location option
}

type mapping = map list
Expand Down
28 changes: 14 additions & 14 deletions compiler/tests-compiler/sourcemap.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,17 @@ let print_mapping (sm : Source_map.t) =
let sources = Array.of_list sm.sources in
let _names = Array.of_list sm.names in
List.iter sm.mappings ~f:(fun (m : Source_map.map) ->
let file = function
| -1 -> "null"
| n -> normalize_path sources.(n)
in
Printf.printf
"%s:%d:%d -> %d:%d\n"
(file m.ori_source)
m.ori_line
m.ori_col
m.gen_line
m.gen_col)
match m.ori_location with
| Some ori ->
let file n = normalize_path sources.(n) in
Printf.printf
"%s:%d:%d -> %d:%d\n"
(file ori.source)
ori.line
ori.col
m.gen_line
m.gen_col
| None -> Printf.printf "null -> %d:%d\n" m.gen_line m.gen_col)

let%expect_test _ =
with_temp_dir ~f:(fun () ->
Expand Down Expand Up @@ -78,7 +78,7 @@ let%expect_test _ =
/dune-root/test.ml:1:7 -> 6:25
/dune-root/test.ml:1:12 -> 6:27
/dune-root/test.ml:1:4 -> 7:18
null:-1:-1 -> 10:2
null -> 10:2
|}]

let%expect_test _ =
Expand Down Expand Up @@ -119,8 +119,8 @@ let%expect_test _ =
;;;;EAEE,EAAE,EAAC,CAAE;ECQY,UACC |}]

let%expect_test _ =
let gen (gen_line, gen_col) (ori_line, ori_col) ori_source : Source_map.map =
{ gen_line; gen_col; ori_source; ori_line; ori_col; ori_name = None }
let gen (gen_line, gen_col) (line, col) source : Source_map.map =
{ gen_line; gen_col; ori_location = Some { source; line; col; name = None } }
in
let s1 : Source_map.t =
{ (Source_map.empty ~filename:"1.map") with
Expand Down
2 changes: 0 additions & 2 deletions compiler/tests-sourcemap/dump.reference
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,3 @@ b.ml:1:10 -> 17: function f(x){<>return x - 1 | 0;}
b.ml:1:6 -> 24: function f(x){return <>x - 1 | 0;}
b.ml:1:15 -> 34: function f(x){return x - 1 | 0;<>}
b.ml:1:4 -> 23: var Testlib_B = [0, <>f];
a.ml:-1:-1 -> 3: <>function caml_call1(f, a0){
a.ml:-1:-1 -> 2: <>}
23 changes: 12 additions & 11 deletions compiler/tests-sourcemap/dump_sourcemap.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,18 @@ let print_mapping lines (sm : Source_map.t) =
^ "<>"
^ String.sub line ~pos:col ~len:(len - col)
in
if match file m.ori_source with
| "a.ml" | "b.ml" | "c.ml" | "d.ml" -> true
| _ -> false
then
Printf.printf
"%s:%d:%d -> %d:%s\n"
(file m.ori_source)
m.ori_line
m.ori_col
m.gen_col
(mark m.gen_col lines.(m.gen_line - 1)))
Option.iter m.ori_location ~f:(fun (ori : Source_map.ori_location) ->
if match file ori.source with
| "a.ml" | "b.ml" | "c.ml" | "d.ml" -> true
| _ -> false
then
Printf.printf
"%s:%d:%d -> %d:%s\n"
(file ori.source)
ori.line
ori.col
m.gen_col
(mark m.gen_col lines.(m.gen_line - 1))))

let files = Sys.argv |> Array.to_list |> List.tl

Expand Down