Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: slices: MutableValues iterator #70813

Open
pkierski opened this issue Dec 12, 2024 · 14 comments
Open

proposal: slices: MutableValues iterator #70813

pkierski opened this issue Dec 12, 2024 · 14 comments
Labels
Milestone

Comments

@pkierski
Copy link

Proposal Details

It's a common task to iterate over a slice and modify its elements. For the following structure:

type element struct {
	field int
}

var (
	elements = []element{
		{field: 1},
		{field: 2},
		{field: 3},
	}

	doubledElements = []element{
		{field: 2},
		{field: 4},
		{field: 6},
	}
)

we have to use one of:

func TestDoubleWithRangeAndIndex(t *testing.T) {
	// change base elements without MutableValues
	// using indexing
	for i, e := range elements {
		e.field *= 2
		elements[i] = e
	}

	if !slices.Equal(elements, doubledElements) {
		t.Fail()
	}
}

or

func TestDoubleWithRangeAndPointer(t *testing.T) {
	// change base elements without MutableValues
	// using pointer to element (useful in case of neested loops
	// or operations on more than one field)
	for i := range elements {
		e := &elements[i]
		e.field *= 2
	}

	if !slices.Equal(elements, doubledElements) {
		t.Fail()
	}
}

The proposal is to add MutableValues iterator to slices package:

func MutableValues[Slice ~[]E, E any](s Slice) iter.Seq[*E] {
	return func(yield func(*E) bool) {
		for i := range s {
			if !yield(&s[i]) {
				return
			}
		}
	}
}

Mutating of all elements of the slice can looks like:

func TestDoubleWithMutableValuesIterator(t *testing.T) {
	// values in elements will be changed
	for e := range slices.MutableValues(elements) {
		// we can directly change element via pointer
		e.field *= 2
	}

	if !slices.Equal(elements, doubledElements) {
		t.Fail()
	}
}
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gopherbot gopherbot added this to the Proposal milestone Dec 12, 2024
@zigo101
Copy link

zigo101 commented Dec 15, 2024

@pkierski
I suggest you to re-open this issue. Because the other two have been closed, both by their owners, not by the proposal process.
There should be one opened.

@pkierski pkierski reopened this Dec 15, 2024
@seankhliao
Copy link
Member

This is just a restatement of #69916 without addressing any of the concerns in the comments. I don't think we need to rehash the discussion.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Dec 15, 2024
@zigo101
Copy link

zigo101 commented Dec 16, 2024

I think @pkierski well addresses the concerns in the first comment.

The other two haven't undergone the proposal process yet.

@ianlancetaylor @aclements

@zigo101
Copy link

zigo101 commented Dec 18, 2024

@ianlancetaylor @aclements

Kindly re-ping. This alike proposals have never been accepted or declined. Do you think @seankhliao have the right to close it?
It looks to me @seankhliao has violated the proposal process.

@ianlancetaylor
Copy link
Member

OK, I'll reopen it to follow the process, but I think the comments are fair: this proposal does not appear to address the shortcomings pointed out for the earlier proposals. Without doing that the proposal review committee is just going to close it. In particular the behavior if the slice changes during iteration is going to be unclear and surprising.

@zigo101
Copy link

zigo101 commented Dec 18, 2024

this proposal does not appear to address the shortcomings pointed out for the earlier proposals

I don't see any valid objection reasons there. Those reasons look not iterator specific. They also apply to traditional loops.

@seankhliao
Copy link
Member

The objection is not that it isn't sometimes useful, but that the existence of a standard library function decreases safety by making a footgun more accessible, and encourages a poor choice of slice element types.

@zigo101
Copy link

zigo101 commented Dec 18, 2024

As I have stated in my last comment, the so-called "footgun" exists before Go 1.23, which introduced iterators.

@apparentlymart
Copy link

The baseline without this proposal is (as shown in the proposal text) a loop which begins by immediately taking the address of the current element:

for i := range example {
    e := &example[i]

    // and then, presumably, mutate what e points at
}

The net effect of this proposal is to move the e := &example[i] statement out of the loop body and into the function returned by the slices.MutableValues helper:

for i, e := range slices.MutableValues(example) {
    // and then, presumably, mutate what e points at
}

It seems like this is a classic tension between convenience and explicitness.

The original example makes it relatively clear in the loop body that e is the address obtained through a slice, which is a clue to an experienced Gopher that they are holding a pointer to something in the slice's backing array and thus that pointer remains valid only as long as the slice is valid.

The proposed example moves that information slightly further out of reach: the reader must now understand what slices.MutableValues does and what implications that has. However, once you've learned what that function does you now have the same information as the experienced Gopher in my previous paragraph: you're holding a pointer to something in the slice's backing array, etc.

The way I understood the concerns in #69916 was that they were imagining someone maintaining some code that is already using this helper function but doesn't notice that e is a pointer because they didn't read the range clause well enough and are accustomed to how slice iteration normally behaves. They might then use e in a way that is not just indirect mutation and instead retains that pointer beyond the end of the loop.

It's always tricky to trade off a concrete benefit against a hypothetical hazard, but perhaps we can sidestep that by asking a more straightforward question: is the benefit of eliminating that single statement great enough to justify any language or library change at all?

As with the previous proposal, for me the primary concern is helping an author discover that they are mistakenly modifying a value that will be immediately discarded at the end of the loop iteration, which caused me to suggest a go vet rule in the other proposal. Personally, I think that helping authors avoid that confusing mistake is the most important thing, since adding that one extra statement to take the address once you've noticed your error doesn't tend to be a significant hardship.

However, I wouldn't be particularly upset if this proposal were accepted. I don't personally think it pulls its weight in terms of overall value provided, but it also seems like a relatively straightforward function that's unlikely to need significant maintenance once it's been added. 🤷‍♂️

@zigo101
Copy link

zigo101 commented Dec 19, 2024

The proposal has a big benefit. For a slice []E and a function needs a iter.Seq[*E] parameter, we don't need to construct a []*E slice from the []E slice to satisfy the parameter.

@apparentlymart
Copy link

The proposal doesn't include any examples of functions that require sequences of pointers in particular. I expect it would help to add some realistic examples of such situations if that's a significant part of the value proposition.

@pkierski
Copy link
Author

I agree that in simple use case (single loop) this is not worth to add new method like that. But for more complicated case like nested loops tratidional way is more complicated and not so easy to read:

type (
	element struct {
		field         int
		innerElements []innerElement
	}

	innerElement struct {
		innerField int
	}
)

var (
	srcElements = []element{
		{
			field: 1,
			innerElements: []innerElement{
				{innerField: 1},
				{innerField: 2},
			},
		},
		{
			field: 1,
			innerElements: []innerElement{
				{innerField: 3},
				{innerField: 4},
			},
		},
		{
			field: 1,
			innerElements: []innerElement{
				{innerField: 5},
				{innerField: 6},
			},
		},
	}

	doubledElements = []element{
		{
			field: 1,
			innerElements: []innerElement{
				{innerField: 2},
				{innerField: 4},
			},
		},
		{
			field: 1,
			innerElements: []innerElement{
				{innerField: 6},
				{innerField: 8},
			},
		},
		{
			field: 1,
			innerElements: []innerElement{
				{innerField: 10},
				{innerField: 12},
			},
		},
	}
)

func TestDoubleWithRangeAndIndex(t *testing.T) {
	elements := slices.Clone(srcElements)

	// change base elements without MutableValues
	// using indexing
	for i, e := range elements {
		for ii, ie := range e.innerElements {
			elements[i].innerElements[ii].innerField = ie.innerField * 2
			// alternative for mutating more than one inner field:
			// e.innerElements[ii].innerField = ie.innerField * 2
		}
		// alternative for mutating more than one inner field:
		//elements[i] = e
	}

	if !reflect.DeepEqual(elements, doubledElements) {
		t.Fail()
	}
}

func TestDoubleWithRangeAndPointer(t *testing.T) {
	elements := slices.Clone(srcElements)

	// change base elements without MutableValues
	// using pointer to element (useful in case of neested loops)
	for i := range elements {
		e := &elements[i]
		for ii := range e.innerElements {
			ie := &e.innerElements[ii]
			ie.innerField *= 2
		}
	}

	if !reflect.DeepEqual(elements, doubledElements) {
		t.Fail()
	}
}

MutableValues allows to write it in clean compact form:

func TestDoubleWithMutableValuesIterator(t *testing.T) {
	elements := slices.Clone(srcElements)

	// values in elements will be changed
	for e := range slices.MutableValues(elements) {
		for ie := range slices.MutableValues(e.innerElements) {
			ie.innerField *= 2
			// changing other fields is also easy here
		}
	}

	if !reflect.DeepEqual(elements, doubledElements) {
		t.Fail()
	}
}

@zigo101
Copy link

zigo101 commented Dec 19, 2024

I want to address it again. Iterators are not only to shorten loop code. They certainly have other meaningfulness. Otherwise, the following APIs in the std library are totally meaningless (they don't shorten loop code):

slices.All
slices.Values
maps.All
maps.Keys
maps.Values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

7 participants