Skip to content
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

fix(sourcemap): Improve scope name resolution #786

Merged
merged 6 commits into from
May 15, 2023

Conversation

loewenheim
Copy link
Contributor

We have seen cases in Flutter where a scope name cannot be resolved, but the scope's closing brace has the correct name attached. Therefore, if we can't resolve a scope name the straightforward way, we try to grab the name off the scope's last token.

@loewenheim loewenheim requested a review from a team May 15, 2023 10:14
@loewenheim loewenheim self-assigned this May 15, 2023
@loewenheim
Copy link
Contributor Author

See getsentry/sentry-dart#1430 for a case that should be fixed by this.

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #786 (9e007c4) into master (3b2f1f6) will increase coverage by 0.03%.
The diff coverage is 91.35%.

❗ Current head 9e007c4 differs from pull request most recent head 48b1784. Consider uploading reports for the commit 48b1784 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #786      +/-   ##
==========================================
+ Coverage   74.99%   75.02%   +0.03%     
==========================================
  Files          71       71              
  Lines       15570    15610      +40     
==========================================
+ Hits        11676    11711      +35     
- Misses       3894     3899       +5     

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

it might be worth extracting that into its own fn try_resolve_closing_name(ctx, sm, range) or some such

symbolic-sourcemapcache/src/writer.rs Outdated Show resolved Hide resolved
ctx.offset_to_position(range.end - 1).and_then(|sp| {
let token = sm.lookup_token(sp.line, sp.column);
token
.filter(|t| t.get_dst() == (sp.line, sp.column))
Copy link
Member

Choose a reason for hiding this comment

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

We are looking for an exact token match at the closing } which is good. However in testing, we figured out that this trailing token is exactly 1 byte wide.
We could further filter this down to make sure that the token at range.end is a different token. I wonder how much that would change things though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to check for a literal closing brace?

Copy link
Member

Choose a reason for hiding this comment

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

only partly. Sure, the end of an arrow function with an expression body would not have a closing }, however the case with dart is that it should only match that }, and the following token immediately afterwards was something different. I believe we should make this special-case as narrow as possible to avoid false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants