-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
feat(proguard): Remap exception names in proguard #15795
Conversation
@jan-auer not super happy with the completely broken abstraction here but i rather refactor the entire thing at a later point than to clean it up now. Not sure though if this invalidates some assumptions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this can be cleaned up later, as the abstraction is already a bit leaky.
def _report_stack(stacktrace, container): | ||
if not stacktrace or not get_path(stacktrace, "frames", filter=True): | ||
def _report_stack(stacktrace, container, is_exception=False): | ||
if not is_exception and (not stacktrace or not get_path(stacktrace, "frames", filter=True)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is indeed confusing as we're now reporting non-existing stack traces just to get to the exception container. I also don't have a better idea, since I suppose you need the stack frame's platform to decide whether you want to touch the container exception.
To be safe, you could default frames
to an empty list, but since you only selectively turn this on, I could not find an invalidated invariant. At least the native plugin relies on empty stacktraces being removed, but there you don't toggle, so it's fine.
As an alternative, could we add a find_exceptions_in_data
and call it in should_process_for_stacktraces
as well as process_stacktraces
before processing the stack traces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to change the entire thing to return stacks + related data and treat missing stacks as empty stacks.
for view in self.mapping_views: | ||
original = view.lookup(key) | ||
if original != key: | ||
new_module, new_cls = original.rsplit(".", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work with generics that contain class paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea. At least not different to how we already do with methods
Is there anything blocking this PR? |
No description provided.