[move-core] Add ident_str! macro to create const IdentStr's

[phlip9]

Overview

A PR for something that's been bothering me for a while; namely, storing &'static strs instead of &'static IdentStrs for MoveResource::MODULE_NAME and MoveResource::STRUCT_NAME, which means lots of Lazy<Identifier> and unwraps everywhere in diem-types.

This diff adds a new macro ident_str! which lets us construct const &'static IdentStrs that are asserted valid at compile-time.

Note for reviewers: most of the diff is just swapping Identifier::new("..") and IdentStr::new("..") for ident_str!. The interesting parts to check out are the macro in language/move-core/types/src/identifier.rs and the consts in MoveResource changed to &'static IdentStr.

There's also an unsafe line in here:

macro_rules ident_str {
    ($ident:literal) => {{
        // ..

        // SAFETY: the following transmute is safe because
        // (1) it's equivalent to the unsafe-reborrow inside IdentStr::ref_cast()
        //     (which we can't use b/c it's not const).
        // (2) we've just asserted that IdentStr impls RefCast<From = str>, which
        //     already guarantees the transmute is safe (RefCast checks that
        //     IdentStr(str) is #[repr(transparent)]).
        // (3) both in and out lifetimes are 'static, so we're not widening the lifetime.
        // (4) we've just asserted that the IdentStr passes the is_valid check.
        //
        // Note: this lint is unjustified and no longer checked. See issue:
        // https://github.com/rust-lang/rust-clippy/issues/6372
        #[allow(clippy::transmute_ptr_to_ptr)]
        unsafe {
            ::std::mem::transmute::<&'static str, &'static $crate::identifier::IdentStr>(s)
        }
    }}
}

which is safe because we derive RefCast for IdentStr, which guarantees this kind of transmute is safe if the trait is derivable. Finally, the transmute is effectively the same as the unsafe pointer cast inside RefCast::ref_cast(s), except we can actually use transmute in a const context (though not in a const fn for some reason, which is why this is implemented as a macro and not an IdentStr method).

[tnowacki]

woah...

[tnowacki]

I think this can be made const now?

[phlip9]

I don't think we can make ModuleId::new const b/c we can't make const Identifiers, as they need to allocate on the heap.

[tnowacki]

Ah right... Unfortunate.

[bmwill]

@tnowacki It seems like you didn't have any major concerns with this PR? If not I'm going to go ahead and approve this so that Philip can get this landed. Unfortunately it already looks like its bitrotted a lot and will require a rebase.

[tnowacki]

Sorry for the delay, but yes it seems fine to me! I think this does a good bit of relieving pain around the str version of Idents, especially for constants. Thanks!

[phlip9]

rebased

[phlip9]

/land

[github-actions]

Cluster Test Result

Test runner setup time spent 390 secs
Cluster setup failed: `Nodes did not become healthy after deployment`
Logs: http://kibana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-05-13T03:40:53Z',to:'2021-05-13T03:55:52Z'))
Dashboard: http://grafana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/d/performance/performance?from=1620877253000&to=1620878152000
Validator 1 logs: http://kibana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-05-13T03:40:53Z',to:'2021-05-13T03:55:52Z'))&_a=(columns:!(log),query:(language:kuery,query:'kubernetes.pod_name:"val-1"'),sort:!(!('@timestamp',desc)))

❗ Cluster Test failed - non-zero exit code for cti

Repro cmd:

./scripts/cti --tag land_4b237180 --cluster-test-tag land_d01fbab7 -E BATCH_SIZE=15 -E UPDATE_TO_TAG=land_d01fbab7 --report report.json --suite land_blocking_compat

[bors-libra]

💔 Test Failed - ci-test

[zihaoccc]

/land

[github-actions]

Cluster Test Result

Test runner setup time spent 264 secs
Compatibility test results for land_88bef8ec ==> land_9428e916 (PR)
1. All instances running land_88bef8ec, generating some traffic on network
2. First full node land_88bef8ec ==> land_9428e916, to validate new full node to old validator node traffic
3. First Validator node land_88bef8ec ==> land_9428e916, to validate storage compatibility
4. First batch validators (14) land_88bef8ec ==> land_9428e916, to test consensus and traffic between old full nodes and new validator node
5. First batch full nodes (14) land_88bef8ec ==> land_9428e916
6. Second batch validators (15) land_88bef8ec ==> land_9428e916, to upgrade rest of the validators
7. Second batch of full nodes (15) land_88bef8ec ==> land_9428e916, to finish the network upgrade, time spent 674 secs
all up : 857 TPS, 5299 ms latency, 6050 ms p99 latency, no expired txns, time spent 250 secs
Logs: http://kibana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-05-13T16:13:17Z',to:'2021-05-13T16:36:01Z'))
Dashboard: http://grafana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/d/performance/performance?from=1620922397000&to=1620923761000
Validator 1 logs: http://kibana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-05-13T16:13:17Z',to:'2021-05-13T16:36:01Z'))&_a=(columns:!(log),query:(language:kuery,query:'kubernetes.pod_name:"val-1"'),sort:!(!('@timestamp',desc)))

Repro cmd:

./scripts/cti --tag land_88bef8ec --cluster-test-tag land_9428e916 -E BATCH_SIZE=15 -E UPDATE_TO_TAG=land_9428e916 --report report.json --suite land_blocking_compat

🎉 Land-blocking cluster test passed! 👌