@ysv reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)

a discussion (no related file):
@miladz68 @masihyeganeh do you think it is safe to include this deterministicmap change in v6 release ? Or better to have it for the next one so we test it longer ?

go.work line 11 at r1 (raw file):

use ./integration-tests

use ./pkg/deterministic_map/deterministicmaplint

I think in golang it is suggested to use - dashes or single word for directory names ?

go.work line 11 at r1 (raw file):

use ./integration-tests

use ./pkg/deterministic_map/deterministicmaplint

is it required to have deterministicmaplint as a separate module ?

pkg/deterministic_map/deterministicmaplint/deterministicmaplint.go line 1 at r1 (raw file):

package deterministicmaplint

what about existing linter which @TxCorpi0x proposed ?
Can't we use it instead ?

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)

pkg/deterministic_map/deterministicmaplint/deterministicmaplint.go line 1 at r1 (raw file):

or this option is better ?

@ysv reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x).

pkg/deterministic_map/deterministicmaplint/testdata/src/testlintdata/deterministic/deterministicmap.go line 7 at r3 (raw file):

func SomeFunc() {
	m := map[string]int{"a": 1}
	for k, v := range m /* want "ranging over map is forbidden (iteration order is nondeterministic); use DeterministicMap instead" */ {

should we have a successful test case ?
E.g one which uses DeterministicMap ? or has different map usage without range ?

@masihyeganeh made 5 comments.
Reviewable status: 9 of 12 files reviewed, 5 unresolved discussions (waiting on @miladz68, @TxCorpi0x, and @ysv).

a discussion (no related file):

It is decided to not include it in the v6 release, but it will be merged to master for next releases

go.work line 11 at r1 (raw file):

Done.

go.work line 11 at r1 (raw file):

Yes. In the .custom-gcl.yml file, we need to pass module path of a package with a go.mod file

pkg/deterministic_map/deterministicmaplint/deterministicmaplint.go line 1 at r1 (raw file):

Tested forbidigo and go-critics and they could not provide such functionality

pkg/deterministic_map/deterministicmaplint/testdata/src/testlintdata/deterministic/deterministicmap.go line 7 at r3 (raw file):

After moving to range implementation, their testing method does not handle that and I had to remove the test completely

@masihyeganeh reviewed 12 files and all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @miladz68, @TxCorpi0x, and @ysv).

@masihyeganeh partially reviewed 17 files and all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @miladz68, @TxCorpi0x, and @ysv).

@ysv partially reviewed 19 files and all commit messages, and resolved 5 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @TxCorpi0x).

@TxCorpi0x partially reviewed 20 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68).

@miladz68 partially reviewed 20 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @masihyeganeh).

pkg/deterministic-map/deterministic_map.go line 64 at r6 (raw file):

// Get retrieves a value by key.
func (m *Map[K, V]) Get(key K) (V, bool) {

I think we can get better performance if the map contains key->array index and we store the values inside the list. What do you think ?

pkg/deterministic-map/deterministic_map.go line 86 at r6 (raw file):

	delete(m.data, key)

	for i, k := range m.keys {

this looks a little expensive

pkg/deterministic-map/deterministic_map.go line 145 at r6 (raw file):

// Range iterates over the map in deterministic sorted order.
// Returning false from fn stops iteration.
func (m *Map[K, V]) Range(fn func(key K, value V) bool) {

range and rangeErr can be merged, so that fn return signature is (bool,error)

pkg/deterministic-map/deterministic_map.go line 152 at r6 (raw file):

	m.ensureSorted()

	for _, k := range m.keys {

this also looks a bit expensive as well.

@miladz68 made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @masihyeganeh).

a discussion (no related file):
we are using maps inside some of the modules. why is the linter not warning us to change them ?

@masihyeganeh reviewed 4 files and all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @masihyeganeh).

@ysv reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @masihyeganeh and @miladz68).

pkg/deterministic-map/deterministic_map.go line 86 at r6 (raw file):

hm, good point. might be very expensive on a big data set

What is your suggestion @miladz68
Should we use binary search here since keys are ordered ? Or maybe rethink the approach fully ?

Because another option is to sort keys only inside Range function. This might be more reasonable

pkg/deterministic-map/deterministic_map.go line 152 at r6 (raw file):

or at least re-use the same code in both.
So Range calls RangeErr and doesn't return err

@masihyeganeh partially reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @miladz68).

@ysv partially reviewed 3 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @masihyeganeh and @miladz68).

pkg/deterministic-map/deterministic_map.go line 14 at r7 (raw file):

// entry represents a key/value pair.
type entry[K comparable, V any] struct {
	key   K

I'm just not sure if we need both K & V in entries slice.
Is there a usage for entries[i].key ?

Code quote:

key   K

pkg/deterministic-map/deterministic_map.go line 68 at r7 (raw file):

// New keys are appended deterministically.
func (m *Map[K, V]) Set(key K, value V) {
	m.ensure()

in which case we might have m.index == nil (since it is initialized with make(map[K]int)) ?
Do we still need this ensure call everywhere ?

also I still see check if m.index == nil in different places. But is it really possible or this is some legacy ?

pkg/deterministic-map/deterministic_map.go line 107 at r7 (raw file):

	}

	last := len(m.entries) - 1

nit: lastIdx

pkg/deterministic-map/deterministic_map.go line 110 at r7 (raw file):

	lastEntry := m.entries[last]

	if i != last {

lets add more test-cases
e.g for this scenario

@miladz68 reviewed 2 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @masihyeganeh and @ysv).

pkg/deterministic-map/deterministic_map.go line 14 at r7 (raw file):

I agree, the entry type seems redundant.

@masihyeganeh made 7 comments and resolved 1 discussion.
Reviewable status: 19 of 21 files reviewed, 5 unresolved discussions (waiting on @miladz68, @TxCorpi0x, and @ysv).

pkg/deterministic-map/deterministic_map.go line 64 at r6 (raw file):

Done.

pkg/deterministic-map/deterministic_map.go line 86 at r6 (raw file):

Done. Fixed by the alternative implementation

pkg/deterministic-map/deterministic_map.go line 145 at r6 (raw file):

Done. Merged

pkg/deterministic-map/deterministic_map.go line 152 at r6 (raw file):

Those are merged. I can return the entries list here (or clone of that to make sure we don't mutate it) instead of calling the callback function. Let me know which one you think is better

pkg/deterministic-map/deterministic_map.go line 14 at r7 (raw file):

Since we are imitating a map, in the Range function, we should call the callback with key and value. Since we just loop over this slice in the Range function, we need the key to exist here and there is no way to look it up otherwise.
Let me know if we want to just loop over the values and we will never need the keys

pkg/deterministic-map/deterministic_map.go line 68 at r7 (raw file):

It's just an way to make sure we can use this map without actually initializing it.
This code should work with this implementation:

var m Map[string, string]
m.Set("key", "val")

It is not important at all. If you believe we don't need it, I can just remove it

pkg/deterministic-map/deterministic_map.go line 107 at r7 (raw file):

Done.

@masihyeganeh made 1 comment.
Reviewable status: 19 of 21 files reviewed, 5 unresolved discussions (waiting on @miladz68, @TxCorpi0x, and @ysv).

a discussion (no related file):

We only check where we range over a map. Do you think it should have found other instances?

@masihyeganeh partially reviewed 2 files.
Reviewable status: all files reviewed (commit messages unreviewed), 5 unresolved discussions (waiting on @miladz68 and @ysv).

@masihyeganeh reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @miladz68 and @ysv).

@ysv made 1 comment and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @masihyeganeh and @miladz68).

pkg/deterministic-map/deterministic_map.go line 68 at r7 (raw file):

lets enforce proper initialization via .New method

@masihyeganeh partially reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68 and @ysv).

pkg/deterministic-map/deterministic_map.go line 68 at r7 (raw file):

Done,

pkg/deterministic-map/deterministic_map.go line 110 at r7 (raw file):

Done. It just replaces the item we want to delete with the last one and then shrinks the slice. Added test for removing one item and zero item

@masihyeganeh reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68 and @ysv).

@miladz68 partially reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ysv).

@ysv partially reviewed 3 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68).

@miladz68 resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh).