Attribute for checking of trivial encoding and decoding#7575
Attribute for checking of trivial encoding and decoding#7575
Conversation
PR SummaryHigh Risk Overview Extends type-layout/triviality machinery by enriching Updates the stdlib codec with Written by Cursor Bugbot for commit e630091. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
|
|
||
| if let Ok(t) = ast_res.typed.as_ref() { | ||
| dbg!(&t.decls_to_check); | ||
| } |
There was a problem hiding this comment.
Debug statement accidentally committed to production
Medium Severity
A dbg! macro call printing decls_to_check was left in the production code path of compile_to_asm. This runs on every compilation, printing debug output to stderr for all users of the compiler.
...src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/src/main.sw
Show resolved
Hide resolved
...src/e2e_vm_tests/test_programs/should_pass/language/attributes/attribute_require/stdout.snap
Show resolved
Hide resolved
| panicking_fn_cache, | ||
| None, | ||
| compiled_fn_cache, | ||
| &[], |
There was a problem hiding this comment.
Trivial check skipped for contracts and predicates
Medium Severity
decls_to_check is extracted from the program but only passed through to compile_entry_function for scripts. For contracts and predicates, &[] is hardcoded, so #[require] attribute checks silently do nothing. The documentation specifically mentions contracts and predicates as supported targets.
Additional Locations (2)
| vec![TypeMetadata::CheckDecl(TyDecl::StructDecl(decl.clone()))] | ||
| } else { | ||
| vec![] | ||
| } |
There was a problem hiding this comment.
Overly broad attribute check collects all attributed structs
Low Severity
The check !d.attributes.is_empty() adds every struct with any attribute (including doc comments, #[cfg], #[deprecated], etc.) to decls_to_check. This is overly broad — only structs with AttributeKind::Require need to be collected, since that's all compile_entry_function processes.
| TypeContent::Bool => MemoryRepresentation::Blob { | ||
| len_in_bytes: 1, | ||
| range: Some(0..2), | ||
| }, |
There was a problem hiding this comment.
Bool runtime representation uses range, breaking triviality equality
Medium Severity
The range field on MemoryRepresentation::Blob is included in the derived PartialEq. The runtime representation for bool uses range: Some(0..2) while the encoding representation uses range: None. This means bool is correctly detected as non-trivially-decodable. However, u8 has range: None in both, so u8 is treated as trivially decodable — yet the documentation table says u8 is trivially decodable, so that's fine. The real concern is enum discriminants: encoding uses range: Some(0..N) but runtime uses range: None (via the Or variant's inner Blobs). This means the range field creates asymmetry that controls triviality purely through equality — an implicit and fragile mechanism that's easy to get wrong as the code evolves.
Additional Locations (1)
| fn trivial_bool_when_invalid_is_valid() { | ||
| let slice = encode(TrivialBool { value: 2 }); | ||
| abi_decode::<TrivialBool>(slice).unwrap(); | ||
| } |
There was a problem hiding this comment.
Duplicate test doesn't verify is_valid method
Medium Severity
trivial_bool_when_invalid_is_valid and trivial_bool_when_invalid_unwrap have identical bodies — both call .unwrap(). The first test's name implies it verifies that is_valid() returns false for an invalid value, but it never calls is_valid(). This means is_valid() on TrivialBool has zero test coverage for the invalid case.
Additional Locations (1)
| span: span.clone(), | ||
| }; | ||
| Ok((intrinsic_function, ctx.engines.te().id_of_u64())) | ||
| } |
There was a problem hiding this comment.
No enum type validation for EnumDiscriminantCount intrinsic
Medium Severity
type_check_enum_discriminant_count validates argument count and type argument count but never verifies the type argument is actually an enum. Since TrivialEnum<T> has no generic constraint requiring T to be an enum, a user could write TrivialEnum<u64> which would pass type checking but hit todo!("ICE") in IR generation, crashing the compiler.
Additional Locations (1)
|
|
||
| // Require Attributes | ||
| pub const REQUIRE_ATTRIBUTE_NAME: &str = "require"; | ||
| pub const REQUIRE_ARG_NAME_TRIVIALLY_ENCODABLE: &str = "trivially_encodable"; |
There was a problem hiding this comment.
Trivially encodable check defined but never enforced
Medium Severity
REQUIRE_ARG_NAME_TRIVIALLY_ENCODABLE is defined and registered as a valid argument for the #[require] attribute, but the checking logic in compile_entry_function only checks for REQUIRE_ARG_NAME_TRIVIALLY_DECODABLE. A user writing #[require(trivially_encodable = "true")] gets no compiler error and no validation — the attribute is silently accepted and ignored, giving a false sense of safety.
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 10 total unresolved issues (including 7 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Encodable require argument is ignored
Medium Severity
run_ir_decl_checks only handles trivially_decodable and never processes trivially_encodable, even though the attribute parser accepts both keys. Using #[require(trivially_encodable = ...)] currently performs no validation.
| } else { | ||
| vec![] | ||
| } | ||
| } |
There was a problem hiding this comment.
Require allowed on enums but never checked
Medium Severity
#[require] is accepted on enums, but enum declarations are excluded from TypeMetadata::CheckDecl collection, so enum annotations never reach the checker.
Additional Locations (1)
| } | ||
| Intrinsic::EnumDiscriminantCount => { | ||
| todo!(); | ||
| } |
There was a problem hiding this comment.
Const-eval intrinsic branch still unimplemented
High Severity
Intrinsic::EnumDiscriminantCount in const-eval calls todo!(). Using this intrinsic in any constant-evaluated context will panic the compiler.
ironcev
changed the title
ironcev
changed the title
1 participant
Description
Closes #7567.
Checklist
Breaking*orNew Featurelabels where relevant.