feat: Implement Frame Remapping

[Swatinem]

This essentially reimplements the whole parser, as well as a java
stacktrace parser.

The main focus is on the ProguardMapper::remap_frame function, which creates
a new StackFrame, while de-obfuscating class and method names,
re-mapping lines, and resolving inline functions.

[Swatinem]

• The current solution is really naive, and does a linear scan per member, trying to find mappings that match the line number.
• When remapping, it tries to remap as much as it can, and will return a copy of the original frame when it fails to find anything.
• It basically only has a single API, and requests to only map the class can be done by using a bogus method/line pair, such as StackFrame::new("class.to.remap", "", "", 0).
• The mapping file does not include any filenames, and we try to infer the filename from the classname, which can give wrong results when the source is actually at .kt. Not sure if/how we can do better here.

[jan-auer]

Very excited about this rewrite! Thanks for putting the effort in.

[jan-auer]

This is great. 🎉 The separation and API are great. I have a bunch of really small nit picks now, and then this is G2G!

One thing that struck me, though: We do not expose any parsing errors. If parsing the file fails, we just receive None, but we don't know what it was that threw us off. This can become a challenge in debugging new formats, since there won't be any alerting when records are skipped silently, etc. Should we expose an error type for those and change the record iterator to return Result<Record, ProguardError>?

[jan-auer]

Wouldn't link into Sentry docs on this crate, since it is completely independent of Sentry. Aside from that, 🚢 it!

[Swatinem]

This can become a challenge in debugging new formats, since there won't be any alerting when records are skipped silently, etc. Should we expose an error type for those and change the record iterator to return Result<Record, ProguardError>?

I was also thinking about this. From a high level perspective, I think it should re-map as much as it can rather than failing the whole file. The record iterator is line-based, which should hopefully give enough context. I was also thinking about making it non_exhaustive, to be more future-proof.

[jan-auer]

it should re-map as much as it can rather than failing the whole file

👍 Yes, fully agree with this.

The record iterator is line-based, which should hopefully give enough context

It could still return Result which the user is free to skip (and log), for instance. Right now, since the iterator swallows None values, there's no way to detect errors. A file with only invalid values will just be empy.

I was also thinking about making it non_exhaustive

I still haven't got a good feeling for when to use this. Would say to leave it exhaustive and then major-bump if we add support for a major new format that has new capabilities. WDYT?

[Swatinem]

Would say to leave it exhaustive and then major-bump if we add support for a major new format that has new capabilities. WDYT?

Yes, on second thought, non_exhaustive can be quite a pain to use, even if it is just for integration tests.

It could still return Result

The iterator does: https://github.com/getsentry/rust-proguard/pull/11/files#diff-90baa1ca016fe9c6cc78a9f5b29770d3R130

[jan-auer]

Sorry, I was typing too quick. I meant a Result with some error that describes what went wrong (being that it's invalid UTF-8, or something unexpected in the line, etc). At the moment it returns the line (since MappingRecord::try_parse only returns Option) - which may be enough to guess what's going on, but one cannot match on it or print nice error messages.

All these are optional -- the current interface is fine from my end, so feel free to merge.