Improved comments

Author: zslayton

src/lazy/binary/raw/v1_1/value.rs

@@ -290,7 +290,10 @@ impl<'top> LazyRawBinaryValue_1_1<'top> {
             header: Header {
                 // It is an int, that's true.
                 ion_type: IonType::Int,
-                // Nonsense values for now
+                // Eventually we'll refactor `EncodedValue` to accommodate values that don't have
+                // a header (i.e., parameters with tagless encodings). See:
+                // https://github.com/amazon-ion/ion-rust/issues/805
+                // For now, we'll populate these fields with nonsense values and ignore them.
                 ion_type_code: OpcodeType::Nop,
                 low_nibble: 0,
             },

src/lazy/expanded/compiler.rs

@@ -48,10 +48,20 @@ impl ExpansionAnalysis {
     }
 }
 
-/// When static analysis can detect that a template body will always expand to a single value,
-/// information inferred about that value is stored in this type. When this template backs a
-/// lazy value, having these fields available allows the lazy value to answer basic queries without
-/// needing to fully evaluate the template.
+/// When the [`TemplateCompiler`] is able to determine that a macro's template will always produce
+/// exactly one value, that macro is considered a "singleton macro." Singleton macros offer
+/// a few benefits:
+///
+/// * Because evaluation will produce be exactly one value, the reader can hand out a LazyValue
+///   holding the e-expression as its backing data. Other macros cannot do this because if you're
+///   holding a LazyValue and the macro later evaluates to 0 values or 100 values, there's not a way
+///   for the application to handle those outcomes.
+/// * Expanding a singleton macro doesn't require an evaluator with a stack because as soon as
+///   you've gotten a value, you're done--no need to `pop()` and preserve state.
+///
+/// Information inferred about a singleton macro's output value is stored in an `ExpansionSingleton`.
+/// When a singleton macro backs a lazy value, having these fields available allows the lazy value to
+/// answer basic queries without needing to fully evaluate the template.
 #[derive(Copy, Clone, Debug, PartialEq)]
 pub struct ExpansionSingleton {
     pub(crate) is_null: bool,

src/lazy/expanded/macro_evaluator.rs

@@ -16,7 +16,6 @@ use std::fmt::{Debug, Formatter};
 use std::ops::Range;
 
 use bumpalo::collections::{String as BumpString, Vec as BumpVec};
-use ice_code::ice;
 
 use crate::lazy::decoder::{Decoder, HasSpan, LazyRawValueExpr};
 use crate::lazy::expanded::e_expression::{
@@ -408,16 +407,16 @@ impl<'top, D: Decoder> MacroExpansion<'top, D> {
     }
 
     /// Expands the current macro with the expectation that it will produce exactly one value.
+    /// For more information about singleton macros, see
+    /// [`ExpansionSingleton`](crate::lazy::expanded::compiler::ExpansionSingleton).
     #[inline(always)]
     pub(crate) fn expand_singleton(mut self) -> IonResult<LazyExpandedValue<'top, D>> {
         // We don't need to construct an evaluator because this is guaranteed to produce exactly
         // one value.
         match self.next_step()? {
-            // If the expansion produces anything other than a final value, there's a bug.
             MacroExpansionStep::FinalStep(Some(ValueExpr::ValueLiteral(value))) => Ok(value),
-            _ => ice!(IonResult::decoding_error(format!(
-                "expansion of {self:?} was required to produce exactly one value",
-            ))),
+            // If the expansion produces anything other than a final value, there's a bug.
+            _ => unreachable!("expansion of {self:?} was required to produce exactly one value"),
         }
     }
 

src/lazy/expanded/macro_table.rs

@@ -179,7 +179,7 @@ impl MacroTable {
     pub const NUM_SYSTEM_MACROS: usize = Self::SYSTEM_MACRO_KINDS.len();
     // When a user defines new macros, this is the first ID that will be assigned. This value
     // is expected to change as development continues. It is currently used in several unit tests.
-    pub const FIRST_USER_MACRO_ID: usize = 4;
+    pub const FIRST_USER_MACRO_ID: usize = Self::NUM_SYSTEM_MACROS;
 
     pub fn new() -> Self {
         let macros_by_id = vec![

src/lazy/system_reader.rs

@@ -1086,7 +1086,7 @@ mod tests {
             &[
                 Symbol::from("foo"),
                 Symbol::from("bar"),
-                Symbol::from("baz")
+                Symbol::from("baz"),
             ]
         );
 
@@ -1096,11 +1096,11 @@ mod tests {
         // This directive defines two more.
         assert_eq!(new_macro_table.len(), 2 + MacroTable::NUM_SYSTEM_MACROS);
         assert_eq!(
-            new_macro_table.macro_with_id(4),
+            new_macro_table.macro_with_id(MacroTable::FIRST_USER_MACRO_ID),
             new_macro_table.macro_with_name("seventeen")
         );
         assert_eq!(
-            new_macro_table.macro_with_id(5),
+            new_macro_table.macro_with_id(MacroTable::FIRST_USER_MACRO_ID + 1),
             new_macro_table.macro_with_name("twelve")
         );