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(forge): debugger should load sourcemaps by file_id #7058

Merged
merged 7 commits into from
Feb 10, 2024

Conversation

emo-eth
Copy link
Contributor

@emo-eth emo-eth commented Feb 9, 2024

Motivation

Summary from #7043:

ContractSources is a mapping of name -> id -> (source code, bytecode)

In src_map, the source for the contract is looked up by name. Since this may return multiple contracts with the same name, the entries are iterated over and the SourceElement's index is compared to the file_id of the current contract source. If they match, the source code is returned.

The problem is that this will never return source code for inherited contracts, because it is only loading source code for (contracts with the same name as) the contract being invoked; if the SourceElement points to a different index (aka file_id), it will never match any of the contract sources loaded from ContractSources.

ContractSources should maintain an index both of name -> id -> (source, bytecode) and id -> (source, bytecode) for looking up cross-source sourcemaps.

Solution

ContractSources now also maintains a mapping of file_id/index -> (source code, bytecode). When the loaded file_id does not match source_element.index, it looks up the contract source using the source_element's index.

I could use some help testing this – it looks to me like the debug.rs test doesn't actually make any assertions, so I don't have much to go off of.

Closes #7043

Before:
Screenshot 2024-02-08 at 6 40 40 PM

After:
Screenshot 2024-02-08 at 6 41 06 PM

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

hyped for this

crates/debugger/src/tui/draw.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg added the T-bug Type: bug label Feb 9, 2024
crates/common/src/compile.rs Outdated Show resolved Hide resolved
@emo-eth emo-eth force-pushed the debugger-sourcemaps branch from e5edb08 to 40d197b Compare February 9, 2024 21:02
crates/common/src/compile.rs Outdated Show resolved Hide resolved
crates/common/src/compile.rs Outdated Show resolved Hide resolved
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Needs cargo +nightly fmt

crates/debugger/src/tui/draw.rs Outdated Show resolved Hide resolved
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Pending @onbjerg

@DaniPopes DaniPopes requested a review from onbjerg February 9, 2024 22:01
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty!
lgtm

@mattsse mattsse merged commit d37328a into foundry-rs:master Feb 10, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forge test debugger does not show sourcemaps for inherited contracts
4 participants