From 85909f159030c1fb94c01018999c2a088cf9e134 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Mon, 11 Oct 2021 12:58:48 -0400 Subject: [PATCH] tamer: sym::SymbolStr: Remove This removes `SymbolStr` in favor of, simply, `&'static str`. The abstraction provided no additional safety since the slice was trivially extracted (and commonly, in practice), and was inconvenient to work with. This is part of a process of relaxing lookups so that symbols can be conveniently displayed in errors; rather than trying to prevent the developer from doing something bad, we'll just rely on conventions, hope that it doesn't happen, and if it does, address it either at that time or when it shows up in the profiler. --- tamer/src/ir/asg/base.rs | 6 +-- tamer/src/ir/asg/object.rs | 24 ++++----- tamer/src/ir/legacyir.rs | 2 +- tamer/src/ld/poc.rs | 2 +- tamer/src/obj/xmlo/asg_builder.rs | 6 +-- tamer/src/obj/xmlo/reader/mod.rs | 8 ++- tamer/src/sym/interner.rs | 18 +++---- tamer/src/sym/mod.rs | 12 ++--- tamer/src/sym/symbol.rs | 83 ++----------------------------- 9 files changed, 40 insertions(+), 121 deletions(-) diff --git a/tamer/src/ir/asg/base.rs b/tamer/src/ir/asg/base.rs index 4cc99de4..10367712 100644 --- a/tamer/src/ir/asg/base.rs +++ b/tamer/src/ir/asg/base.rs @@ -392,8 +392,8 @@ mod test { UnresolvedError, }; use crate::ir::legacyir::SymDtype; + use crate::sym::GlobalSymbolIntern; use crate::sym::SymbolId; - use crate::sym::{GlobalSymbolIntern, SymbolStr}; use std::cell::RefCell; #[derive(Debug, Default, PartialEq)] @@ -622,7 +622,7 @@ mod test { let node = sut.declare(sym, IdentKind::Meta, src.clone())?; let obj = sut.get(node).unwrap(); let terr = TransitionError::ExternResolution { - name: SymbolStr::test_from_str("test fail"), + name: &"test fail", expected: IdentKind::Meta, given: IdentKind::Meta, }; @@ -687,7 +687,7 @@ mod test { // It doesn't matter that this isn't the error that'll actually be // returned, as long as it's some sort of TransitionError. let terr = TransitionError::ExternResolution { - name: SymbolStr::test_from_str("test fail"), + name: &"test fail", expected: IdentKind::Meta, given: IdentKind::Meta, }; diff --git a/tamer/src/ir/asg/object.rs b/tamer/src/ir/asg/object.rs index 4015b8a1..585d750b 100644 --- a/tamer/src/ir/asg/object.rs +++ b/tamer/src/ir/asg/object.rs @@ -24,7 +24,7 @@ use super::ident::IdentKind; use crate::ir::legacyir::SymAttrs; -use crate::sym::{GlobalSymbolResolve, SymbolId, SymbolStr}; +use crate::sym::{GlobalSymbolResolve, SymbolId}; use std::result::Result; pub type TransitionResult = Result; @@ -471,25 +471,25 @@ impl IdentObjectState for IdentObject { pub enum TransitionError { /// Attempted to redeclare a concrete, non-virtual identifier without an /// override. - Redeclare { name: SymbolStr<'static> }, + Redeclare { name: &'static str }, /// Extern resolution failure. /// /// An extern could not be resolved because the provided identifier had /// a type that is incompatible with the extern definition. ExternResolution { - name: SymbolStr<'static>, + name: &'static str, expected: IdentKind, given: IdentKind, }, /// Attempt to override a non-virtual identifier. - NonVirtualOverride { name: SymbolStr<'static> }, + NonVirtualOverride { name: &'static str }, /// Overriding a virtual identifier failed due to an incompatible /// [`IdentKind`]. VirtualOverrideKind { - name: SymbolStr<'static>, + name: &'static str, existing: IdentKind, given: IdentKind, }, @@ -553,17 +553,17 @@ impl std::error::Error for TransitionError { #[derive(Clone, Debug, PartialEq)] pub enum UnresolvedError { /// Expected identifier is missing and nothing about it is known. - Missing { name: SymbolStr<'static> }, + Missing { name: &'static str }, /// Expected identifier has not yet been resolved with a concrete /// definition. Extern { /// Identifier name. - name: SymbolStr<'static>, + name: &'static str, /// Expected identifier type. kind: IdentKind, /// Name of package where the extern was defined. - pkg_name: Option>, + pkg_name: Option<&'static str>, }, } @@ -583,7 +583,7 @@ impl std::fmt::Display for UnresolvedError { "unresolved extern `{}` of type `{}`, declared in `{}`", name, kind, - pkg_name.as_ref().map(|s| s.as_str()).unwrap_or(""), + pkg_name.as_ref().unwrap_or(&""), ), } } @@ -988,7 +988,7 @@ mod test { fn resolved_on_extern_error_fmt_without_pkg() { let meta = IdentKind::Meta; let err = UnresolvedError::Extern { - name: SymbolStr::test_from_str("foo"), + name: &"foo", kind: IdentKind::Meta, pkg_name: None, }; @@ -1003,10 +1003,10 @@ mod test { #[test] fn resolved_on_extern_error_fmt_with_pkg() { let meta = IdentKind::Meta; - let pkg = SymbolStr::test_from_str("pkg"); + let pkg = &"pkg"; let err = UnresolvedError::Extern { - name: SymbolStr::test_from_str("foo"), + name: &"foo", kind: IdentKind::Meta, pkg_name: Some(pkg.clone()), }; diff --git a/tamer/src/ir/legacyir.rs b/tamer/src/ir/legacyir.rs index 93399bd1..dafa8036 100644 --- a/tamer/src/ir/legacyir.rs +++ b/tamer/src/ir/legacyir.rs @@ -295,7 +295,7 @@ impl Into for SymDtype { impl AsRef for SymDtype { /// Produce `xmlo`-compatible representation. fn as_ref(&self) -> &str { - self.as_sym().lookup_str().as_str() + self.as_sym().lookup_str() } } diff --git a/tamer/src/ld/poc.rs b/tamer/src/ld/poc.rs index 31c81e9b..d6b5e3a4 100644 --- a/tamer/src/ld/poc.rs +++ b/tamer/src/ld/poc.rs @@ -155,7 +155,7 @@ pub fn graphml(package_path: &str, output: &str) -> Result<(), Box> { vec![ ("label".into(), name.into()), - ("kind".into(), kind.lookup_str().as_str().into()), + ("kind".into(), kind.lookup_str().into()), ("generated".into(), generated.into()), ] })); diff --git a/tamer/src/obj/xmlo/asg_builder.rs b/tamer/src/obj/xmlo/asg_builder.rs index 0f77ba2c..0cec7657 100644 --- a/tamer/src/obj/xmlo/asg_builder.rs +++ b/tamer/src/obj/xmlo/asg_builder.rs @@ -72,7 +72,7 @@ use crate::ir::asg::{ Asg, AsgError, IdentKind, IdentKindError, IdentObjectState, IndexType, ObjectRef, Source, }; -use crate::sym::{GlobalSymbolResolve, SymbolId, SymbolStr}; +use crate::sym::{GlobalSymbolResolve, SymbolId}; use std::collections::HashSet; use std::convert::TryInto; use std::error::Error; @@ -292,13 +292,13 @@ pub enum AsgBuilderError { AsgError(AsgError), /// Fragment encountered for an unknown identifier. - MissingFragmentIdent(SymbolStr<'static>), + MissingFragmentIdent(&'static str), /// Eligibility classification references unknown identifier. /// /// This is generated by the compiler and so should never happen. /// (That's not to say that it won't, but it shouldn't.) - BadEligRef(SymbolStr<'static>), + BadEligRef(&'static str), } impl Display for AsgBuilderError { diff --git a/tamer/src/obj/xmlo/reader/mod.rs b/tamer/src/obj/xmlo/reader/mod.rs index 208421f5..4aaa8b59 100644 --- a/tamer/src/obj/xmlo/reader/mod.rs +++ b/tamer/src/obj/xmlo/reader/mod.rs @@ -91,7 +91,7 @@ //! XmloEvent::SymDecl(sym, attrs) => syms.push((sym, attrs.ty)), //! XmloEvent::SymDeps(sym, symdeps) => deps.push((sym, symdeps)), //! XmloEvent::Fragment(sym, text) => -//! fragments.push((sym, text.lookup_str().as_str())), +//! fragments.push((sym, text.lookup_str())), //! //! // Do not read past end of header. //! XmloEvent::Eoh => break, @@ -137,9 +137,7 @@ //! ``` use crate::ir::legacyir::{PackageAttrs, SymAttrs, SymType}; -use crate::sym::{ - GlobalSymbolInternUnchecked, GlobalSymbolResolve, SymbolId, SymbolStr, -}; +use crate::sym::{GlobalSymbolInternUnchecked, GlobalSymbolResolve, SymbolId}; #[cfg(test)] use crate::test::quick_xml::MockBytesStart as BytesStart; #[cfg(test)] @@ -801,7 +799,7 @@ pub enum XmloError { /// A `preproc:fragment` element was found, but is missing `@id`. UnassociatedFragment, /// A `preproc:fragment` element was found, but is missing `text()`. - MissingFragmentText(SymbolStr<'static>), + MissingFragmentText(&'static str), } impl From for XmloError { diff --git a/tamer/src/sym/interner.rs b/tamer/src/sym/interner.rs index 9c9974dd..3b3d4e89 100644 --- a/tamer/src/sym/interner.rs +++ b/tamer/src/sym/interner.rs @@ -67,7 +67,6 @@ //! assert_eq!("foo", interner.index_lookup(ia).unwrap()); //! ``` -use super::symbol::SymbolStr; use super::{SymbolId, SymbolIndexSize}; use crate::global; use bumpalo::Bump; @@ -85,7 +84,7 @@ use std::hash::BuildHasher; /// allowing symbols to be compared for equality cheaply by comparing /// integers. /// Symbol locations in memory are fixed for the lifetime of the interner, -/// and can be retrieved as [`SymbolStr`] using +/// and can be retrieved as [`str`] using /// [`index_lookup`](Interner::index_lookup). /// /// If you care whether a value has been interned yet or not, @@ -141,11 +140,15 @@ pub trait Interner<'i, Ix: SymbolIndexSize> { /// Look up a symbol's string value by its [`SymbolId`]. /// - /// This will always return a [`SymbolStr`] as long as the provided + /// This will always return a [`str`] as long as the provided /// `index` represents a symbol interned with this interner. /// If the index is not found, /// the result is [`None`]. - fn index_lookup(&'i self, index: SymbolId) -> Option>; + /// + /// [`str`] requires significantly more storage than an appropriate + /// [`SymbolId`] and should only be used when a string value must be + /// written (e.g. to a file or displayed to the user). + fn index_lookup(&'i self, index: SymbolId) -> Option<&'i str>; /// Intern an assumed-UTF-8 slice of bytes or return an existing /// [`SymbolId`]. @@ -339,11 +342,8 @@ where self.map.borrow().len() } - fn index_lookup(&'i self, index: SymbolId) -> Option> { - self.strings - .borrow() - .get(index.as_usize()) - .map(|str| SymbolStr::from_interned_slice(*str)) + fn index_lookup(&'i self, index: SymbolId) -> Option<&'i str> { + self.strings.borrow().get(index.as_usize()).map(|str| *str) } } diff --git a/tamer/src/sym/mod.rs b/tamer/src/sym/mod.rs index 1e4b481e..9cffb9cc 100644 --- a/tamer/src/sym/mod.rs +++ b/tamer/src/sym/mod.rs @@ -62,7 +62,7 @@ //! assert_ne!(foo, "bar".intern()); //! //! // Interned slices can be looked up by their symbol id. -//! assert_eq!("foo", foo.lookup_str().as_str()); +//! assert_eq!("foo", foo.lookup_str()); //! ``` //! //! What Is String Interning? @@ -131,8 +131,6 @@ //! The string associated with a [`SymbolId`] can be looked up from the pool //! using [`GlobalSymbolResolve::lookup_str`] for global interners, //! or [`Interner::index_lookup`] otherwise. -//! Interned strings are represented by [`SymbolStr`], -//! which can be dereferenced into [`&str`]. //! Symbols allocated using a global interner will have a `'static` //! lifetime. //! @@ -177,7 +175,7 @@ //! a part of Rust itself, //! and treated no differently than other core memory allocation. //! -//! All [`SymbolStr`] objects returned from global interners hold a +//! All [`str`] objects returned from global interners hold a //! `'static` lifetime to simplify lifetime management and borrowing. //! However, //! these should not be used in place of [`SymbolId`] if the string value @@ -190,7 +188,7 @@ //! - [`GlobalSymbolResolve`] provides a `lookup_str` method on //! [`SymbolId`] which resolves the symbol using the appropriate //! global interner, -//! producing a [`SymbolStr`] holding a reference to the `'static` +//! producing a [`str`] holding a reference to the `'static` //! string slice within the pool. //! //! These traits are intentionally separate so that it is clear how a @@ -200,7 +198,7 @@ //! //! TAMER does not currently utilize threads, //! and global interners are never dropped, -//! and so [`SymbolStr`] will always refer to a valid string. +//! and so [`str`] will always refer to a valid string. //! //! There is no mechanism preventing [`SymbolId`] from one interner from //! being used with another beyond [`SymbolIndexSize`] bounds; @@ -357,5 +355,5 @@ pub use interner::{ }; pub use symbol::{ GlobalSymbolIntern, GlobalSymbolInternUnchecked, GlobalSymbolResolve, - SymbolId, SymbolIndexSize, SymbolStr, + SymbolId, SymbolIndexSize, }; diff --git a/tamer/src/sym/symbol.rs b/tamer/src/sym/symbol.rs index b269f5ca..8feecd57 100644 --- a/tamer/src/sym/symbol.rs +++ b/tamer/src/sym/symbol.rs @@ -24,7 +24,7 @@ use super::{DefaultInterner, Interner}; use crate::global; use std::convert::{TryFrom, TryInto}; -use std::fmt::{self, Debug, Display}; +use std::fmt::Debug; use std::hash::Hash; use std::num::{NonZeroU16, NonZeroU32}; use std::ops::Deref; @@ -44,9 +44,6 @@ use std::thread::LocalKey; /// /// Symbol Strings /// ============== -/// [`SymbolId`] intentionally omits the [`Display`] trait to ensure that -/// compile-time errors occur when symbols are used in contexts where -/// strings are expected. /// To resolve a [`SymbolId`] into the string that it represents, /// see either [`GlobalSymbolResolve::lookup_str`] or /// [`Interner::index_lookup`]. @@ -179,80 +176,6 @@ thread_local! { supported_symbol_index!(u16, NonZeroU16, Static16Interner, INTERNER_16); supported_symbol_index!(u32, NonZeroU32, Static32Interner, INTERNER_32); -/// A string retrieved from the intern pool using a [`SymbolId`]. -/// -/// The lifetime of the inner string is constrained to the lifetime of the -/// interner itself. -/// For global interners, -/// this means that the string slice has a `'static` lifetime. -/// -/// [`SymbolStr`] requires significantly more storage than an appropriate -/// [`SymbolId`] and should only be used when a string value must be -/// written (e.g. to a file or displayed to the user). -/// -/// This value is intended to be short-lived. -#[derive(Debug, Default, Clone)] -pub struct SymbolStr<'i>(&'i str); - -impl<'i> SymbolStr<'i> { - pub fn as_str(&self) -> &'i str { - self.0 - } - - /// Create a [`SymbolStr`] from a string for testing. - /// - /// _This function is only available for tests for convenience!_ - /// `SymbolStr` must always represent a real, interned string in - /// non-test code. - #[cfg(test)] - pub fn test_from_str(s: &'i str) -> Self { - SymbolStr(s) - } -} - -impl<'i> SymbolStr<'i> { - pub(super) fn from_interned_slice(slice: &'i str) -> SymbolStr<'i> { - SymbolStr(slice) - } -} - -impl<'i, T: Deref> PartialEq for SymbolStr<'i> { - fn eq(&self, other: &T) -> bool { - self.0 == other.deref() - } -} - -impl PartialEq> for &str { - fn eq(&self, other: &SymbolStr<'_>) -> bool { - *self == other.0 - } -} - -impl<'i> Display for SymbolStr<'i> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -// Once we have unsafe_impls stabalized, -// we should prevent `SymbolStr` from crossing threads. -// TAMER does not use threads at the time of writing, -// so this isn't a practical concern. -// If we _do_ want to pass between threads, -// we need to ensure the thread holding the interner lives longer than all -// other threads. -//impl<'i> !Send for SymbolStr<'i> {} -//impl<'i> !Sync for SymbolStr<'i> {} - -impl<'i> Deref for SymbolStr<'i> { - type Target = str; - - #[inline] - fn deref(&self) -> &'i str { - self.as_str() - } -} - /// Acquire a static reference to a global interner. /// /// Global interners are static and thread-local. @@ -301,11 +224,11 @@ pub trait GlobalSymbolResolve { /// lookup is performed on the global interner pool, /// which requires locking and so comes at a (small) cost. /// This shouldn't be done more than is necessary. - fn lookup_str(&self) -> SymbolStr<'static>; + fn lookup_str(&self) -> &'static str; } impl GlobalSymbolResolve for SymbolId { - fn lookup_str(&self) -> SymbolStr<'static> { + fn lookup_str(&self) -> &'static str { Ix::with_static_interner(|interner| { interner.index_lookup(*self).unwrap() })