use Box::leak instead of forget

[pchampin]

attempt to address #5

[pchampin]

@mkatychev, could you check whether this solves the problem described in #5?

[mkatychev]

@pchampin this does indeed solve the problem. ideally I would like a unit tests that would cover this scenario to prevent regression.

The test_hash unit tests managed to reproduce the scenario best. On latest master, inserting mown1 and using println! on it seems to do the UB.

A CI miri test would also help avoid this in the future.

Addressing some of the clippy problems (such as the MownStr::from_str method name) for a new (non-hotfix) release would also be helpful.

Let me know if you'd like some help with the unit test/CI miri work.

[pchampin]

Let me know if you'd like some help with the unit test/CI miri work.

Yes, that would be very much appreciated. 👍

[mkatychev]

would it be better to have CI be noisy here? This may help hide underlying issues.
EDIT: I was not able to actually run the CI jobs myself, going off of this, I would suggest this be added to miri test:

mownstr/.github/workflows/lint_and_test.yml

Line 32 in 5c4c239

- uses: dtolnay/rust-toolchain@nightly

with:
  components: miri

[pchampin]

would it be better to have CI be noisy here? This may help hide underlying issues.

I marked this test as ignored because it accesses the filesystem, which miri can't do (sandboxing issue). This is not related to the code of mownstr but to the code of the test itself, so I don't think it is a significant issue to report.

I would suggest this be added to miri test:

Yes, I added the "with" directive in a further commit: 05c3350

[mkatychev]

Could this instead be done on the locations where m0 and m1 are defined & invoked so that there's still miri coverage? That may be overzealous but this would give us miri coverage of

mownstr/src/lib.rs

Lines 491 to 498 in 815c793

	let s = unsafe { String::from_utf8_unchecked(vec![b'a' + i; CAP]) };
	v.push(MownStr::from(s));
	println!(
	"{} MownStr(s) in the Vec, of len {}, starting with {:?}",
	v.len(),
	v[v.len() - 1].len(),
	&v[v.len() - 1][..2]
	);

[pchampin]

fair enough, I will proceed otherwise

[pchampin]

solved by d27f233

[mkatychev]

Thank you!

[pchampin]

I merged this branch locally, but for some reason github does not see it as merged, and complains about conflicts with the 'main' branch. This is probably due to the fact that I merged 'main' in this PR at some point...

Closing it, but note that it was effectively merged.