-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Add support for parsing debug data attributes #14857
base: develop
Are you sure you want to change the base?
Conversation
47e3966
to
be1913d
Compare
b17a4f7
to
fe70d58
Compare
a0eb445
to
84df68d
Compare
8221ce1
to
6396d56
Compare
983f48b
to
d2bb60a
Compare
6396d56
to
e983c75
Compare
7d1853c
to
0ab7dbc
Compare
e983c75
to
539a523
Compare
0ab7dbc
to
cb2b661
Compare
c0710bf
to
87d86bf
Compare
a783743
to
161f0b1
Compare
161f0b1
to
34249a2
Compare
} | ||
} | ||
// ---- | ||
// SyntaxError 5721: (37-65): @debug.merge: Could not parse debug data: parse error at line 1, column 12: syntax error while parsing object - unexpected end of input; expected '}' |
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 always error out now on invalid @debug.*
attributes? What happened before if we had this kind of attribute? If this compiled before the change, we need to wonder if this is breaking (and should only be parsed if enabled somehow) - even though it may be fine in practice. But can you explain how this behaved before the PR?
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.
@cameel what would you say to this? this is technically breaking, but would you say its ok?
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 think I will just base on #15289. So attribute parsing is only enabled, if ethdebug was requested.
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.
Looks like this would be ignored before and now it is an error. I checked now and I see that when we introduced the @src
tag we did it like this too - solc 0.8.8 just started issuing errors on invalid @src
tags. So there is a precedent for ignoring such minor breaking changes.
Similarly in #15209, we were careful not to be completely breaking, but it was still too constraining to keep it 100% compatible for every minor irrelevant detail - e.g. now single quotes are supported and you'll get an error on an unmatched quote, which previously would have been not recognized as the beginning of a string and ignored.
So I'd say it's fine if it's a minor thing unlikely to happen in practical use and the alternative requires too much effort. It should still be documented in the changelog though. Looks like you found a good alternative though - parsing it only with ethdebug enabled - so I'd go with that.
Depends on