Report read-only variable name in error message

The refactoring in commit 8e2bcac (#314) made the error message
for assigning to a read-only variable less informative by removing the
variable name. This commit restores the variable name in the error
message.

To this end, this commit introduces a new error type
`AssignReadOnlyError` that is used to report the error in the
expansion phase. The error type is converted from `AssignError` to
convey the variable name, which is not included in `AssignError`.

yash-semantics/src/assign.rs

@@ -17,6 +17,7 @@
 //! Assignment.
 
 use crate::expansion::expand_value;
+use crate::expansion::AssignReadOnlyError;
 use crate::xtrace::XTrace;
 use std::fmt::Write;
 use yash_env::semantics::ExitStatus;
@@ -62,8 +63,12 @@ pub async fn perform_assignment(
     variable
         .assign(value, assign.location.clone())
         .map_err(|e| Error {
-            cause: ErrorCause::AssignError(e),
-            location: assign.location.clone(),
+            cause: ErrorCause::AssignReadOnly(AssignReadOnlyError {
+                name: assign.name.clone(),
+                new_value: e.new_value,
+                read_only_location: e.read_only_location,
+            }),
+            location: e.assigned_location.unwrap(),
         })?;
     if export {
         variable.export(true);
@@ -170,14 +175,12 @@ mod tests {
             .now_or_never()
             .unwrap()
             .unwrap_err();
-        assert_matches!(e.cause, ErrorCause::AssignError(error) => {
+        assert_matches!(e.cause, ErrorCause::AssignReadOnly(error) => {
+            assert_eq!(error.name, "v");
             assert_eq!(error.new_value, Value::scalar("new"));
-            assert_eq!(error.assigned_location, Some(Location::dummy("v=new")));
             assert_eq!(error.read_only_location, location);
         });
-        assert_eq!(*e.location.code.value.borrow(), "v=new");
-        assert_eq!(e.location.code.start_line_number.get(), 1);
-        assert_eq!(e.location.range, 0..5);
+        assert_eq!(e.location, Location::dummy("v=new"));
     }
 
     #[test]

yash-semantics/src/command/compound_command/for_loop.rs

@@ -21,6 +21,7 @@ use crate::assign::ErrorCause;
 use crate::command::Command;
 use crate::expansion::expand_word;
 use crate::expansion::expand_words;
+use crate::expansion::AssignReadOnlyError;
 use crate::xtrace::print;
 use crate::xtrace::trace_fields;
 use crate::xtrace::XTrace;
@@ -84,7 +85,11 @@ pub async fn execute(
                 other => other?,
             },
             Err(error) => {
-                let cause = ErrorCause::AssignError(error);
+                let cause = ErrorCause::AssignReadOnly(AssignReadOnlyError {
+                    name: name.value,
+                    new_value: error.new_value,
+                    read_only_location: error.read_only_location,
+                });
                 let location = name.origin;
                 let error = Error { cause, location };
                 return apply_errexit(error.handle(env).await, env);

yash-semantics/src/expansion.rs

@@ -90,7 +90,7 @@ use std::borrow::Cow;
 use thiserror::Error;
 use yash_env::semantics::ExitStatus;
 use yash_env::system::Errno;
-use yash_env::variable::AssignError;
+use yash_env::variable::Value;
 use yash_env::variable::Variable;
 use yash_syntax::source::pretty::Annotation;
 use yash_syntax::source::pretty::AnnotationType;
@@ -102,6 +102,18 @@ use yash_syntax::syntax::Word;
 #[doc(no_inline)]
 pub use yash_env::semantics::Field;
 
+/// Error returned on assigning to a read-only variable
+#[derive(Clone, Debug, Eq, Error, PartialEq)]
+#[error("cannot assign to read-only variable {name:?}")]
+pub struct AssignReadOnlyError {
+    /// Name of the read-only variable
+    pub name: String,
+    /// Value that was being assigned.
+    pub new_value: Value,
+    /// Location where the variable was made read-only.
+    pub read_only_location: Location,
+}
+
 /// Types of errors that may occur in the word expansion.
 #[derive(Clone, Debug, Eq, Error, PartialEq)]
 pub enum ErrorCause {
@@ -113,10 +125,9 @@ pub enum ErrorCause {
     #[error(transparent)]
     ArithError(#[from] ArithError),
 
-    // TODO AssignError should convey the variable name
     /// Assignment to a read-only variable.
     #[error(transparent)]
-    AssignError(#[from] AssignError),
+    AssignReadOnly(#[from] AssignReadOnlyError),
 
     /// Expansion of an unset parameter with the `nounset` option
     #[error("unset parameter")]
@@ -140,7 +151,7 @@ impl ErrorCause {
         match self {
             CommandSubstError(_) => "error performing the command substitution",
             ArithError(_) => "error evaluating the arithmetic expansion",
-            AssignError(_) => "error assigning to variable",
+            AssignReadOnly(_) => "error assigning to variable",
             UnsetParameter => "unset parameter",
             EmptyExpansion(error) => error.message_or_default(),
             NonassignableParameter(_) => "cannot assign to parameter",
@@ -155,7 +166,7 @@ impl ErrorCause {
         match self {
             CommandSubstError(e) => e.desc().into(),
             ArithError(e) => e.to_string().into(),
-            AssignError(e) => e.to_string().into(),
+            AssignReadOnly(e) => e.to_string().into(),
             UnsetParameter => "unset parameter disallowed by the nounset option".into(),
             EmptyExpansion(e) => e.state.description().into(),
             NonassignableParameter(e) => e.to_string().into(),
@@ -171,7 +182,7 @@ impl ErrorCause {
         match self {
             CommandSubstError(_) => None,
             ArithError(e) => e.related_location(),
-            AssignError(e) => Some((
+            AssignReadOnly(e) => Some((
                 &e.read_only_location,
                 "the variable was made read-only here",
             )),
@@ -357,28 +368,21 @@ mod tests {
     use crate::tests::return_builtin;
     use assert_matches::assert_matches;
     use futures_util::FutureExt;
-    use std::num::NonZeroU64;
-    use std::rc::Rc;
     use yash_env::variable::Scope;
     use yash_syntax::source::pretty::Message;
-    use yash_syntax::source::Code;
-    use yash_syntax::source::Source;
 
     #[test]
     fn from_error_for_message() {
-        let code = Rc::new(Code {
-            value: "hello".to_string().into(),
-            start_line_number: NonZeroU64::new(1).unwrap(),
-            source: Source::Unknown,
-        });
-        let location = Location { code, range: 2..4 };
         let error = Error {
-            cause: ErrorCause::AssignError(AssignError {
+            cause: ErrorCause::AssignReadOnly(AssignReadOnlyError {
+                name: "foo".into(),
                 new_value: "value".into(),
-                assigned_location: Some(Location::dummy("assigned")),
                 read_only_location: Location::dummy("ROL"),
             }),
-            location,
+            location: Location {
+                range: 2..4,
+                ..Location::dummy("hello")
+            },
         };
         let message = Message::from(&error);
         assert_eq!(message.r#type, AnnotationType::Error);
@@ -387,7 +391,7 @@ mod tests {
         assert_eq!(message.annotations[0].r#type, AnnotationType::Error);
         assert_eq!(
             message.annotations[0].label,
-            "cannot assign to read-only variable"
+            "cannot assign to read-only variable \"foo\""
         );
         assert_eq!(message.annotations[0].location, &error.location);
         assert_eq!(message.annotations[1].r#type, AnnotationType::Info);

yash-semantics/src/expansion/initial/arith.rs

@@ -23,12 +23,12 @@ use super::super::ErrorCause;
 use super::Env;
 use super::Error;
 use crate::expansion::expand_text;
+use crate::expansion::AssignReadOnlyError;
 use std::ops::Range;
 use std::rc::Rc;
 use yash_arith::eval;
 use yash_env::option::Option::Unset;
 use yash_env::option::State::{Off, On};
-use yash_env::variable::AssignError;
 use yash_env::variable::Scope::Global;
 use yash_env::variable::Value::Scalar;
 use yash_env::variable::Variable;
@@ -144,7 +144,7 @@ struct UnsetVariable;
 /// It is used to reproduce a location contained in the error cause.
 #[must_use]
 fn convert_error_cause(
-    cause: yash_arith::ErrorCause<UnsetVariable, AssignError>,
+    cause: yash_arith::ErrorCause<UnsetVariable, AssignReadOnlyError>,
     source: &Rc<Code>,
 ) -> ErrorCause {
     use ArithError::*;
@@ -193,7 +193,7 @@ fn convert_error_cause(
             yash_arith::EvalError::ReverseShifting => ErrorCause::ArithError(ReverseShifting),
             yash_arith::EvalError::AssignmentToValue => ErrorCause::ArithError(AssignmentToValue),
             yash_arith::EvalError::GetVariableError(UnsetVariable) => ErrorCause::UnsetParameter,
-            yash_arith::EvalError::AssignVariableError(e) => ErrorCause::AssignError(e),
+            yash_arith::EvalError::AssignVariableError(e) => ErrorCause::AssignReadOnly(e),
         },
     }
 }
@@ -206,7 +206,7 @@ struct VarEnv<'a> {
 
 impl<'a> yash_arith::Env for VarEnv<'a> {
     type GetVariableError = UnsetVariable;
-    type AssignVariableError = AssignError;
+    type AssignVariableError = AssignReadOnlyError;
 
     #[rustfmt::skip]
     fn get_variable(&self, name: &str) -> Result<Option<&str>, UnsetVariable> {
@@ -227,7 +227,7 @@ impl<'a> yash_arith::Env for VarEnv<'a> {
         name: &str,
         value: String,
         range: Range<usize>,
-    ) -> Result<(), AssignError> {
+    ) -> Result<(), AssignReadOnlyError> {
         let code = Rc::new(Code {
             value: self.expression.to_string().into(),
             start_line_number: 1.try_into().unwrap(),
@@ -239,6 +239,11 @@ impl<'a> yash_arith::Env for VarEnv<'a> {
             .get_or_create_variable(name, Global)
             .assign(value, Location { code, range })
             .map(drop)
+            .map_err(|e| AssignReadOnlyError {
+                name: name.to_owned(),
+                new_value: e.new_value,
+                read_only_location: e.read_only_location,
+            })
     }
 }
 

yash-semantics/src/expansion/initial/param/switch.rs

@@ -25,6 +25,7 @@ use crate::expansion::attr_strip::Strip;
 use crate::expansion::expand_word;
 use crate::expansion::initial::expand;
 use crate::expansion::quote_removal::skip_quotes;
+use crate::expansion::AssignReadOnlyError;
 use crate::expansion::ErrorCause;
 use yash_env::variable::Scope;
 use yash_env::variable::Value;
@@ -176,7 +177,7 @@ async fn assign(
 ) -> Result<Phrase, Error> {
     // TODO Support assignment to an array element
     let name = match name {
-        Some(Name::Variable(name)) => name.to_owned(),
+        Some(Name::Variable(name)) => name,
         _ => {
             let cause = ErrorCause::NonassignableParameter(NonassignableError::NotVariable);
             return Err(Error { cause, location });
@@ -189,8 +190,12 @@ async fn assign(
         .get_or_create_variable(name, Scope::Global)
         .assign(final_value, location)
         .map_err(|e| {
-            let location = e.assigned_location.as_ref().unwrap().clone();
-            let cause = ErrorCause::AssignError(e);
+            let location = e.assigned_location.unwrap();
+            let cause = ErrorCause::AssignReadOnly(AssignReadOnlyError {
+                name: name.to_owned(),
+                new_value: e.new_value,
+                read_only_location: e.read_only_location,
+            });
             Error { cause, location }
         })?;
     Ok(value_phrase)
@@ -538,10 +543,10 @@ mod tests {
             .unwrap();
         assert_matches!(result, Some(Err(error)) => {
             assert_eq!(error.location, location);
-            assert_matches!(error.cause, ErrorCause::AssignError(e) => {
+            assert_matches!(error.cause, ErrorCause::AssignReadOnly(e) => {
+                assert_eq!(e.name, "var");
                 assert_eq!(e.new_value, Value::scalar("foo"));
                 assert_eq!(e.read_only_location, Location::dummy("read-only"));
-                assert_eq!(e.assigned_location, Some(location));
             });
         });
         assert_eq!(env.inner.variables.get("var"), Some(&save_var));