[sui-mode][move-compiler] Fixed init bug. Improved other cases #14415
Conversation
- Fixed bug with OTW checks - Improved init error messages
tnowacki
requested review from
amnn,
cgswords,
awelc,
tzakian,
damirka and
dariorussi
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
| Some(sp!(_, TypeName_::ModuleType(m, n))) | ||
| if m == context.current_module() && n.value() == otw_name | ||
| ); | ||
| let is_otw = matches!(&first_ty.value, Type_::UnresolvedError | Type_::Var(_)) |
There was a problem hiding this comment.
This is the only line that I don't really follow. Is this meant to allow for good error recovery for something like the following two?
module a::boop {
fun init(_: BOOP, _: &mut sui::tx_context::TxContext) { ... }
}
and
module a::boop {
fun init<BOOP>(_: BOOP, _: &mut sui::tx_context::TxContext) { ... }
}
If so it may be worthwhile adding tests for these cases.
There was a problem hiding this comment.
I added tests for at least the first one. Type::Var(_) would be an unexpanded type variable.
This is really trying to mimic what would happen if this was done during type checking instead... and maybe I should move some of that logic there
| │ | ||
| = One-time witness types are structs with the following requirements: their name is the upper-case version of the module's name, they have no fields (or a single boolean field), they have no type parameters, and they have only the 'drop' ability. | ||
|
|
||
| error[Sui E02004]: invalid one-time witness declaration |
There was a problem hiding this comment.
- Do we want to emit an error for each field? I think one error is enough here, otherwise things could get noisy.
- Is it worth clarifying that the reason this is a problem is because it was passed to
init? The current wording kind of assumes the conclusion (i.e. thatBEEPis anOTW) but then contradicts itself, because ...it's not a OTW.
How about something like following wording:
The first parameter to a two parameter module initializer must be a one-time witness, meaning: its name is the upper-case version of the module's name, it has no fields (or a single boolean field), no type parameters, and only the 'drop' ability.
There was a problem hiding this comment.
Yeah... I think maybe one message with a note/full description is probably better. This is what I had before and should go back to it.
Is it worth clarifying that the reason this is a problem is because it was passed to init?
Isn't that what I do? I point to the usage in init for these cases
There was a problem hiding this comment.
Isn't that what I do? I point to the usage in init for these cases
Yes, in terms pointing to the correct location in source, but my comment was about the wording, which read confusingly to me, because it seemed to imply the type both was and wasn't an OTW (the annotation on the source location implied it was an OTW, but then the rules about OTWs and the other annotation suggested otherwise).
It would be valid to say that that is kind of the point (as in, that's the error), but for whatever reason, the softening of "Possible attempted usage..." seems to sit better with me (although, nit, you could drop the "Possible") -- it implies an ordering (e.g. you're in this situation because you attempted this, and that caused a problem here), which follows because the reason a type is considered an OTW is first because it shows up in the init parameters, and only subsequently because it meets the criteria.
| let mut check_field_loc = |invalid_field_loc_opt| { | ||
| let Some(invalid_field_loc) = invalid_field_loc_opt else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
As per my earlier comment, this would become (modulo fixes to nesting lower down):
| let mut check_field_loc = |invalid_field_loc_opt| { | |
| let Some(invalid_field_loc) = invalid_field_loc_opt else { | |
| return; | |
| }; | |
| let Some(invalid_field_loc) = invalid_first_field.or(more_than_one_field) else { | |
| return; | |
| }; |
…nLabs#14415) ## Description - Fixed bug with OTW checks - Improved init error messages ## Test Plan - Added tests --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [X] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes This release fixes a bug in the Sui mode of the Move compiler that categorized certain structs erroneously as one-time witnesses.
Description
Test Plan
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes
This release fixes a bug in the Sui mode of the Move compiler that categorized certain structs erroneously as one-time witnesses.