Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Goal: Source mapping PC to TEAL #3726
Goal: Source mapping PC to TEAL #3726
Changes from 3 commits
d2dd036
f00862f
9279f9c
b1ee70f
bf9fc02
6ddc4d1
6645cb5
17d9b9c
3615d00
21aa4a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: I think it'd be nice if the source map format version was included here or somewhere else in the documentation easily accessible to the user.
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.
@algochoi small question/request, was there a specific reason you chose
fname + ".map"
for the map file name? With this the source map is given the name of the input file, which I think is less intuitive/accurate.In JS it's common practice for the map file to be named after the produced output file, since that's the starting point from which you can go "backwards" with the map, so I was expecting
outname + ".map"
here (idk what should happen when the output is stdout, maybe an error?).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.
That is a fair point, and I can see the confusion. I can open up a PR to fix this.
This what I initially thought, but after playing around with the commands, I think the source map should always be output to the file because the
no-out
flag saysdon't write contract program binary
, and a source map isn't necessarily the binary.But alternatively, we could also output the source map to stdout if the no output flag is raised. That sounds fine too, depending on how you interpret the
no-out
flag.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.
That would be great, thanks!
That's definitely a valid interpretation, but what do you name the source map file then? I think
<input>.tok.map
would work.Though to be fair, I think it's quite rare to use the
no-out
flag, so failing in this case would not be that bad in my opinion.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 also thought of putting this file in the
tealdbg
package, but I don't think it can be exported from there.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.
nit: I had to check the spec for some of the fields here so it might be useful to describe the fields in a comment.
Spec:
"Line 1: The entire file is a single JSON object
Line 2: File version (always the first entry in the object) and must be a positive integer.
Line 3: An optional name of the generated code that this source map is associated with.
Line 4: An optional source root, useful for relocating source files on a server or removing repeated values in the “sources” entry. This value is prepended to the individual entries in the “source” field.
Line 5: A list of original sources used by the “mappings” entry.
Line 6: A list of symbol names used by the “mappings” entry.
Line 7: A string with the encoded mapping data in base64 VLQ encoding. Each semicolon separates a line in the generated code. Note that multiple lines in the generated code may point to the same line in the source code."
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.
Good point - I eventually settled on linking the sourcemap specs URL instead in the comments, so the interested user can look at the original doc in more detail. Linking to URLs also might
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.
nit: we might want to make some of these fields optional in the json serialization depending on how closely we want to follow the spec.
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.
Good point, added
omitempty
field forFile
andSourceRoot
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.
nit: this test doesn't cover the (unlikely) edge case where the line number is large enough that a left shift turns it negative.