From 637157a1078d34a9f8f51f500f65d0375c5523da Mon Sep 17 00:00:00 2001 From: Teng Zhang Date: Sun, 13 Oct 2024 00:31:49 -0700 Subject: [PATCH] fix check_variant --- .../naming/module_struct_same_name.exp | 33 +++++++++++ .../naming/module_struct_same_name.move | 13 +++++ .../use_struct_overlap_with_module.exp | 7 +-- .../move-model/src/builder/exp_builder.rs | 5 +- .../move-model/src/builder/module_builder.rs | 58 +++++++++++++++---- 5 files changed, 97 insertions(+), 19 deletions(-) create mode 100644 third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.exp create mode 100644 third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.move diff --git a/third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.exp b/third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.exp new file mode 100644 index 0000000000000..5cc6f56d4dbef --- /dev/null +++ b/third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.exp @@ -0,0 +1,33 @@ + +Diagnostics: +warning: unused alias + ┌─ tests/checking/naming/module_struct_same_name.move:8:15 + │ +8 │ use 0x42::M::{Self, M}; + │ ^ Unused 'use' of alias 'M'. Consider removing it + +// -- Model dump before bytecode pipeline +module 0x42::M { + enum M { + M, + } +} // end 0x42::M +module 0x42::M1 { + use 0x42::M::{Self, M}; // resolved as: 0x42::M + private fun test(_m: M::M): u64 { + 3 + } +} // end 0x42::M1 + +// -- Sourcified model before bytecode pipeline +module 0x42::M { + enum M has drop { + M, + } +} +module 0x42::M1 { + use 0x42::M; + fun test(_m: M::M): u64 { + 3 + } +} diff --git a/third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.move b/third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.move new file mode 100644 index 0000000000000..a222ec7a2cb41 --- /dev/null +++ b/third_party/move/move-compiler-v2/tests/checking/naming/module_struct_same_name.move @@ -0,0 +1,13 @@ +module 0x42::M { + enum M has drop { + M + } +} + +module 0x42::M1 { + use 0x42::M::{Self, M}; + + fun test(_m: M::M): u64 { + 3 + } +} diff --git a/third_party/move/move-compiler-v2/tests/more-v1/expansion/use_struct_overlap_with_module.exp b/third_party/move/move-compiler-v2/tests/more-v1/expansion/use_struct_overlap_with_module.exp index ea5bd62d87125..c656af6345167 100644 --- a/third_party/move/move-compiler-v2/tests/more-v1/expansion/use_struct_overlap_with_module.exp +++ b/third_party/move/move-compiler-v2/tests/more-v1/expansion/use_struct_overlap_with_module.exp @@ -6,8 +6,5 @@ warning: unused alias 7 │ use 0x2::X::{Self, S as X}; │ ^ Unused 'use' of alias 'X'. Consider removing it -error: variants not allowed in this context - ┌─ tests/more-v1/expansion/use_struct_overlap_with_module.move:8:27 - │ -8 │ struct A { f1: X, f2: X::S } - │ ^^^^ + +============ bytecode verification succeeded ======== diff --git a/third_party/move/move-model/src/builder/exp_builder.rs b/third_party/move/move-model/src/builder/exp_builder.rs index 871bfcda81727..651680c20c8d1 100644 --- a/third_party/move/move-model/src/builder/exp_builder.rs +++ b/third_party/move/move-model/src/builder/exp_builder.rs @@ -3054,10 +3054,11 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo } // Treat this as a call to a global function. - if !self.parent.check_no_variant(maccess) { + let (no_variant, maccess) = self.parent.check_no_variant_and_convert_maccess(maccess); + if !no_variant { return self.new_error_exp(); } - let (module_name, name, _) = self.parent.module_access_to_parts(maccess); + let (module_name, name, _) = self.parent.module_access_to_parts(&maccess); // Process `old(E)` scoping let is_old = diff --git a/third_party/move/move-model/src/builder/module_builder.rs b/third_party/move/move-model/src/builder/module_builder.rs index 5b5cdaeae904c..9f04fe84d1237 100644 --- a/third_party/move/move-model/src/builder/module_builder.rs +++ b/third_party/move/move-model/src/builder/module_builder.rs @@ -48,7 +48,10 @@ use move_compiler::{ parser::ast as PA, shared::{unique_map::UniqueMap, Identifier, Name}, }; -use move_ir_types::{ast::ConstantName, location::Spanned}; +use move_ir_types::{ + ast::ConstantName, + location::{sp, Spanned}, +}; use regex::Regex; use std::{ cell::RefCell, @@ -250,8 +253,8 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { /// Converts a ModuleAccess into a qualified symbol which can be used for lookup of /// types or functions. If the access has a struct variant, an error is produced. pub fn module_access_to_qualified(&self, access: &EA::ModuleAccess) -> QualifiedSymbol { - self.check_no_variant(access); - let (qsym, _) = self.module_access_to_qualified_with_variant(access); + let (_, access) = self.check_no_variant_and_convert_maccess(access); + let (qsym, _) = self.module_access_to_qualified_with_variant(&access); qsym } @@ -262,15 +265,46 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { ) } - pub fn check_no_variant(&self, maccess: &EA::ModuleAccess) -> bool { - if Self::is_variant(maccess) { - self.parent.env.error( - &self.parent.to_loc(&maccess.loc), - "variants not allowed in this context", - ); - false + /// If `maccess` takes the form `ModuleAccess(M, _, Some(V))`, + /// check `M::V` is a struct/enum, constant or schema, + /// if so, return the form `ModuleAccess(M, V, None)`, + /// see how `maccess` is created by + /// function `name_access_chain` in `expansion/translate.rs` + pub fn check_no_variant_and_convert_maccess( + &self, + maccess: &EA::ModuleAccess, + ) -> (bool, EA::ModuleAccess) { + if let EA::ModuleAccess_::ModuleAccess(mident, _, Some(var_name)) = &maccess.value { + let addr = self + .parent + .resolve_address(&self.parent.to_loc(&mident.loc), &mident.value.address); + let name = self + .symbol_pool() + .make(mident.value.module.0.value.as_str()); + let module_name = ModuleName::from_address_bytes_and_name(addr, name); + let var_name_sym = self.symbol_pool().make(var_name.value.as_str()); + let qualitifed_name = QualifiedSymbol { + module_name, + symbol: var_name_sym, + }; + if self.parent.struct_table.contains_key(&qualitifed_name) + || self.parent.spec_schema_table.contains_key(&qualitifed_name) + || self.parent.const_table.contains_key(&qualitifed_name) + { + let new_maccess = sp( + maccess.loc, + EA::ModuleAccess_::ModuleAccess(*mident, *var_name, None), + ); + (true, new_maccess) + } else { + self.parent.env.error( + &self.parent.to_loc(&maccess.loc), + "variants not allowed in this context", + ); + (false, maccess.clone()) + } } else { - true + (true, maccess.clone()) } } @@ -410,7 +444,7 @@ impl<'env, 'translator> ModuleBuilder<'env, 'translator> { self.symbol_pool().make(n.value.as_str()), ), EA::ModuleAccess_::ModuleAccess(mident, n, _) => { - self.check_no_variant(macc); + let (_, macc) = self.check_no_variant_and_convert_maccess(macc); let addr_bytes = self.parent.resolve_address( &self.parent.to_loc(&macc.loc), &mident.value.address,