Mock-builder: add support for custom lifetimes #1335

lemunozm · 2023-05-03T01:33:38Z

Description

This is the last technical requirement for mock-builder. After this PR, I think we can test any possible trait. 🎉 Future work is to create procedural macros to reduce boilerplate. Roadmap here

New features:

References supported.
Generics with random lifetimes (other than 'static) supported.

Fixes #1322

Changes and Descriptions

New storage implementation to support input/output types that could not be 'static

wischli

I have to admit I can only follow on a much much higher level and on that, it looks good to me! I'd be interested in an argument for your safe unsafe transmutation, see below.

wischli · 2023-05-08T16:24:34Z

libs/mock-builder/src/storage.rs

+	let f = Box::new(f) as Box<dyn Fn(I) -> O>;
+	let ptr = Box::into_raw(f);
+
+	// SAFETY: transforming a wide pointer to an u128 is always safe.


Generally, I don't think this is true. Can you show a short proof?

It's safe in the same way a "normal" reference is always transformable into a * const u64. In fact, the following line does not require unsafe rust:

let reference: &u64 = ... let ptr = reference as *const u64

This is safe because, it is just an address, a number. What it's not safe is what you do with that address, because all invariants and protections are broken after this.

This is why the other unsafe is the critical one, and you must ensure that the ptr still exists and corresponds to the same type as the original.

Regarding "why exactly u128?" It's because any &dyn Type is a wide pointer: one part corresponds to the pointer itself and the other to the metadata pointer that contains the trait information of the closure. We do not care about how both pointers are sorted in memory, because we are undoing the same we did in the other method for the same type.

It might be good to add more explanation in the comments--like how this enables us to use a fixed size in the CallInfo struct for the ptr as opposed to having to know the size/alignment of the vtable ptr in the fat pointer.
Would it be reasonable to have CallInfo have 2 usize fields for the closure address and vtable address instead and use ptr::from_raw_parts to build ptr instead?

Agree 👍🏻 I'll add a better explanation. Regarding the 2 usize fields, I think separating could confuse in the sense that seems that they should be used separately when not. We only need to allocate that, but not use the internals. Again, I think a good explanation into the code is needed.

wischli · 2023-05-08T16:25:17Z

libs/mock-builder/src/storage.rs

+			));
+		}
+
+		// SAFETY: The existance of this boxed clousure in consequent calls is ensured


nit

Suggested change

// SAFETY: The existance of this boxed clousure in consequent calls is ensured

// SAFETY: The existence of this boxed closure in consequent calls is ensured

This is some real inception level stuff: The existence is ensured by the forget call 🤯

Realize this means that you have done a very good review. Thanks so much! 🙌🏻

wischli

I have to admit I can only follow on a much much higher level and on that, it looks good to me! I'd be interested in an argument for your safe unsafe transmutation, see below.

thea-leake

Looks good to me! I really like the better error formatting, known size for stored mock fns, and reduced transmute usage!
Added a few comments, but nothing blocking.

thea-leake · 2023-05-11T17:14:35Z

libs/mock-builder/src/storage.rs

+	let f = Box::new(f) as Box<dyn Fn(I) -> O>;
+	let ptr = Box::into_raw(f);
+
+	// SAFETY: transforming a wide pointer to an u128 is always safe.


It might be good to add more explanation in the comments--like how this enables us to use a fixed size in the CallInfo struct for the ptr as opposed to having to know the size/alignment of the vtable ptr in the fat pointer.
Would it be reasonable to have CallInfo have 2 usize fields for the closure address and vtable address instead and use ptr::from_raw_parts to build ptr instead?

thea-leake · 2023-05-11T17:16:33Z

libs/mock-builder/src/storage.rs

+			));
+		}
+
+		// SAFETY: The existence of this boxed closure in consequent calls is ensured


It might be good to update comment to indicate how these are being persisted--that forget takes ownership, but doesn't call destructor. I was initially unclear as to what was actually going on here.

Ok! I'll do it. Thanks for the advice and for taking the time to deep into it, it's not an easy PR.

thea-leake · 2023-05-11T17:19:19Z

libs/mock-builder/src/storage.rs

-				Error::TypeNotMatch => "The function is registered but the type mismatches",
-			}
-		)
+		match self {


thea-leake · 2023-05-11T17:20:19Z

libs/mock-builder/src/storage.rs


 /// Identify a call in the call storage
 pub type CallId = u64;

-trait Callable {
-	fn as_any(&self) -> &dyn Any;
+struct CallInfo {


Very cool that we can have this as known fixed size, though have comment below in casting thread about the transmute.

lemunozm · 2023-05-12T07:57:42Z

@thea-leake, I've documented every weird case I was able to see. In the process, I've simplified a bit the forget() call part (which is no longer needed).

thea-leake

LGTM!

lemunozm added D2-notify Pull request can be merged and notification about changes should be documented. crcl-protocol Circle protocol related. labels May 3, 2023

lemunozm requested review from NunoAlexandre, wischli and mustermeiszer May 3, 2023 01:33

lemunozm self-assigned this May 3, 2023

lemunozm force-pushed the mock-builder/custom-lifetimes branch from c2d8023 to 27039bc Compare May 4, 2023 09:50

lemunozm added 8 commits May 4, 2023 16:29

new generic storage

0276109

storage is now safe to use it

a5d5867

added tests for no static lifetimes

4ae02aa

minor renames

4b61914

TypeSignature to util file

fcea24c

remove unused unsafe from mocks

184a812

clippy fix

4c1090c

fix comment line size

37190f7

lemunozm force-pushed the mock-builder/custom-lifetimes branch from 26f9f80 to 37190f7 Compare May 4, 2023 14:29

wischli approved these changes May 8, 2023

View reviewed changes

wischli previously approved these changes May 8, 2023

View reviewed changes

fix nits

2afc1ef

lemunozm dismissed wischli’s stale review via 2afc1ef May 9, 2023 07:04

lemunozm requested a review from wischli May 9, 2023 07:05

thea-leake previously approved these changes May 11, 2023

View reviewed changes

overdocumented and minor simplification

ac76fe6

lemunozm dismissed thea-leake’s stale review via ac76fe6 May 12, 2023 07:55

thea-leake approved these changes May 12, 2023

View reviewed changes

lemunozm enabled auto-merge (squash) May 12, 2023 08:47

lemunozm merged commit 0613791 into main May 12, 2023
11 checks passed

lemunozm deleted the mock-builder/custom-lifetimes branch May 12, 2023 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mock-builder: add support for custom lifetimes #1335

Mock-builder: add support for custom lifetimes #1335

lemunozm commented May 3, 2023 •

edited

wischli left a comment

wischli May 8, 2023

lemunozm May 9, 2023

lemunozm May 9, 2023

thea-leake May 11, 2023

lemunozm May 11, 2023

wischli May 8, 2023

wischli May 8, 2023

lemunozm May 9, 2023

wischli left a comment

thea-leake left a comment •

edited

thea-leake May 11, 2023

thea-leake May 11, 2023 •

edited

lemunozm May 11, 2023

thea-leake May 11, 2023

thea-leake May 11, 2023

lemunozm commented May 12, 2023

thea-leake left a comment

	// SAFETY: The existance of this boxed clousure in consequent calls is ensured
	// SAFETY: The existence of this boxed closure in consequent calls is ensured

Mock-builder: add support for custom lifetimes #1335

Mock-builder: add support for custom lifetimes #1335

Conversation

lemunozm commented May 3, 2023 • edited

Description

Changes and Descriptions

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

thea-leake left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thea-leake May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented May 12, 2023

thea-leake left a comment

Choose a reason for hiding this comment

lemunozm commented May 3, 2023 •

edited

thea-leake left a comment •

edited

thea-leake May 11, 2023 •

edited