Associated ABI or trait constant must be declared in impls only if it is not initialized in the ABI or trait
#6343
Assignees
Labels
compiler: frontend
Everything to do with type checking, control flow analysis, and everything between parsing and IRgen
compiler
General compiler. Should eventually become more specific as the issue is triaged
team:compiler
Compiler Team
Comments
ironcev
added
compiler
8 tasks
ironcev
added a commit
that referenced
this issue
Aug 1, 2024
## Description This PR strengthens existing and adds new tests for associated constants. This extended test coverage revealed the following existing issues: #6310, #6348, #6345, #6346, #6343, #6344. It also pointed out that we are missing rules for a contract implementing several ABIs with overlapping interface surfaces, where constants can also be overlapping: #6306. All the issues are linked to tests via TODOs in code that is commented out. We will address the issues in separate PRs. With this PR, we want to first have tests in place. Additionally, the PR: - cleans up dead code that become obsolete when `TyConfigurableDecl` was introduced in #6058. - deletes some redundant tests for constants. - groups some tests to reduce test compilation and execution time and provides guidelines for such groupings in the testing README.md. ## 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). - [ ] 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. - [ ] 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 issue
Aug 13, 2024
## Description This PR strengthens existing and adds new tests for associated constants. This extended test coverage revealed the following existing issues: #6310, #6348, #6345, #6346, #6343, #6344. It also pointed out that we are missing rules for a contract implementing several ABIs with overlapping interface surfaces, where constants can also be overlapping: #6306. All the issues are linked to tests via TODOs in code that is commented out. We will address the issues in separate PRs. With this PR, we want to first have tests in place. Additionally, the PR: - cleans up dead code that become obsolete when `TyConfigurableDecl` was introduced in #6058. - deletes some redundant tests for constants. - groups some tests to reduce test compilation and execution time and provides guidelines for such groupings in the testing README.md. ## 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). - [ ] 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. - [ ] 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.
tritao
added a commit
to tritao/sway
that referenced
this issue
Dec 2, 2024
From the reporting issue:
Current implementation of the initialized associated constants in ABIs
and traits is contradictory. Namely, even if we initialize them, we
anyway always have to implement them in the contract or type `impl`s.
This eliminates the whole purpose of being able to initialize them on
the ABI/trait level.
E.g., for ABIs (and the same is for traits):
```
abi ABI {
const CONST: u32 = 111; // <<<--- Initialized here. But this value
cannot be used, because...
}
impl ABI for Contract {
// ... associated constants MUST always be implemented in the
contract/type and initialized anew.
const CONST: u32 = 222; // <<<--- This must be here and this value
wins.
}
```
What we want to have instead, is the same behavior as in Rust. If an
associated constant is already initialized in the ABI or trait
declaration, the `impl`s do not need to provide the constant. If they
do, the value from the `impl` wins. The existing checks for having the
same constant type in the ABI/trait declaration and `impl`s, of course,
remain.
Fixes FuelLabs#6343.
8 tasks
tritao
added a commit
to tritao/sway
that referenced
this issue
Dec 2, 2024
From the reporting issue:
Current implementation of the initialized associated constants in ABIs
and traits is contradictory. Namely, even if we initialize them, we
anyway always have to implement them in the contract or type `impl`s.
This eliminates the whole purpose of being able to initialize them on
the ABI/trait level.
E.g., for ABIs (and the same is for traits):
```
abi ABI {
const CONST: u32 = 111; // <<<--- Initialized here. But this value
cannot be used, because...
}
impl ABI for Contract {
// ... associated constants MUST always be implemented in the
contract/type and initialized anew.
const CONST: u32 = 222; // <<<--- This must be here and this value
wins.
}
```
What we want to have instead, is the same behavior as in Rust. If an
associated constant is already initialized in the ABI or trait
declaration, the `impl`s do not need to provide the constant. If they
do, the value from the `impl` wins. The existing checks for having the
same constant type in the ABI/trait declaration and `impl`s, of course,
remain.
Fixes FuelLabs#6343.
tritao
added a commit
that referenced
this issue
Dec 4, 2024
## Description [Fix handling of initialized associated constants in ABIs and traits.](88fee13) [Fix impl trait associated constant reference wrongly resolving to the ABI value](3a401f6) Fixes #6343 and #6310. ## 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. --------- Co-authored-by: Sophie Dankel <[email protected]> Co-authored-by: Joshua Batty <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Current implementation of the initialized associated constants in ABIs and traits is contradictory. Namely, even if we initialize them, we anyway always have to implement them in the contract or type
impls. This eliminates the whole purpose of being able to initialize them on the ABI/trait level.E.g., for ABIs (and the same is for traits):
What we want to have instead, is the same behavior as in Rust. If an associated constant is already initialized in the ABI or trait declaration, the
impls do not need to provide the constant. If they do, the value from theimplwins. The existing checks for having the same constant type in the ABI/trait declaration andimpls, of course, remain.The text was updated successfully, but these errors were encountered: