-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
__log outputting encoded values #5306
Conversation
a759b88
to
34e3ad8
Compare
34e3ad8
to
29da837
Compare
55a2da1
to
db632a2
Compare
Some tests are breaking because of #5383. The problem is that when we generate the auto impl, it ends up resolving to the wrong types. |
The task to test std-lib does not work, because it depends on the SDK parsing the log correctly. Something we still need to do. |
c09df0c
to
c42bde1
Compare
## Description Closes #5383. #5383 blocks PR #5306. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [ ] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
c42bde1
to
48ae9bd
Compare
I'm not sufficiently familiar with most of the code being touched here, so I don't feel comfortable giving a review a such. I did want to raise the issue of whether it's possible to go from the log output back to the data? The type of the data doesn't seem to be part of the encoding, so to map the log back to the data it came from you would need to supply the types separately. |
Only looking at the bytes... no. But this was a explicit decision (that we can discuss if is a good one or not).
The test that test this is the one broken because we need to update the Rust SDK. What we do in that test is running a macro on top on this JSON that "recreate" all the types and give you easy access to all typed logged data. Unfortunately both repos are dependent on one another. :( |
100dc5a
to
6676f97
Compare
b1ee16b
to
9ed5396
Compare
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.
A few nitpicks but LGTM
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 good, excited to see this land.
…5481) ## Description This PR is a continuation of #5306. - I am fixing some code reviews issues that were raised in the other PR; - I am incorporating the encoding version inside the JSON ABI as: ```json { "configurables": [], "encoding": "1", <- look here "functions": [ { "attributes": null, "inputs": [], "name": "main", "output": { "name": "", "type": 13, "typeArguments": null } } ], ``` This field is a string to allow any kind of versioning we choose. - This PR has also improved testing and making more explicit how each type is being encoded. ## Dependencies - [x] FuelLabs/fuel-abi-types#17 ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
…5481) ## Description This PR is a continuation of #5306. - I am fixing some code reviews issues that were raised in the other PR; - I am incorporating the encoding version inside the JSON ABI as: ```json { "configurables": [], "encoding": "1", <- look here "functions": [ { "attributes": null, "inputs": [], "name": "main", "output": { "name": "", "type": 13, "typeArguments": null } } ], ``` This field is a string to allow any kind of versioning we choose. - This PR has also improved testing and making more explicit how each type is being encoded. ## Dependencies - [x] FuelLabs/fuel-abi-types#17 ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
…5481) This PR is a continuation of #5306. - I am fixing some code reviews issues that were raised in the other PR; - I am incorporating the encoding version inside the JSON ABI as: ```json { "configurables": [], "encoding": "1", <- look here "functions": [ { "attributes": null, "inputs": [], "name": "main", "output": { "name": "", "type": 13, "typeArguments": null } } ], ``` This field is a string to allow any kind of versioning we choose. - This PR has also improved testing and making more explicit how each type is being encoded. - [x] FuelLabs/fuel-abi-types#17 - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
## Description Enables a did_change stress test with random wait times between each trigger to emulate random key typing. This then internally kicks off compilation and triggers garbage collection every 3 keystrokes. I had intended to include this test when garbage collection was implemented in #5251. However, at that time, we were only performing GC every 10th keystroke and it was overloading the stack in CI. Now that we have reduced this to every 3rd keystroke it seems to be fine for CI. Unfortunately, this test wasn't running to catch a bug that slipped through in #5306. As such, this PR currently allows a way for us to reproduce this error but won't be able to pass CI until the underlying issue is resolved. P.S.: This also adds a temporary fix where we retain elements without a source id. This is tantamount to leaking them but seems preferable to disabling the GC altogether. --------- Co-authored-by: xunilrj <xunilrj@hotmail.com> Co-authored-by: IGI-111 <igi-111@protonmail.com>
Description
This PR changes the output of
__log
to encoded values (see #4769). A quick example of what that means iswill output
This works in two steps:
1 -
__log(s)
is desugared into__log(encode(s))
;2 - call
encode
. Its impl can be found at https://github.com/FuelLabs/sway/pull/5306/files#diff-ee5cebc963e841e8af05f3986de17dd266ee6e9b49dbe089a5eb64764f3b802eR307It simply creates an append-only buffer and call
abi_encode
from a special trait namedAbiEncode
.To be encodable, a type must implement
AbiEncode
. In the example above,S
is auto-implemented by the compiler because all its fields areAbiEncode
. But we can also have custom impl likeVec
here: https://github.com/FuelLabs/sway/pull/5306/files#diff-b5d9688741fea479477f26ca44cd1d1ecbd2f003f3875292abb23df7fad85c58All this is behind a compiler flag:
The same flag is available for the
e2e
tests:Limitations
1 - Now that __log demands a
fn
calledencode
, when something is compiled withimplicit-std: false
, the intrinsic function__log
does not work out-of-box anymore. A function calledencode
must "visible" and the worst part is that it needs to be functional to log anything.2 - Arrays, string arrays and tuples will have limited implementations. Currently up to five items.
Checklist
Breaking*
orNew Feature
labels where relevant.