From b489a41d0c1eee44b3a2dc7e9b68b39146e6d557 Mon Sep 17 00:00:00 2001 From: "Celina G. Val" Date: Wed, 15 Jan 2025 13:54:04 -0800 Subject: [PATCH] Refactor contract builtin macro + error handling Instead of parsing the different components of a function signature, eagerly look for either the `where` keyword or the function body. - Also address feedback to use `From` instead of `TryFrom` in cranelift contract and ubcheck codegen. --- .../rustc_builtin_macros/src/contracts.rs | 142 +++++++++--------- compiler/rustc_codegen_cranelift/src/base.rs | 4 +- .../contract-annotation-limitations.rs | 27 ++++ .../contract-annotation-limitations.stderr | 14 ++ .../contracts/contract-attributes-generics.rs | 70 +++++++++ .../disallow-contract-annotation-on-non-fn.rs | 52 +++++++ ...allow-contract-annotation-on-non-fn.stderr | 44 ++++++ 7 files changed, 280 insertions(+), 73 deletions(-) create mode 100644 tests/ui/contracts/contract-annotation-limitations.rs create mode 100644 tests/ui/contracts/contract-annotation-limitations.stderr create mode 100644 tests/ui/contracts/contract-attributes-generics.rs create mode 100644 tests/ui/contracts/disallow-contract-annotation-on-non-fn.rs create mode 100644 tests/ui/contracts/disallow-contract-annotation-on-non-fn.stderr diff --git a/compiler/rustc_builtin_macros/src/contracts.rs b/compiler/rustc_builtin_macros/src/contracts.rs index be7f276cdaa93..fbdd8af99542d 100644 --- a/compiler/rustc_builtin_macros/src/contracts.rs +++ b/compiler/rustc_builtin_macros/src/contracts.rs @@ -35,90 +35,90 @@ impl AttrProcMacro for ExpandEnsures { } } -fn expand_injecting_circa_where_clause( +/// Expand the function signature to include the contract clause. +/// +/// The contracts clause will be injected before the function body and the optional where clause. +/// For that, we search for the body / where token, and invoke the `inject` callback to generate the +/// contract clause in the right place. +/// +// FIXME: this kind of manual token tree munging does not have significant precedent among +// rustc builtin macros, probably because most builtin macros use direct AST manipulation to +// accomplish similar goals. But since our attributes need to take arbitrary expressions, and +// our attribute infrastructure does not yet support mixing a token-tree annotation with an AST +// annotated, we end up doing token tree manipulation. +fn expand_contract_clause( ecx: &mut ExtCtxt<'_>, attr_span: Span, annotated: TokenStream, - inject: impl FnOnce(&mut Vec) -> Result<(), ErrorGuaranteed>, + inject: impl FnOnce(&mut TokenStream) -> Result<(), ErrorGuaranteed>, ) -> Result { - let mut new_tts = Vec::with_capacity(annotated.len()); + let mut new_tts = TokenStream::default(); let mut cursor = annotated.iter(); - // Find the `fn name(x:X,...)` and inject the AST contract forms right after - // the formal parameters (and return type if any). - while let Some(tt) = cursor.next() { - new_tts.push(tt.clone()); - if let TokenTree::Token(tok, _) = tt - && tok.is_ident_named(kw::Fn) - { - break; - } + let is_kw = |tt: &TokenTree, sym: Symbol| { + if let TokenTree::Token(token, _) = tt { token.is_ident_named(sym) } else { false } + }; + + // Find the `fn` keyword to check if this is a function. + if cursor + .find(|tt| { + new_tts.push_tree((*tt).clone()); + is_kw(tt, kw::Fn) + }) + .is_none() + { + return Err(ecx + .sess + .dcx() + .span_err(attr_span, "contract annotations can only be used on functions")); } - // Found the `fn` keyword, now find the formal parameters. - // - // FIXME: can this fail if you have parentheticals in a generics list, like `fn foo Y>` ? - while let Some(tt) = cursor.next() { - new_tts.push(tt.clone()); - - if let TokenTree::Delimited(_, _, token::Delimiter::Parenthesis, _) = tt { - break; - } - if let TokenTree::Token(token::Token { kind: token::TokenKind::Semi, .. }, _) = tt { - panic!("contract attribute applied to fn without parameter list."); + // Found the `fn` keyword, now find either the `where` token or the function body. + let next_tt = loop { + let Some(tt) = cursor.next() else { + return Err(ecx.sess.dcx().span_err( + attr_span, + "contract annotations is only supported in functions with bodies", + )); + }; + // If `tt` is the last element. Check if it is the function body. + if cursor.peek().is_none() { + if let TokenTree::Delimited(_, _, token::Delimiter::Brace, _) = tt { + break tt; + } else { + return Err(ecx.sess.dcx().span_err( + attr_span, + "contract annotations is only supported in functions with bodies", + )); + } } - } - // There *might* be a return type declaration (and figuring out where that ends would require - // parsing an arbitrary type expression, e.g. `-> Foo` - // - // Instead of trying to figure that out, scan ahead and look for the first occurence of a - // `where`, a `{ ... }`, or a `;`. - // - // FIXME: this might still fall into a trap for something like `-> Ctor`. I - // *think* such cases must be under a Delimited (e.g. `[T; { N }]` or have the braced form - // prefixed by e.g. `const`, so we should still be able to filter them out without having to - // parse the type expression itself. But rather than try to fix things with hacks like that, - // time might be better spent extending the attribute expander to suport tt-annotation atop - // ast-annotated, which would be an elegant way to sidestep all of this. - let mut opt_next_tt = cursor.next(); - while let Some(next_tt) = opt_next_tt { - if let TokenTree::Token(tok, _) = next_tt - && tok.is_ident_named(kw::Where) - { - break; - } - if let TokenTree::Delimited(_, _, token::Delimiter::Brace, _) = next_tt { - break; - } - if let TokenTree::Token(token::Token { kind: token::TokenKind::Semi, .. }, _) = next_tt { - break; + if is_kw(tt, kw::Where) { + break tt; } - - // for anything else, transcribe the tt and keep looking. - new_tts.push(next_tt.clone()); - opt_next_tt = cursor.next(); - } + new_tts.push_tree(tt.clone()); + }; // At this point, we've transcribed everything from the `fn` through the formal parameter list // and return type declaration, (if any), but `tt` itself has *not* been transcribed. // // Now inject the AST contract form. // - // FIXME: this kind of manual token tree munging does not have significant precedent among - // rustc builtin macros, probably because most builtin macros use direct AST manipulation to - // accomplish similar goals. But since our attributes need to take arbitrary expressions, and - // our attribute infrastructure does not yet support mixing a token-tree annotation with an AST - // annotated, we end up doing token tree manipulation. inject(&mut new_tts)?; - // Above we injected the internal AST requires/ensures contruct. Now copy over all the other + // Above we injected the internal AST requires/ensures construct. Now copy over all the other // token trees. - if let Some(tt) = opt_next_tt { - new_tts.push(tt.clone()); - } + new_tts.push_tree(next_tt.clone()); while let Some(tt) = cursor.next() { - new_tts.push(tt.clone()); + new_tts.push_tree(tt.clone()); + if cursor.peek().is_none() + && !matches!(tt, TokenTree::Delimited(_, _, token::Delimiter::Brace, _)) + { + return Err(ecx.sess.dcx().span_err( + attr_span, + "contract annotations is only supported in functions with bodies", + )); + } } // Record the span as a contract attribute expansion. @@ -126,7 +126,7 @@ fn expand_injecting_circa_where_clause( // which is gated via `rustc_contracts_internals`. ecx.psess().contract_attribute_spans.push(attr_span); - Ok(TokenStream::new(new_tts)) + Ok(new_tts) } fn expand_requires_tts( @@ -135,16 +135,16 @@ fn expand_requires_tts( annotation: TokenStream, annotated: TokenStream, ) -> Result { - expand_injecting_circa_where_clause(_ecx, attr_span, annotated, |new_tts| { - new_tts.push(TokenTree::Token( + expand_contract_clause(_ecx, attr_span, annotated, |new_tts| { + new_tts.push_tree(TokenTree::Token( token::Token::from_ast_ident(Ident::new(kw::RustcContractRequires, attr_span)), Spacing::Joint, )); - new_tts.push(TokenTree::Token( + new_tts.push_tree(TokenTree::Token( token::Token::new(token::TokenKind::OrOr, attr_span), Spacing::Alone, )); - new_tts.push(TokenTree::Delimited( + new_tts.push_tree(TokenTree::Delimited( DelimSpan::from_single(attr_span), DelimSpacing::new(Spacing::JointHidden, Spacing::JointHidden), token::Delimiter::Parenthesis, @@ -160,12 +160,12 @@ fn expand_ensures_tts( annotation: TokenStream, annotated: TokenStream, ) -> Result { - expand_injecting_circa_where_clause(_ecx, attr_span, annotated, |new_tts| { - new_tts.push(TokenTree::Token( + expand_contract_clause(_ecx, attr_span, annotated, |new_tts| { + new_tts.push_tree(TokenTree::Token( token::Token::from_ast_ident(Ident::new(kw::RustcContractEnsures, attr_span)), Spacing::Joint, )); - new_tts.push(TokenTree::Delimited( + new_tts.push_tree(TokenTree::Delimited( DelimSpan::from_single(attr_span), DelimSpacing::new(Spacing::JointHidden, Spacing::JointHidden), token::Delimiter::Parenthesis, diff --git a/compiler/rustc_codegen_cranelift/src/base.rs b/compiler/rustc_codegen_cranelift/src/base.rs index 2328fae6c5c08..8c3c50580d19e 100644 --- a/compiler/rustc_codegen_cranelift/src/base.rs +++ b/compiler/rustc_codegen_cranelift/src/base.rs @@ -858,7 +858,7 @@ fn codegen_stmt<'tcx>( NullOp::UbChecks => { let val = fx.tcx.sess.ub_checks(); let val = CValue::by_val( - fx.bcx.ins().iconst(types::I8, i64::try_from(val).unwrap()), + fx.bcx.ins().iconst(types::I8, i64::from(val)), fx.layout_of(fx.tcx.types.bool), ); lval.write_cvalue(fx, val); @@ -867,7 +867,7 @@ fn codegen_stmt<'tcx>( NullOp::ContractChecks => { let val = fx.tcx.sess.contract_checks(); let val = CValue::by_val( - fx.bcx.ins().iconst(types::I8, i64::try_from(val).unwrap()), + fx.bcx.ins().iconst(types::I8, i64::from(val)), fx.layout_of(fx.tcx.types.bool), ); lval.write_cvalue(fx, val); diff --git a/tests/ui/contracts/contract-annotation-limitations.rs b/tests/ui/contracts/contract-annotation-limitations.rs new file mode 100644 index 0000000000000..f01d526e3f70e --- /dev/null +++ b/tests/ui/contracts/contract-annotation-limitations.rs @@ -0,0 +1,27 @@ +//! Test for some of the existing limitations and the current error messages. +//! Some of these limitations may be removed in the future. + +#![feature(rustc_contracts)] +#![allow(dead_code)] + +/// Represent a 5-star system. +struct Stars(u8); + +impl Stars { + fn is_valid(&self) -> bool { + self.0 <= 5 + } +} + +trait ParseStars { + #[core::contracts::ensures(|ret| ret.is_none_or(Stars::is_valid))] + //~^ ERROR contract annotations is only supported in functions with bodies + fn parse_string(input: String) -> Option; + + #[core::contracts::ensures(|ret| ret.is_none_or(Stars::is_valid))] + //~^ ERROR contract annotations is only supported in functions with bodies + fn parse(input: T) -> Option where T: for<'a> Into<&'a str>; +} + +fn main() { +} diff --git a/tests/ui/contracts/contract-annotation-limitations.stderr b/tests/ui/contracts/contract-annotation-limitations.stderr new file mode 100644 index 0000000000000..25b01744aac8e --- /dev/null +++ b/tests/ui/contracts/contract-annotation-limitations.stderr @@ -0,0 +1,14 @@ +error: contract annotations is only supported in functions with bodies + --> $DIR/contract-annotation-limitations.rs:17:5 + | +LL | #[core::contracts::ensures(|ret| ret.is_none_or(Stars::is_valid))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: contract annotations is only supported in functions with bodies + --> $DIR/contract-annotation-limitations.rs:21:5 + | +LL | #[core::contracts::ensures(|ret| ret.is_none_or(Stars::is_valid))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + diff --git a/tests/ui/contracts/contract-attributes-generics.rs b/tests/ui/contracts/contract-attributes-generics.rs new file mode 100644 index 0000000000000..87088ce9de2ce --- /dev/null +++ b/tests/ui/contracts/contract-attributes-generics.rs @@ -0,0 +1,70 @@ +//! Test that contracts can be applied to generic functions. + +//@ revisions: unchk_pass chk_pass chk_fail_pre chk_fail_post chk_const_fail +// +//@ [unchk_pass] run-pass +//@ [chk_pass] run-pass +// +//@ [chk_fail_pre] run-fail +//@ [chk_fail_post] run-fail +//@ [chk_const_fail] run-fail +// +//@ [unchk_pass] compile-flags: -Zcontract-checks=no +// +//@ [chk_pass] compile-flags: -Zcontract-checks=yes +//@ [chk_fail_pre] compile-flags: -Zcontract-checks=yes +//@ [chk_fail_post] compile-flags: -Zcontract-checks=yes +//@ [chk_const_fail] compile-flags: -Zcontract-checks=yes + +#![feature(rustc_contracts)] + +use std::ops::Sub; + +/// Dummy fn contract that precondition fails for val < 0, and post-condition fail for val == 1 +#[core::contracts::requires(val > 0u8.into())] +#[core::contracts::ensures(|ret| *ret > 0u8.into())] +fn decrement(val: T) -> T +where T: PartialOrd + Sub + From +{ + val - 1u8.into() +} + +/// Create a structure that takes a constant parameter. +#[allow(dead_code)] +struct Capped(usize); + +/// Now declare a function to create stars which shouldn't exceed 5 stars. +// Add redundant braces to ensure the built-in macro can handle this syntax. +#[allow(unused_braces)] +#[core::contracts::requires(num <= 5)] +unsafe fn stars_unchecked(num: usize) -> Capped<{ 5 }> { + Capped(num) +} + + +fn main() { + check_decrement(); + check_stars(); +} + +fn check_stars() { + // This should always pass. + let _ = unsafe { stars_unchecked(3) }; + + // This violates the contract. + #[cfg(any(unchk_pass, chk_const_fail))] + let _ = unsafe { stars_unchecked(10) }; +} + +fn check_decrement() { + // This should always pass + assert_eq!(decrement(10u8), 9u8); + + // This should fail requires but pass with no contract check. + #[cfg(any(unchk_pass, chk_fail_pre))] + assert_eq!(decrement(-2i128), -3i128); + + // This should fail ensures but pass with no contract check. + #[cfg(any(unchk_pass, chk_fail_post))] + assert_eq!(decrement(1i32), 0i32); +} diff --git a/tests/ui/contracts/disallow-contract-annotation-on-non-fn.rs b/tests/ui/contracts/disallow-contract-annotation-on-non-fn.rs new file mode 100644 index 0000000000000..76ed30e856462 --- /dev/null +++ b/tests/ui/contracts/disallow-contract-annotation-on-non-fn.rs @@ -0,0 +1,52 @@ +//! Checks for compilation errors related to adding contracts to non-function items. + +#![feature(rustc_contracts)] +#![allow(dead_code)] + +#[core::contracts::requires(true)] +//~^ ERROR contract annotations can only be used on functions +struct Dummy(usize); + +#[core::contracts::ensures(|v| v == 100)] +//~^ ERROR contract annotations can only be used on functions +const MAX_VAL: usize = 100; + +// FIXME: Improve the error message here. The macro thinks this is a function. +#[core::contracts::ensures(|v| v == 100)] +//~^ ERROR contract annotations is only supported in functions with bodies +type NewDummy = fn(usize) -> Dummy; + +#[core::contracts::ensures(|v| v == 100)] +//~^ ERROR contract annotations is only supported in functions with bodies +const NEW_DUMMY_FN : fn(usize) -> Dummy = || { Dummy(0) }; + +#[core::contracts::requires(true)] +//~^ ERROR contract annotations can only be used on functions +impl Dummy { + + // This should work + #[core::contracts::ensures(|ret| ret.0 == v)] + fn new(v: usize) -> Dummy { + Dummy(v) + } +} + +#[core::contracts::ensures(|dummy| dummy.0 > 0)] +//~^ ERROR contract annotations can only be used on functions +impl From for Dummy { + // This should work. + #[core::contracts::ensures(|ret| ret.0 == v)] + fn from(value: usize) -> Self { + Dummy::new(value) + } +} + +/// You should not be able to annotate a trait either. +#[core::contracts::requires(true)] +//~^ ERROR contract annotations can only be used on functions +pub trait DummyBuilder { + fn build() -> Dummy; +} + +fn main() { +} diff --git a/tests/ui/contracts/disallow-contract-annotation-on-non-fn.stderr b/tests/ui/contracts/disallow-contract-annotation-on-non-fn.stderr new file mode 100644 index 0000000000000..4d6d23340ac0e --- /dev/null +++ b/tests/ui/contracts/disallow-contract-annotation-on-non-fn.stderr @@ -0,0 +1,44 @@ +error: contract annotations can only be used on functions + --> $DIR/disallow-contract-annotation-on-non-fn.rs:6:1 + | +LL | #[core::contracts::requires(true)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: contract annotations can only be used on functions + --> $DIR/disallow-contract-annotation-on-non-fn.rs:10:1 + | +LL | #[core::contracts::ensures(|v| v == 100)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: contract annotations is only supported in functions with bodies + --> $DIR/disallow-contract-annotation-on-non-fn.rs:15:1 + | +LL | #[core::contracts::ensures(|v| v == 100)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: contract annotations is only supported in functions with bodies + --> $DIR/disallow-contract-annotation-on-non-fn.rs:19:1 + | +LL | #[core::contracts::ensures(|v| v == 100)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: contract annotations can only be used on functions + --> $DIR/disallow-contract-annotation-on-non-fn.rs:23:1 + | +LL | #[core::contracts::requires(true)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: contract annotations can only be used on functions + --> $DIR/disallow-contract-annotation-on-non-fn.rs:34:1 + | +LL | #[core::contracts::ensures(|dummy| dummy.0 > 0)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: contract annotations can only be used on functions + --> $DIR/disallow-contract-annotation-on-non-fn.rs:45:1 + | +LL | #[core::contracts::requires(true)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors +