Add #[abi_name(name = "foo")] attribute to rename ABI items.
#7057
Conversation
#[abi_name() attribute to rename ABI items.#[abi_name(name = "foo") attribute to rename ABI items.
CodSpeed Performance ReportMerging #7057 will not alter performanceComparing Summary
|
04f5114 to
0bddb9d
Compare
#[abi_name(name = "foo") attribute to rename ABI items.#[abi_name(name = "foo")] attribute to rename ABI items.
test/src/e2e_vm_tests/test_programs/should_pass/language/abi_name_attribute/.gitignore
Outdated
Show resolved
Hide resolved
test/src/e2e_vm_tests/test_programs/should_pass/language/abi_name_attribute/src/lib.sw
Outdated
Show resolved
Hide resolved
test/src/e2e_vm_tests/test_programs/should_pass/language/abi_name_attribute/src/lib.sw
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The attribute should be added to the Attributes chapter in the documentation.
I've changed that documentation page fully in #7058 because it was seriously outdated. So, whoever gets the PR merged later will have some slight merge conflicts to solve on that page 😄
Pull request was converted to draft
0bddb9d to
e45048c
Compare
e45048c to
e364c5c
Compare
e364c5c to
b993940
Compare
b993940 to
a9d5c2a
Compare
a9d5c2a to
9046048
Compare
9046048 to
236cb14
Compare
236cb14 to
be2a5ec
Compare
be2a5ec to
a5e0532
Compare
6e51087 to
295a938
Compare
Add `#[abi_name(name = "foo")` attribute to rename enum and struct ABI
items.
Here is example of how it can be used:
```sway
contract;
struct MyStruct {}
enum MyEnum {
A: ()
}
abi MyAbi {
fn my_struct() -> MyStruct;
fn my_enum() -> MyEnum;
}
impl MyAbi for Contract {
fn my_struct() -> MyStruct { MyStruct{} }
fn my_enum() -> MyEnum { MyEnum::A }
}
```
Closes FuelLabs#5955.
295a938 to
ec8d86b
Compare
|
Rebased, feedback address, ready for re-review. |
There was a problem hiding this comment.
The changes added since the previous review are quite substantial so it will take some time to re-review fully 😄
Here some quick observations and suggestions.
And one general question. I assume the checking was moved to the ABI generation phase, which results in returning Results everywhere and passing Handler all the way down, because there is no option to detect at the type checking phase which types will actually end up in the ABI?
If so, maybe this assumption can be added to the issue description and shortly elaborated, it will help in understanding the reason for the widespread change.
| This means that when a contract ABI JSON file is generated, the name that is output is the one specified | ||
| by the attribute. This can be useful to allow renaming items, while allowing for keeping backwards | ||
| compatibility at the contract ABI level. |
There was a problem hiding this comment.
I would remove "contract" here because scripts and predicates also have ABI JSON generated, and the attribute is effective there as well.
| This means that when a contract ABI JSON file is generated, the name that is output is the one specified | |
| by the attribute. This can be useful to allow renaming items, while allowing for keeping backwards | |
| compatibility at the contract ABI level. | |
| This means that when an ABI JSON file is generated, the name that is output is the one specified | |
| by the attribute. This can be useful to allow renaming items, while allowing for keeping backwards | |
| compatibility at the ABI level. |
|
|
||
| > **Note**: At the moment, only enum and struct types support the attribute. | ||
|
|
||
| In the example that follows, we originally had a `MyStruct` and `MyEnum` types, which we renamed in Sway: |
There was a problem hiding this comment.
I was very confused with this part until I read the conclusion that came after the example. Perhaps formulation along these lines, to make it fully clear:
| In the example that follows, we originally had a `MyStruct` and `MyEnum` types, which we renamed in Sway: | |
| In the example that follows, we originally had `MyStruct` and `MyEnum` types, which we, later on, renamed to "MyStruct" and "MyEnum" in code. To keep the backward compatibility of the ABI, we annotate the types with the `#[abi_name]` attribute and give them the original names: |
| ``` | ||
|
|
||
| We get the same JSON ABI output both before and after renaming the types, due to attributing them with | ||
| `#[abi_name(name = ...)]`, which forces them to be generated with the previous Sway name. |
There was a problem hiding this comment.
| `#[abi_name(name = ...)]`, which forces them to be generated with the previous Sway name. | |
| `#[abi_name(name = ...)]`, which forces them to be generated with their previous Sway names. |
| match *engines.te().get(type_id) { | ||
| TypeInfo::Enum(decl_id) => { | ||
| let enum_decl = engines.de().get_enum(&decl_id); | ||
| if let Some(abi_name_attr) = enum_decl.attributes.abi_name() { |
There was a problem hiding this comment.
Why not match on Option here instead of if else?
| } | ||
| TypeInfo::Struct(decl_id) => { | ||
| let struct_decl = engines.de().get_struct(&decl_id); | ||
| if let Some(abi_name_attr) = struct_decl.attributes.abi_name() { |
| name = "attributes_abi_name" | ||
|
|
||
| [dependencies] | ||
| std = { path = "../../../../../../../sway-lib-std" } |
There was a problem hiding this comment.
We don't need the full-blown std here. We can use the reduced sway-lib-std-core.
| 4 | struct MyStruct {} | ||
| 5 | | ||
| 6 | #[abi_name(name = "SameName")] | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Duplicated name found for renamed ABI type. |
There was a problem hiding this comment.
We can heavily benefit here from expressive diagnostics, by pointing to the source of the other name. Because this name can be a name of the type which is not in the same file, or even of an another one #[abi_name] that is also not in the same file.
The proposal is:
- to use expressive diagnostics to point to the other name.
- to extend the test and cover the cases where the duplicated name is a name of another type, or another
#[abi_name]in the current or some other file.
Another suggestion is to underline only the name itself, "SameName" in this example, because that's where the issue actually is. The rest of the attribute usage is perfectly fine and underlining it gives a misleading impression that the usage of the attribute is wrong.
| | | ||
| 10 | struct MyStruct2 {} | ||
| 11 | | ||
| 12 | #[abi_name(name = "")] |
There was a problem hiding this comment.
Here also, underlining only the "" and not the whole attribute better points to the actual problem.
Personally, I would have expressing diagnostics here as well, explaining that the name must be a valid Sway identifier, and in this case with addition 'and cannot be empty'.
Description
Add
#[abi_name(name = "foo")]attribute to rename enum and struct ABI items.Here is example of how it can be used:
Closes #5955.
Checklist
Breaking*orNew Featurelabels where relevant.