styled: Clean up representation of Segment and Text.

Segment is now always used as a pointer and Text is always used as a
value.

Add test to make sure that the "styled" builtin does not modify its
arguments.

This fixes #719.

eval/builtin_fn_styled.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/elves/elvish/eval/vals"
+	"github.com/elves/elvish/parse"
 	"github.com/elves/elvish/styled"
 )
 
@@ -46,19 +47,19 @@ func styledSegment(options RawOptions, input interface{}) (*styled.Segment, erro
 
 // Styled turns a string, a styled Segment or a styled Text into a styled Text.
 // This is done by applying a range of transformers to the input.
-func Styled(fm *Frame, input interface{}, transformers ...interface{}) (*styled.Text, error) {
+func Styled(fm *Frame, input interface{}, transformers ...interface{}) (styled.Text, error) {
 	var text styled.Text
 
 	switch input := input.(type) {
 	case string:
-		text = styled.Text{styled.Segment{
+		text = styled.Text{&styled.Segment{
 			Text:  input,
 			Style: styled.Style{},
 		}}
 	case *styled.Segment:
-		text = styled.Text{*input}
-	case *styled.Text:
-		text = *input
+		text = styled.Text{input.Clone()}
+	case styled.Text:
+		text = input.Clone()
 	default:
 		return nil, fmt.Errorf("expected string, styled segment or styled text; got %s", vals.Kind(input))
 	}
@@ -68,26 +69,24 @@ func Styled(fm *Frame, input interface{}, transformers ...interface{}) (*styled.
 		case string:
 			transformerFn := styled.FindTransformer(transformer)
 			if transformerFn == nil {
-				return nil, fmt.Errorf("'%s' is no valid style transformer", transformer)
+				return nil, fmt.Errorf("%s is not a valid style transformer", parse.Quote(transformer))
 			}
-
-			for i, segment := range text {
-				text[i] = transformerFn(segment)
+			for _, seg := range text {
+				transformerFn(seg)
 			}
-
 		case Callable:
-			for i, segment := range text {
-				vs, err := fm.CaptureOutput(transformer, []interface{}{&segment}, NoOpts)
+			for i, seg := range text {
+				vs, err := fm.CaptureOutput(transformer, []interface{}{seg}, NoOpts)
 				if err != nil {
 					return nil, err
 				}
 
 				if n := len(vs); n != 1 {
-					return nil, fmt.Errorf("style transformers must return a single styled segment; got %d", n)
+					return nil, fmt.Errorf("style transformers must return a single styled segment; got %d values", n)
 				} else if transformedSegment, ok := vs[0].(*styled.Segment); !ok {
 					return nil, fmt.Errorf("style transformers must return a styled segment; got %s", vals.Kind(vs[0]))
 				} else {
-					text[i] = *transformedSegment
+					text[i] = transformedSegment
 				}
 			}
 
@@ -96,5 +95,5 @@ func Styled(fm *Frame, input interface{}, transformers ...interface{}) (*styled.
 		}
 	}
 
-	return &text, nil
+	return text, nil
 }

eval/builtin_fn_styled_test.go

@@ -7,7 +7,7 @@ import (
 
 func TestStyledString(t *testing.T) {
 	Test(t,
-		That("print (styled abc hopefully-never-exists)").ErrorsWith(errors.New("'hopefully-never-exists' is no valid style transformer")),
+		That("print (styled abc hopefully-never-exists)").ErrorsWith(errors.New("hopefully-never-exists is not a valid style transformer")),
 		That("print (styled abc bold)").Prints("\033[1mabc\033[m"),
 		That("print (styled abc red cyan)").Prints("\033[36mabc\033[m"),
 		That("print (styled abc bg-green)").Prints("\033[42mabc\033[m"),
@@ -46,6 +46,15 @@ func TestStyledText(t *testing.T) {
 	)
 }
 
+func TestStyled_DoesNotModifyArgument(t *testing.T) {
+	Test(t,
+		That("x = (styled text); _ = (styled $x red); put $x[0][fg-color]").
+			Puts("default"),
+		That("x = (styled-segment text); _ = (styled $x red); put $x[fg-color]").
+			Puts("default"),
+	)
+}
+
 func TestStyledConcat(t *testing.T) {
 	Test(t,
 		// string+segment

styled/segment.go

@@ -14,12 +14,12 @@ type Segment struct {
 	Text string
 }
 
-func (Segment) Kind() string { return "styled-segment" }
+func (*Segment) Kind() string { return "styled-segment" }
 
 // Repr returns the representation of this Segment. The string can be used to
 // construct an identical Segment. Unset or default attributes are skipped. If
 // the Segment represents an unstyled string only this string is returned.
-func (s Segment) Repr(indent int) string {
+func (s *Segment) Repr(indent int) string {
 	buf := new(bytes.Buffer)
 	addIfNotEqual := func(key string, val, cmp interface{}) {
 		if val != cmp {
@@ -43,12 +43,12 @@ func (s Segment) Repr(indent int) string {
 	return fmt.Sprintf("(styled-segment %s %s)", s.Text, strings.TrimSpace(buf.String()))
 }
 
-func (s Segment) IterateKeys(fn func(v interface{}) bool) {
+func (*Segment) IterateKeys(fn func(v interface{}) bool) {
 	vals.Feed(fn, "text", "fg-color", "bg-color", "bold", "dim", "italic", "underlined", "blink", "inverse")
 }
 
 // Index provides access to the attributes of the Segment.
-func (s Segment) Index(k interface{}) (v interface{}, ok bool) {
+func (s *Segment) Index(k interface{}) (v interface{}, ok bool) {
 	switch k {
 	case "text":
 		v = s.Text
@@ -78,31 +78,36 @@ func (s Segment) Index(k interface{}) (v interface{}, ok bool) {
 }
 
 // Concat implements Segment+string, Segment+Segment and Segment+Text.
-func (s Segment) Concat(v interface{}) (interface{}, error) {
+func (s *Segment) Concat(v interface{}) (interface{}, error) {
 	switch rhs := v.(type) {
 	case string:
 		return Text{
 			s,
-			Segment{Text: rhs},
+			&Segment{Text: rhs},
 		}, nil
 	case *Segment:
-		return Text{s, *rhs}, nil
-	case *Text:
-		return Text(append([]Segment{s}, *rhs...)), nil
+		return Text{s, rhs}, nil
+	case Text:
+		return Text(append([]*Segment{s}, rhs...)), nil
 	}
 
 	return nil, vals.ErrConcatNotImplemented
 }
 
 // RConcat implements string+Segment.
-func (s Segment) RConcat(v interface{}) (interface{}, error) {
+func (s *Segment) RConcat(v interface{}) (interface{}, error) {
 	switch lhs := v.(type) {
 	case string:
 		return Text{
-			Segment{Text: lhs},
+			&Segment{Text: lhs},
 			s,
 		}, nil
 	}
-
 	return nil, vals.ErrConcatNotImplemented
 }
+
+// Clone returns a copy of the Segment.
+func (s *Segment) Clone() *Segment {
+	value := *s
+	return &value
+}

styled/text.go

@@ -9,7 +9,7 @@ import (
 )
 
 // Text contains of a list of styled Segments.
-type Text []Segment
+type Text []*Segment
 
 func (t Text) Kind() string { return "styled-text" }
 
@@ -47,11 +47,11 @@ func (t Text) Index(k interface{}) (interface{}, error) {
 func (t Text) Concat(v interface{}) (interface{}, error) {
 	switch rhs := v.(type) {
 	case string:
-		return Text(append(t, Segment{Text: rhs})), nil
+		return Text(append(t, &Segment{Text: rhs})), nil
 	case *Segment:
-		return Text(append(t, *rhs)), nil
-	case *Text:
-		return Text(append(t, *rhs...)), nil
+		return Text(append(t, rhs)), nil
+	case Text:
+		return Text(append(t, rhs...)), nil
 	}
 
 	return nil, vals.ErrConcatNotImplemented
@@ -61,7 +61,7 @@ func (t Text) Concat(v interface{}) (interface{}, error) {
 func (t Text) RConcat(v interface{}) (interface{}, error) {
 	switch lhs := v.(type) {
 	case string:
-		return Text(append([]Segment{{Text: lhs}}, t...)), nil
+		return Text(append([]*Segment{{Text: lhs}}, t...)), nil
 	}
 
 	return nil, vals.ErrConcatNotImplemented
@@ -76,14 +76,14 @@ func (t Text) Partition(indicies ...int) []Text {
 	for i, idx := range indicies {
 		text := make(Text, 0)
 		for len(segs) > 0 && idx >= consumedSegsLen+len(segs[0].Text) {
-			text = append(text, Segment{
+			text = append(text, &Segment{
 				segs[0].Style, segs[0].Text[seg0Consumed:]})
 			consumedSegsLen += len(segs[0].Text)
 			seg0Consumed = 0
 			segs = segs[1:]
 		}
 		if len(segs) > 0 && idx > consumedSegsLen {
-			text = append(text, Segment{
+			text = append(text, &Segment{
 				segs[0].Style, segs[0].Text[:idx-consumedSegsLen]})
 			seg0Consumed = idx - consumedSegsLen
 		}
@@ -91,7 +91,7 @@ func (t Text) Partition(indicies ...int) []Text {
 	}
 	trailing := make(Text, 0)
 	for len(segs) > 0 {
-		trailing = append(trailing, Segment{
+		trailing = append(trailing, &Segment{
 			segs[0].Style, segs[0].Text[seg0Consumed:]})
 		seg0Consumed = 0
 		segs = segs[1:]
@@ -99,3 +99,12 @@ func (t Text) Partition(indicies ...int) []Text {
 	out[len(indicies)] = trailing
 	return out
 }
+
+// Clone returns a deep copy of Text.
+func (t Text) Clone() Text {
+	newt := make(Text, len(t))
+	for i, seg := range t {
+		newt[i] = seg.Clone()
+	}
+	return newt
+}

styled/text_test.go

@@ -16,8 +16,8 @@ var (
 	text2 = Text{red("lorem"), blue("foobar")}
 )
 
-func red(s string) Segment  { return Segment{Style{Foreground: "red"}, s} }
-func blue(s string) Segment { return Segment{Style{Foreground: "blue"}, s} }
+func red(s string) *Segment  { return &Segment{Style{Foreground: "red"}, s} }
+func blue(s string) *Segment { return &Segment{Style{Foreground: "blue"}, s} }
 
 var partitionTests = tt.Table{
 	Args(text0).Rets([]Text{text0}),

styled/transform.go

@@ -6,38 +6,25 @@ import (
 
 // FindTransformer looks up a transformer name and if successful returns a
 // function that can be used to transform a styled Segment.
-func FindTransformer(transformerName string) func(Segment) Segment {
-	var innerTransformer func(*Segment)
-
+func FindTransformer(transformerName string) func(*Segment) {
 	switch {
 	// Catch special colors early
 	case transformerName == "default":
-		innerTransformer = func(s *Segment) { s.Foreground = "" }
+		return func(s *Segment) { s.Foreground = "" }
 	case transformerName == "bg-default":
-		innerTransformer = func(s *Segment) { s.Background = "" }
-
+		return func(s *Segment) { s.Background = "" }
 	case strings.HasPrefix(transformerName, "bg-"):
-		innerTransformer = buildColorTransformer(strings.TrimPrefix(transformerName, "bg-"), false)
+		return buildColorTransformer(strings.TrimPrefix(transformerName, "bg-"), false)
 	case strings.HasPrefix(transformerName, "no-"):
-		innerTransformer = buildBoolTransformer(strings.TrimPrefix(transformerName, "no-"), false, false)
+		return buildBoolTransformer(strings.TrimPrefix(transformerName, "no-"), false, false)
 	case strings.HasPrefix(transformerName, "toggle-"):
-		innerTransformer = buildBoolTransformer(strings.TrimPrefix(transformerName, "toggle-"), false, true)
+		return buildBoolTransformer(strings.TrimPrefix(transformerName, "toggle-"), false, true)
 
 	default:
-		innerTransformer = buildColorTransformer(transformerName, true)
-
-		if innerTransformer == nil {
-			innerTransformer = buildBoolTransformer(transformerName, true, false)
+		if f := buildColorTransformer(transformerName, true); f != nil {
+			return f
 		}
-	}
-
-	if innerTransformer == nil {
-		return nil
-	}
-
-	return func(segment Segment) Segment {
-		innerTransformer(&segment)
-		return segment
+		return buildBoolTransformer(transformerName, true, false)
 	}
 }