Fix issue with name clash on auto implementation of AbiEncode and AbiDecode #6044
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for a862118Click to view benchmark
|
jjcnn
changed the title
Benchmark for f47438aClick to view benchmark
|
|
@jjcnn Setting I've taken a look a look at the failing tests for references. The failures has nothing to do with references as such. They are caused by the facts that all of those tests have a separate file with traits defined in them (impls.sw), as well as trait implementations. Those traits now do not get properly resolved. Also, there are other E2E tests failing like: They also have traits defined in separate files and used in main.sw. To the E2E tests, the It looks to me that the simple one line change triggered some other issues related to trait import and resolution. One thought crossing my mind. The opening sentence in the PR description:
reminds me a lot of #5500. Perhaps it makes sense to take a closer look on that issue and fix it first. |
|
@ironcev : Thanks - that helps quite a bit. I'll take a look at the issue you linked to, and keep digging. |
…sh_struct_def_glob_import
Benchmark for 410a412Click to view benchmark
|
…sh_struct_def_glob_import
Benchmark for f1ddf14Click to view benchmark
|
Benchmark for 85fd359Click to view benchmark
|
Benchmark for d548db3Click to view benchmark
|
…sh_struct_def_glob_import
Benchmark for d252efdClick to view benchmark
|
Benchmark for 5ef7c7aClick to view benchmark
|
Benchmark for 78ff707Click to view benchmark
|
8 tasks
tritao
reviewed
Jun 7, 2024
Co-authored-by: João Matos <joao@tritao.eu>
Benchmark for 4fecbf8Click to view benchmark
|
IGI-111
approved these changes
Jun 10, 2024
Benchmark for f58cb90Click to view benchmark
|
hal3e
reviewed
Jun 10, 2024
There was a problem hiding this comment.
I tried it with this branch but I still get an error. Unfortunately, I do not get any specifics just: error: Could not generate the entry method. See errors above for more details. and there is nothing above :/.
Steps to recreate:
- clone
fuels-rsand checkouthal3e/name-clash-abi-encode - cd into
fuels-rs/e2e/sway/bindings/type_paths forc buildorforc build --json-abi-with-callpaths
P.S. I installed forc with:
cargo install --locked forc --force --git https://github.com/FuelLabs/sway --branch "jjcnn/name_clash_struct_def_glob_import"
Doing the above does indeed still fail. I originally tried to recreate your test case within the sway repo where it seemed to compile, but it no longer does, so there's something else going on. I'll remove the references to #5864 from this PR, and add your recreation steps to the issue. |
Benchmark for 290fe2fClick to view benchmark
|
tritao
approved these changes
Jun 11, 2024
kayagokalp
approved these changes
Jun 11, 2024
IGI-111
pushed a commit
that referenced
this pull request
Jul 9, 2024
## Description #6044 increased the memory usage of the compiler by an estimated 400%. This PR alleviates some of that increased footprint. `is_external` in the `Module` struct needs to be updated to allow for compilation of dependencies (a module is not external while it is being compiled, but is external when one of its dependents is being compiled). #6044 introduced an extra level of cloning, which in this PR is changed by making `Module` mutable and destructively updating `is_external`. ## 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). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [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.
esdrubal
pushed a commit
that referenced
this pull request
Aug 13, 2024
## Description #6044 increased the memory usage of the compiler by an estimated 400%. This PR alleviates some of that increased footprint. `is_external` in the `Module` struct needs to be updated to allow for compilation of dependencies (a module is not external while it is being compiled, but is external when one of its dependents is being compiled). #6044 introduced an extra level of cloning, which in this PR is changed by making `Module` mutable and destructively updating `is_external`. ## 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). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fixes #5500 . Also fixes the issue that caused one of @xunilrj 's tests to fail.
The callpaths generated for struct declarations have so far been incorrect. The
shadowed_glob_importstest case shows how this causes a compilation error when there is a name clash between a star imported struct and a locally declared struct. This causes a number of issues, in particular that the generated trait impls ofAbiEncodeandAbiDecodecan refer to the wrong type or to a type that doesn't exist.The incorrect paths were the result of a combination of two bugs:
is_externalflag on the containing modules was often set incorrectly. In particular,coreoften hadis_external = false, leading to the package name being added to the path of anything declared incore.This PR fixes both bugs:
is_external = true. This happens inNamespace::init_root().CallPath::ident_to_fullpath().Step 1 unfortunately adds an extra cloning step in the
namespacemodule, but I don't think it's avoidable at the moment. To avoid this extra cloning step we would need to have a better way of building the external module structure than we currently have. #5498 tracks this and other issues.The improved callpath generation should move us a step closer towards fixing #4700.
Checklist
Breaking*orNew Featurelabels where relevant.