From e40ad80332fd84465d8163ef71e302497b5132cf Mon Sep 17 00:00:00 2001 From: joshua-maros <60271685+joshua-maros@users.noreply.github.com> Date: Sat, 16 Oct 2021 11:42:12 -0700 Subject: [PATCH] Fix covariance detection --- examples/src/lib.rs | 1 - examples/src/ok_tests.rs | 4 - ouroboros_macro/src/covariance_detection.rs | 81 ++++++++++++--------- ouroboros_macro/src/parse.rs | 11 ++- 4 files changed, 54 insertions(+), 43 deletions(-) diff --git a/examples/src/lib.rs b/examples/src/lib.rs index f7c8c45..ba05c68 100644 --- a/examples/src/lib.rs +++ b/examples/src/lib.rs @@ -30,7 +30,6 @@ pub struct DocumentationExample { #[borrows(int_data)] int_reference: &'this i32, #[borrows(mut float_data)] - #[not_covariant] float_reference: &'this mut f32, } diff --git a/examples/src/ok_tests.rs b/examples/src/ok_tests.rs index 4481928..918dda8 100644 --- a/examples/src/ok_tests.rs +++ b/examples/src/ok_tests.rs @@ -7,7 +7,6 @@ use std::fmt::Debug; struct TraitObject { data: Box, #[borrows(data)] - #[covariant] dref: &'this dyn Debug, } @@ -22,7 +21,6 @@ struct BoxAndRef { struct BoxAndMutRef { data: i32, #[borrows(mut data)] - #[not_covariant] dref: &'this mut i32, } @@ -39,7 +37,6 @@ struct ChainedAndUndocumented { struct BoxCheckWithLifetimeParameter<'t> { external_data: &'t (), #[borrows(external_data)] - #[covariant] self_reference: &'this &'t (), } @@ -67,7 +64,6 @@ where data2: &'this C, data3: B, #[borrows(mut data3)] - #[not_covariant] data4: &'this mut C, } diff --git a/ouroboros_macro/src/covariance_detection.rs b/ouroboros_macro/src/covariance_detection.rs index 9f14de0..9ef8b23 100644 --- a/ouroboros_macro/src/covariance_detection.rs +++ b/ouroboros_macro/src/covariance_detection.rs @@ -40,88 +40,99 @@ pub fn apparent_std_container_type(raw_type: &Type) -> Option<(&'static str, &Ty None } -/// Returns true if the specified type can be assumed to be covariant. -pub fn type_is_covariant(ty: &syn::Type, in_template: bool) -> bool { +/// Returns Some(true or false) if the type is known to be covariant / not covariant. +pub fn type_is_covariant_over_this_lifetime(ty: &syn::Type) -> Option { use syn::Type::*; // If the type never uses the 'this lifetime, we don't have to // worry about it not being covariant. if !uses_this_lifetime(ty.to_token_stream()) { - return true; + return Some(true); } match ty { - Array(arr) => type_is_covariant(&*arr.elem, in_template), + Array(arr) => type_is_covariant_over_this_lifetime(&*arr.elem), BareFn(f) => { for arg in f.inputs.iter() { - if !type_is_covariant(&arg.ty, true) { - return false; + if uses_this_lifetime(arg.ty.to_token_stream()) { + return None; } } if let syn::ReturnType::Type(_, ty) = &f.output { - type_is_covariant(ty, true) - } else { - true + if uses_this_lifetime(ty.to_token_stream()) { + return None; + } } + Some(true) } - Group(ty) => type_is_covariant(&ty.elem, in_template), - ImplTrait(..) => false, // Unusable in struct definition. - Infer(..) => false, // Unusable in struct definition. - Macro(..) => false, // Assume false since we don't know. - Never(..) => false, - Paren(ty) => type_is_covariant(&ty.elem, in_template), + Group(ty) => type_is_covariant_over_this_lifetime(&ty.elem), + ImplTrait(..) => None, // Unusable in struct definition. + Infer(..) => None, // Unusable in struct definition. + Macro(..) => None, // Assume false since we don't know. + Never(..) => None, + Paren(ty) => type_is_covariant_over_this_lifetime(&ty.elem), Path(path) => { if let Some(qself) = &path.qself { - if !type_is_covariant(&qself.ty, in_template) { - return false; + if !type_is_covariant_over_this_lifetime(&qself.ty)? { + return Some(false); } } - let mut is_covariant = false; + let mut all_parameters_are_covariant = false; // If the type is Box, Arc, or Rc, we can assume it to be covariant. if apparent_std_container_type(ty).is_some() { - is_covariant = true; + all_parameters_are_covariant = true; } for segment in path.path.segments.iter() { let args = &segment.arguments; if let syn::PathArguments::AngleBracketed(args) = &args { for arg in args.args.iter() { if let syn::GenericArgument::Type(ty) = arg { - if !type_is_covariant(ty, !is_covariant) { - return false; + if all_parameters_are_covariant { + if !type_is_covariant_over_this_lifetime(ty)? { + return Some(false); + } + } else { + if uses_this_lifetime(ty.to_token_stream()) { + return None; + } } } else if let syn::GenericArgument::Lifetime(lt) = arg { - if lt.ident.to_string() == "this" && !is_covariant { - return false; + if lt.ident.to_string() == "this" && !all_parameters_are_covariant { + return None; } } } } else if let syn::PathArguments::Parenthesized(args) = &args { for arg in args.inputs.iter() { - if !type_is_covariant(arg, true) { - return false; + if uses_this_lifetime(arg.to_token_stream()) { + return None; } } if let syn::ReturnType::Type(_, ty) = &args.output { - if !type_is_covariant(ty, true) { - return false; + if uses_this_lifetime(ty.to_token_stream()) { + return None; } } } } - true + Some(true) } - Ptr(ptr) => type_is_covariant(&ptr.elem, in_template), + Ptr(ptr) => type_is_covariant_over_this_lifetime(&ptr.elem), // Ignore the actual lifetime of the reference because Rust can automatically convert those. Reference(rf) => { - !in_template && rf.mutability.is_none() && type_is_covariant(&rf.elem, in_template) + if rf.mutability.is_some() { + Some(!uses_this_lifetime(rf.elem.clone().into_token_stream())) + } else { + type_is_covariant_over_this_lifetime(&rf.elem) + } } - Slice(sl) => type_is_covariant(&sl.elem, in_template), - TraitObject(..) => false, + Slice(sl) => type_is_covariant_over_this_lifetime(&sl.elem), + TraitObject(..) => None, Tuple(tup) => { for ty in tup.elems.iter() { - if !type_is_covariant(ty, in_template) { - return false; + if !type_is_covariant_over_this_lifetime(ty)? { + return Some(false) } } - false + Some(true) } // As of writing this, syn parses all the types we could need. Verbatim(..) => unimplemented!(), diff --git a/ouroboros_macro/src/parse.rs b/ouroboros_macro/src/parse.rs index ed07c7c..546aa7c 100644 --- a/ouroboros_macro/src/parse.rs +++ b/ouroboros_macro/src/parse.rs @@ -3,7 +3,7 @@ use quote::format_ident; use syn::{spanned::Spanned, Attribute, Error, Fields, GenericParam, ItemStruct}; use crate::{ - covariance_detection::type_is_covariant, + covariance_detection::type_is_covariant_over_this_lifetime, info_structures::{BorrowRequest, Derive, FieldType, StructFieldInfo, StructInfo}, utils::submodule_contents_visiblity, }; @@ -141,8 +141,7 @@ pub fn parse_struct(def: &ItemStruct) -> Result { for field in &mut def_fields.named { let mut borrows = Vec::new(); let mut self_referencing = false; - let covariant = type_is_covariant(&field.ty, false); - let mut covariant = if covariant { Some(true) } else { None }; + let mut covariant = type_is_covariant_over_this_lifetime(&field.ty); let mut remove_attrs = Vec::new(); for (index, attr) in field.attrs.iter().enumerate() { let path = &attr.path; @@ -161,10 +160,16 @@ pub fn parse_struct(def: &ItemStruct) -> Result { remove_attrs.push(index); } if path.segments.first().unwrap().ident == "covariant" { + if covariant.is_some() { + panic!("TODO: Nice error, covariance specified twice."); + } covariant = Some(true); remove_attrs.push(index); } if path.segments.first().unwrap().ident == "not_covariant" { + if covariant.is_some() { + panic!("TODO: Nice error, covariance specified twice."); + } covariant = Some(false); remove_attrs.push(index); }