Attribute for checking of trivial encoding and decoding#7575
Attribute for checking of trivial encoding and decoding#7575
Conversation
PR SummaryMedium Risk Overview The compiler now runs a new post-typecheck declaration check that evaluates Stdlib Reviewed by Cursor Bugbot for commit a09b65c. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 4 issues found in the latest run.
- ✅ Fixed: Debug statement accidentally committed to production
- Removed the stray
dbg!(&t.decls_to_check)call fromcompile_to_asmso compilation no longer emits unintended stderr output.
- Removed the stray
- ✅ Fixed: Misplaced test: non-trivially-decodable struct in should_pass
- Updated the
attribute_requireshould-pass program to use trivially decodable field types (u64and nestedu64struct) so it now matches its expected passing placement.
- Updated the
- ✅ Fixed: Attribute value required but silently ignored in check
- The require check now reads
arg.valueand only enforcestrivially_decodablewhen the value is explicitly true ("true"ortrue).
- The require check now reads
- ✅ Fixed: Require attribute check skipped for predicates and contracts
- Threaded
decls_to_checkthrough predicate and contract compilation paths so#[require(trivially_decodable = "true")]is validated there too.
- Threaded
Or push these changes by commenting:
@cursor push 73287ee042
Preview (73287ee042)
diff --git a/sway-core/src/ir_generation.rs b/sway-core/src/ir_generation.rs
--- a/sway-core/src/ir_generation.rs
+++ b/sway-core/src/ir_generation.rs
@@ -399,6 +399,7 @@
&mut panicking_fn_cache,
&test_fns,
&mut compiled_fn_cache,
+ decls_to_check,
),
ty::TyProgramKind::Contract {
entry_function,
@@ -409,6 +410,7 @@
abi_entries,
namespace,
declarations,
+ decls_to_check,
&logged_types,
&messages_types,
panic_occurrences,
diff --git a/sway-core/src/ir_generation/compile.rs b/sway-core/src/ir_generation/compile.rs
--- a/sway-core/src/ir_generation/compile.rs
+++ b/sway-core/src/ir_generation/compile.rs
@@ -1,9 +1,19 @@
use crate::{
- Engines, PanicOccurrences, PanickingCallOccurrences, TypeInfo, decl_engine::{DeclEngineGet, DeclId, DeclRefFunction}, ir_generation::{
- KeyedTyFunctionDecl, PanickingFunctionCache, convert::convert_resolved_typeid_no_span
- }, language::{
- Visibility, ty::{self, StructDecl, TyDecl}
- }, metadata::MetadataManager, namespace::ResolvedDeclaration, semantic_analysis::namespace, transform::AttributeKind, type_system::TypeId, types::{LogId, MessageId}
+ decl_engine::{DeclEngineGet, DeclId, DeclRefFunction},
+ ir_generation::{
+ convert::convert_resolved_typeid_no_span, KeyedTyFunctionDecl, PanickingFunctionCache,
+ },
+ language::{
+ ty::{self, StructDecl, TyDecl},
+ Visibility,
+ },
+ metadata::MetadataManager,
+ namespace::ResolvedDeclaration,
+ semantic_analysis::namespace,
+ transform::AttributeKind,
+ type_system::TypeId,
+ types::{LogId, MessageId},
+ Engines, PanicOccurrences, PanickingCallOccurrences, TypeInfo,
};
use super::{
@@ -13,7 +23,7 @@
CompiledFunctionCache,
};
-use sway_ast::attribute::REQUIRE_ARG_NAME_TRIVIALLY_DECODABLE;
+use sway_ast::{attribute::REQUIRE_ARG_NAME_TRIVIALLY_DECODABLE, Literal};
use sway_error::{error::CompileError, handler::Handler};
use sway_ir::{metadata::combine as md_combine, *};
use sway_types::{Ident, Span, Spanned};
@@ -104,6 +114,7 @@
panicking_fn_cache: &mut PanickingFunctionCache,
test_fns: &[(Arc<ty::TyFunctionDecl>, DeclRefFunction)],
compiled_fn_cache: &mut CompiledFunctionCache,
+ decls_to_check: &[TyDecl],
) -> Result<Module, Vec<CompileError>> {
let module = Module::new(context, Kind::Predicate);
@@ -138,7 +149,7 @@
panicking_fn_cache,
None,
compiled_fn_cache,
- &[],
+ decls_to_check,
)?;
compile_tests(
engines,
@@ -164,6 +175,7 @@
abi_entries: &[DeclId<ty::TyFunctionDecl>],
namespace: &namespace::Namespace,
declarations: &[ty::TyDecl],
+ decls_to_check: &[TyDecl],
logged_types_map: &HashMap<TypeId, LogId>,
messages_types_map: &HashMap<TypeId, MessageId>,
panic_occurrences: &mut PanicOccurrences,
@@ -217,7 +229,7 @@
panicking_fn_cache,
None,
compiled_fn_cache,
- &[],
+ decls_to_check,
)?;
} else {
// In the case of the encoding v0, we need to compile individual ABI entries
@@ -602,7 +614,18 @@
for (_, atts) in atts {
for att in atts.iter() {
for arg in att.args.iter() {
- if arg.name.as_str() == REQUIRE_ARG_NAME_TRIVIALLY_DECODABLE && !is_type_trivially_decodable(*decl_id) {
+ let requires_trivially_decodable = matches!(
+ &arg.value,
+ Some(Literal::String(val)) if val.parsed == "true"
+ ) || matches!(
+ &arg.value,
+ Some(Literal::Bool(val)) if val.kind.into()
+ );
+
+ if arg.name.as_str() == REQUIRE_ARG_NAME_TRIVIALLY_DECODABLE
+ && requires_trivially_decodable
+ && !is_type_trivially_decodable(*decl_id)
+ {
let mut infos = vec![];
let mut helps = vec![];
let mut bottom_helps = BTreeSet::new();
diff --git a/sway-core/src/lib.rs b/sway-core/src/lib.rs
--- a/sway-core/src/lib.rs
+++ b/sway-core/src/lib.rs
@@ -1188,10 +1188,6 @@
experimental,
)?;
- if let Ok(t) = ast_res.typed.as_ref() {
- dbg!(&t.decls_to_check);
- }
-
ast_to_asm(handler, engines, &ast_res, build_config, experimental)
}
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/another_file.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/another_file.sw
--- a/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/another_file.sw
+++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/another_file.sw
@@ -1,5 +1,5 @@
library;
pub struct InnerStruct {
- pub a: bool,
-}
\ No newline at end of file
+ pub a: u64,
+}
diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/main.sw
--- a/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/main.sw
+++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/main.sw
@@ -5,21 +5,15 @@
#[require(trivially_decodable = "true")]
struct MyStruct {
- a: bool,
+ a: u64,
b: InnerStruct,
- c: SomeEnum,
- d: Vec<u64>,
+ c: u64,
+ d: u64,
}
-enum SomeEnum {
- A: ()
-}
-
fn main(s: MyStruct) {
__log(s.a);
__log(s.b.a);
__log(s.c);
__log(s.d);
- let a = SomeEnum::A;
- __log(a);
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 11 total unresolved issues (including 8 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Debug
eprintln!left in production code path- I removed the unconditional
eprintln!block fromtype_check_finalizeforEnumVariantsValuesso no debug output is emitted during normal compilation.
- I removed the unconditional
- ✅ Fixed:
EnumVariantsValuesintrinsic hardcodestruefor all variants- I replaced the hardcoded
truewith per-variant triviality computation by comparing each variant type’s encoding and runtime memory representations.
- I replaced the hardcoded
- ✅ Fixed: Type check hardcodes literal
3, panics otherwise- I removed the literal-
3match andtodo!()panic path, corrected the expected argument count to 1, and return a proper intrinsic type error for non-enum type arguments.
- I removed the literal-
Or push these changes by commenting:
@cursor push 64daa5eea7
Preview (64daa5eea7)
diff --git a/sway-core/src/ir_generation/function.rs b/sway-core/src/ir_generation/function.rs
--- a/sway-core/src/ir_generation/function.rs
+++ b/sway-core/src/ir_generation/function.rs
@@ -2477,7 +2477,7 @@
let elems = decl
.variants
.iter()
- .map(|_variant| {
+ .map(|variant| {
// let codec = namespace.module_from_absolute_path(&[
// BaseIdent::new_no_span("std".to_string()),
// BaseIdent::new_no_span("codec".to_string()),
@@ -2526,9 +2526,27 @@
// let value = r.get_content(context).as_bool().unwrap();
// dbg!(value);
- ConstantContent::new_bool(context, true)
+ let variant_type_info =
+ engines.te().get(variant.type_argument.type_id);
+ let encoding_representation =
+ get_encoding_representation(engines, &variant_type_info);
+ let runtime_type = convert_resolved_type_id(
+ self.engines,
+ context,
+ md_mgr,
+ self.module,
+ Some(self),
+ variant.type_argument.type_id,
+ &variant.type_argument.span,
+ )?;
+ let runtime_representation =
+ Some(get_runtime_representation(context, runtime_type));
+
+ let is_decode_trivial =
+ encoding_representation == runtime_representation;
+ Ok(ConstantContent::new_bool(context, is_decode_trivial))
})
- .collect::<Vec<_>>();
+ .collect::<Result<Vec<_>, CompileError>>()?;
let elems_len = ConstantContent::new_uint(context, 64, elems.len() as u64);
let elems_len = Constant::unique(context, elems_len);
diff --git a/sway-core/src/language/ty/expression/expression_variant.rs b/sway-core/src/language/ty/expression/expression_variant.rs
--- a/sway-core/src/language/ty/expression/expression_variant.rs
+++ b/sway-core/src/language/ty/expression/expression_variant.rs
@@ -1330,13 +1330,6 @@
for expr in kind.arguments.iter_mut() {
expr.type_check_finalize(handler, ctx)?;
}
-
- if let sway_ast::Intrinsic::EnumVariantsValues = kind.kind {
- eprintln!(
- "type_check_finalize: {:?}",
- ctx.engines.help_out(&kind.type_arguments)
- );
- }
}
TyExpressionVariant::AbiName(_) => {
todo!("")
diff --git a/sway-core/src/semantic_analysis/ast_node/expression/intrinsic_function.rs b/sway-core/src/semantic_analysis/ast_node/expression/intrinsic_function.rs
--- a/sway-core/src/semantic_analysis/ast_node/expression/intrinsic_function.rs
+++ b/sway-core/src/semantic_analysis/ast_node/expression/intrinsic_function.rs
@@ -187,7 +187,7 @@
if arguments.len() != 1 {
return Err(handler.emit_err(CompileError::IntrinsicIncorrectNumArgs {
name: kind.to_string(),
- expected: 0,
+ expected: 1,
span,
}));
}
@@ -198,11 +198,6 @@
ty::TyExpression::type_check(handler, ctx, &arguments[0])?
};
- let _value_id = match first_argument_typed_expr.expression {
- ty::TyExpressionVariant::Literal(Literal::U64(3)) => 3,
- _ => todo!(),
- };
-
let arguments = vec![first_argument_typed_expr];
if type_arguments.len() != 1 {
@@ -233,12 +228,13 @@
let return_type = ctx.engines.te().insert_slice(ctx.engines, elem_type);
match &*ctx.engines.te().get(arg) {
- TypeInfo::UnknownGeneric { .. } => {}
- TypeInfo::Enum(_) => {
- todo!();
- }
+ TypeInfo::UnknownGeneric { .. } | TypeInfo::Enum(_) => {}
_ => {
- todo!()
+ return Err(handler.emit_err(CompileError::IntrinsicUnsupportedArgType {
+ name: kind.to_string(),
+ span: span.clone(),
+ hint: "Type argument must be an enum type.".to_string(),
+ }));
}
};This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
65ab635 to
8b042ca
Compare
8cc3fab to
03bb4ff
Compare
|
I addressed both new BugBot findings in the latest code:
|
aa76235 to
1a7152d
Compare
|
@cursor BugBot run |
|
The PR author's force push (
|
- Replace table.get(&field_fullname).unwrap() with proper match/error handling, consistent with the pattern already used for the top-level type lookup. Prevents compiler panic when a field type's is_decode_trivial expression cannot be evaluated to a constant bool. - Replace todo!() in push_help_if_non_trivially_decodable_type with an empty match arm for unhandled type_decl_span cases, since no specific help message is needed for unexpected types and the compiler should not crash. Co-authored-by: Daniel Frederico Lins Leite <xunilrj@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a09b65c. Configure here.
| 1 => true, | ||
| _ => __revert(INVALID_BOOL_REVERT), | ||
| } | ||
| } |
There was a problem hiding this comment.
TrivialBool methods missing pub visibility modifier
High Severity
TrivialBool's is_valid and unwrap methods are missing the pub keyword, making them inaccessible to external users. The documentation explicitly shows users calling .unwrap() on TrivialBool values. The equivalent methods on TrivialEnum are correctly marked pub. Without pub, the entire purpose of TrivialBool as a user-facing wrapper type is defeated.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a09b65c. Configure here.
| is_decl: bool, | ||
| tid: TypeId, | ||
| type_name_span: Option<Span>| | ||
| -> Option<CheckDecl> { |
There was a problem hiding this comment.
nit: I'd prefer to have the decl handled with an enum, so that the unwrap() below becomes unnecessary. Perhaps something like this:
enum Ty {
Ref(Span),
Decl,
}| // special types | ||
| let full_type = engines.help_out(type_info).to_string(); | ||
| if full_type.starts_with("std::vec::Vec<") { | ||
| let aray_type_name = full_type |
| .replace("std::vec::Vec<", "[") | ||
| .replace(">", "; 64]"); |
There was a problem hiding this comment.
This breaks if any nested types are generic
| #[error("Invalid argument.")] | ||
| InvalidArgument { span: Span }, |
There was a problem hiding this comment.
It looks like this is never used?





Description
Closes #7567.
Checklist
Breaking*orNew Featurelabels where relevant.