From 80bf6883be9af0bee3e764a3cdc0ac73c0cc239a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 6 Oct 2023 10:16:54 +1100 Subject: [PATCH 1/5] Remove an unnecessary `pub(crate)`. --- compiler/rustc_transmute/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_transmute/src/lib.rs b/compiler/rustc_transmute/src/lib.rs index 6c49e94dc31..e0109657c39 100644 --- a/compiler/rustc_transmute/src/lib.rs +++ b/compiler/rustc_transmute/src/lib.rs @@ -9,7 +9,7 @@ extern crate tracing; pub(crate) use rustc_data_structures::fx::{FxIndexMap as Map, FxIndexSet as Set}; pub mod layout; -pub(crate) mod maybe_transmutable; +mod maybe_transmutable; #[derive(Default)] pub struct Assume { From 409193654d6252a4cfa674588a77668ba653e384 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 6 Oct 2023 10:17:29 +1100 Subject: [PATCH 2/5] Make the comment order match variant declaration order. --- compiler/rustc_transmute/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_transmute/src/lib.rs b/compiler/rustc_transmute/src/lib.rs index e0109657c39..4b559632017 100644 --- a/compiler/rustc_transmute/src/lib.rs +++ b/compiler/rustc_transmute/src/lib.rs @@ -19,7 +19,7 @@ pub struct Assume { pub validity: bool, } -/// Either we have an error, transmutation is allowed, or we have an optional +/// Either transmutation is allowed, we have an error, or we have an optional /// Condition that must hold. #[derive(Debug, Hash, Eq, PartialEq, Clone)] pub enum Answer { From 449b84cb99203a1eaa735f49958b6ddabc8de779 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 6 Oct 2023 11:20:02 +1100 Subject: [PATCH 3/5] Remove `map_layouts`. As per the `FIXME` comment, it's an abstraction that makes the code harder to read. --- .../src/maybe_transmutable/mod.rs | 90 +++++++------------ 1 file changed, 33 insertions(+), 57 deletions(-) diff --git a/compiler/rustc_transmute/src/maybe_transmutable/mod.rs b/compiler/rustc_transmute/src/maybe_transmutable/mod.rs index c0141f1f841..ae193ef99e6 100644 --- a/compiler/rustc_transmute/src/maybe_transmutable/mod.rs +++ b/compiler/rustc_transmute/src/maybe_transmutable/mod.rs @@ -32,26 +32,6 @@ where ) -> Self { Self { src, dst, scope, assume, context } } - - // FIXME(bryangarza): Delete this when all usages are removed - pub(crate) fn map_layouts( - self, - f: F, - ) -> Result, Answer<::Ref>> - where - F: FnOnce( - L, - L, - ::Scope, - &C, - ) -> Result<(M, M), Answer<::Ref>>, - { - let Self { src, dst, scope, assume, context } = self; - - let (src, dst) = f(src, dst, scope, &context)?; - - Ok(MaybeTransmutableQuery { src, dst, scope, assume, context }) - } } // FIXME: Nix this cfg, so we can write unit tests independently of rustc @@ -107,42 +87,42 @@ where #[instrument(level = "debug", skip(self), fields(src = ?self.src, dst = ?self.dst))] pub(crate) fn answer(self) -> Answer<::Ref> { let assume_visibility = self.assume.safety; - // FIXME(bryangarza): Refactor this code to get rid of `map_layouts` - let query_or_answer = self.map_layouts(|src, dst, scope, context| { - // Remove all `Def` nodes from `src`, without checking their visibility. - let src = src.prune(&|def| true); - trace!(?src, "pruned src"); + let Self { src, dst, scope, assume, context } = self; - // Remove all `Def` nodes from `dst`, additionally... - let dst = if assume_visibility { - // ...if visibility is assumed, don't check their visibility. - dst.prune(&|def| true) - } else { - // ...otherwise, prune away all unreachable paths through the `Dst` layout. - dst.prune(&|def| context.is_accessible_from(def, scope)) - }; + // Remove all `Def` nodes from `src`, without checking their visibility. + let src = src.prune(&|def| true); - trace!(?dst, "pruned dst"); + trace!(?src, "pruned src"); - // Convert `src` from a tree-based representation to an NFA-based representation. - // If the conversion fails because `src` is uninhabited, conclude that the transmutation - // is acceptable, because instances of the `src` type do not exist. - let src = Nfa::from_tree(src).map_err(|Uninhabited| Answer::Yes)?; + // Remove all `Def` nodes from `dst`, additionally... + let dst = if assume_visibility { + // ...if visibility is assumed, don't check their visibility. + dst.prune(&|def| true) + } else { + // ...otherwise, prune away all unreachable paths through the `Dst` layout. + dst.prune(&|def| context.is_accessible_from(def, scope)) + }; - // Convert `dst` from a tree-based representation to an NFA-based representation. - // If the conversion fails because `src` is uninhabited, conclude that the transmutation - // is unacceptable, because instances of the `dst` type do not exist. - let dst = - Nfa::from_tree(dst).map_err(|Uninhabited| Answer::No(Reason::DstIsPrivate))?; + trace!(?dst, "pruned dst"); - Ok((src, dst)) - }); + // Convert `src` from a tree-based representation to an NFA-based representation. + // If the conversion fails because `src` is uninhabited, conclude that the transmutation + // is acceptable, because instances of the `src` type do not exist. + let src = match Nfa::from_tree(src) { + Ok(src) => src, + Err(Uninhabited) => return Answer::Yes, + }; - match query_or_answer { - Ok(query) => query.answer(), - Err(answer) => answer, - } + // Convert `dst` from a tree-based representation to an NFA-based representation. + // If the conversion fails because `src` is uninhabited, conclude that the transmutation + // is unacceptable, because instances of the `dst` type do not exist. + let dst = match Nfa::from_tree(dst) { + Ok(dst) => dst, + Err(Uninhabited) => return Answer::No(Reason::DstIsPrivate), + }; + + MaybeTransmutableQuery { src, dst, scope, assume, context }.answer() } } @@ -156,14 +136,10 @@ where #[inline(always)] #[instrument(level = "debug", skip(self), fields(src = ?self.src, dst = ?self.dst))] pub(crate) fn answer(self) -> Answer<::Ref> { - // FIXME(bryangarza): Refactor this code to get rid of `map_layouts` - let query_or_answer = self - .map_layouts(|src, dst, scope, context| Ok((Dfa::from_nfa(src), Dfa::from_nfa(dst)))); - - match query_or_answer { - Ok(query) => query.answer(), - Err(answer) => answer, - } + let Self { src, dst, scope, assume, context } = self; + let src = Dfa::from_nfa(src); + let dst = Dfa::from_nfa(dst); + MaybeTransmutableQuery { src, dst, scope, assume, context }.answer() } } From 73420fc13bb23151f1baa7199ffa3a38ab955de4 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 6 Oct 2023 11:21:33 +1100 Subject: [PATCH 4/5] Fix a comment. It was duplicated from the method above. --- compiler/rustc_transmute/src/maybe_transmutable/mod.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/rustc_transmute/src/maybe_transmutable/mod.rs b/compiler/rustc_transmute/src/maybe_transmutable/mod.rs index ae193ef99e6..6484a2968ff 100644 --- a/compiler/rustc_transmute/src/maybe_transmutable/mod.rs +++ b/compiler/rustc_transmute/src/maybe_transmutable/mod.rs @@ -147,9 +147,7 @@ impl MaybeTransmutableQuery::Ref>, C> where C: QueryContext, { - /// Answers whether a `Nfa` is transmutable into another `Nfa`. - /// - /// This method converts `src` and `dst` to DFAs, then computes an answer using those DFAs. + /// Answers whether a `Dfa` is transmutable into another `Dfa`. pub(crate) fn answer(self) -> Answer<::Ref> { MaybeTransmutableQuery { src: &self.src, From 29ed8e492ab99391e9eaa1247fbafe2759f9c383 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 6 Oct 2023 11:25:32 +1100 Subject: [PATCH 5/5] Remove the `MaybeTransmutableQuery<&'l Dfa<...>, C>` impl. Because there is also a `MaybeTransmutableQuery, C>` impl, and we don't need both. --- .../src/maybe_transmutable/mod.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/compiler/rustc_transmute/src/maybe_transmutable/mod.rs b/compiler/rustc_transmute/src/maybe_transmutable/mod.rs index 6484a2968ff..bf3c390c800 100644 --- a/compiler/rustc_transmute/src/maybe_transmutable/mod.rs +++ b/compiler/rustc_transmute/src/maybe_transmutable/mod.rs @@ -149,22 +149,6 @@ where { /// Answers whether a `Dfa` is transmutable into another `Dfa`. pub(crate) fn answer(self) -> Answer<::Ref> { - MaybeTransmutableQuery { - src: &self.src, - dst: &self.dst, - scope: self.scope, - assume: self.assume, - context: self.context, - } - .answer() - } -} - -impl<'l, C> MaybeTransmutableQuery<&'l Dfa<::Ref>, C> -where - C: QueryContext, -{ - pub(crate) fn answer(&mut self) -> Answer<::Ref> { self.answer_memo(&mut Map::default(), self.src.start, self.dst.start) }