Conversation
| /// Provider of the Solidity sources that the Yul code was generated from. | ||
| /// Necessary when code snippets are requested as a part of debug info. When null, code snippets are omitted. | ||
| /// NOTE: Not owned by YulStack, the user must ensure that it is not destoroyed before the stack is. | ||
| langutil::CharStreamProvider const* m_soliditySourceProvider{}; |
There was a problem hiding this comment.
I think that the best solution is to just store a pointer to the source provider (i.e. CompilerStack in YulStack. This will require us to worry about its lifetime, but on the other hand we'll no longer have any issues with things being printed differently due to debug info.
There was a problem hiding this comment.
I don't think that the lifetime is a big worry in this case, so fully agree!
|
|
||
| Bugfixes: | ||
| * SMTChecker: Fix SMT logic error when assigning to an array of addresses. | ||
| * Yul AST: Fix shifted native source locations when debug info selection included code snippets. |
There was a problem hiding this comment.
Not sure if we want to have a changelog entry for this one. Technically, we never made Yul AST output non-experimental. On the other hand, the feature has been there for a while and it's still nice to have a record of what changed.
There was a problem hiding this comment.
I think it is good to have some documentation, even if the feature is experimental. Could be worth clarifying in the changelog that it is indeed an experimental feature, though?
There was a problem hiding this comment.
Yeah, I think it is ok to have a changelog entry here, since we also included it when the feature was added in https://github.com/ethereum/solidity/pull/14177/files#diff-c275fec9c453c2a42515bc5ab47e30fa4130ff1426aef2f6e000a9a34e122cb8R8
There was a problem hiding this comment.
since we also included it when the feature was added in
Yeah, but that was actually an oversight back then :) I was meant to be experimental. And also we do have entries for introduction of experimental features, just not necessarily for updates to them. But given that it was not marked as experimental, perhaps this one is fine not being marked as such either...
cameel
force-pushed
the
fix-code-snippets-shifting-yul-ast-native-src-locations
branch
from
995da44 to
614f312
Compare
|
Looks good to me aside from the mini typo :) |
cameel
force-pushed
the
fix-code-snippets-shifting-yul-ast-native-src-locations
branch
from
614f312 to
4216aa7
Compare
cameel
deleted the
fix-code-snippets-shifting-yul-ast-native-src-locations
branch
3 participants
This fixes the issue with shifted source locations we're seeing in #15451 (comment). Or, more accurately, turns it into a proper fix, because the shift is actually correct and it's the test expectation that was wrong.
Turns out that my assumption in #15309 that we don't need access to code snippets in
YulStackwhen reparsing Obviously, if we parse different source than we output, we'll get wrong AST locations.I think that Yul AST is the only artifact affected. We normally only include origin source locations in the output and print nothing if the native location is the only thing we have. The only way to see native locations is to look into the AST.