Sourcemap: stop producing sourcemap mappings with negative lines and columns #1445
Conversation
…columns This patch began by changing the mapping type in compiler/lib/source_map.ml so that it more obviously encodes the possible cases for a particular mapping. All other changes were made in response to the type errors that ensued.
|
Is a mapping ever useful when ori_location is none ? |
|
Maybe it isn't, but then why are those mappings even allowed? It seems strange for the spec to mention this kind of mapping if they aren't useful. That said, I can't think of a use for them, so maybe you're right. |
|
Seeing that the ocaml compiler seems to generate these maybe we can ask someone who know more about that code why these are generated? |
I don't think the OCaml compiler has anything to do with this. What makes you think that ?
I initially misread your patch. If you have the following list of token Do you know which part of this PR fixes your issue ? Have you measured the impact of boxing ori_location ? |
| && 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 *) |
There was a problem hiding this comment.
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?
| @@ -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 | |||
There was a problem hiding this comment.
It seems that this line was turning no source into an invalid source.
There was a problem hiding this comment.
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.
My understanding was that js_of_ocaml reads a source map produced for the ocaml byte code (by the ocaml compiler) and then translates the mappings to match the generated JavaScript, but this might be inaccurate? Your explanation makes sense though! |
Thanks for explaining; this makes sense to me.
I think the line you pointed out in one of your review comments is indeed the location of bug. However, I'd argue there are several places where the bug could have been. The whole patch fixes the issue for good because you can't forget to handle both cases.
No, I have not. I think it's actually two extra boxes - one for the option and one for the new record. We could eliminate one of them by introducing a specialized option type where the |
You can also turn the mapping record type into a variant with 3 constructors, one for each case defined in the spec. |
Yes, good point; that would elegantly avoid introducing more boxes. That was actually approach I initially considered, not because I wanted to avoid boxing, but just because I thought it a good idea to have the types match the spec. I didn't end up doing that because I thought it would cause the PR to be a bit too large. I'd be happy to re-write the PR to follow your suggestion. Does that sound good to you? I think if we do that, we probably don't need to measure impact after all because we aren't adding or removing boxes. (aside: I'm not sure how I would get a definitive answer about how much impact the changes had, anyway; it seems like a hard measurement to make without putting a lot time into building good benchmarks) |
|
Actually, I just started trying that re-write, but I'm quickly discouraged from it because it greatly increases the friction of accessing |
|
@pmwhite, I've done a minimal fix in #1449 and tried the 3 constructors approach in #1450. It seems to improve both speed and memory consumption. I run Here are the results master This PR #1450 is 10 to 15% faster and consume 17% to 25% less. |
|
I've merged #1450 |
|
Oh, that is significant. Thank you very much for doing this! |
|
Thank you both for doing this! |
This patch began by changing the mapping type in compiler/lib/source_map.ml so that it more obviously encodes the possible cases for a particular mapping. All other changes were made in response to the type errors that ensued.
No new tests were added because this change is mostly behavior preserving, and the situation in which behavior changes is demonstrated by changes to existing tests.
The motivation for this change is that the sourcemaps produced by js_of_ocaml are rejected by the Mozilla sourcemap library. In particular, it's this error message that gets triggered.