rust-sourcemap icon indicating copy to clipboard operation
rust-sourcemap copied to clipboard

[EPIC] Modernize the crate

Open loewenheim opened this issue 2 years ago • 2 comments

This crate is pretty old and has a number of outdated/unidiomatic features. For example:

  • SourceMapBuilder::into_sourcemap should be called build.
  • SourceMap stores tokens as an unordered vector with a separate ordered index. There is no clear reason tokens can't be stored in an ordered way in the first place (as a vector or BTreeMap).
  • SourceMap stores sources as a Vec<String>, but sources may be null in the sourcemap JSON format. This leads to null sources being replaced with "" during parsing, to unclear benefit.
  • SourceMap has a lot of getter and setter methods that could just as well be replaced with mutable access to the corresponding fields.

loewenheim avatar Aug 29 '23 10:08 loewenheim

The crate is also implementing binary_search manually, as well as its own version of IndexMap.

There is also unsafe code related to SourceView, which is a type that definitely has nothing to do in a generic SourceMap crate. Similar to how the heuristics-based "find original function name" is very out of place.

The problem with the above is that it is still used from the legacy Sentry processor via symbolic. I would like to kill all that with fire, but I’m unsure about the right time to do so.

Swatinem avatar Aug 29 '23 12:08 Swatinem

I’m also a bit unsure about making all the fields public. The crate already has a distinction between a raw JSON sourcemap, vs a more concrete type that you can build and manipulate actively.

It might make sense to keep that distinction, but there is an argument to be made to make the concrete type more opaque. But at the same time, having different variants for SourceMapHermes for example does make sense, as a sourcemap with the hermes-style extension for scopes does serve a different usecase.

Swatinem avatar Aug 29 '23 12:08 Swatinem