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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ast/term: sort set's keys slice on insert #4028

Merged

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Nov 17, 2021

Before, we'd sort a set's keys slice in-place, and only when it
was actually compared to another set. This side-effect of a comparison
can be unexpected.

Now, we'll keep the keys slice sorted by inserting every new element
into its sorted position.

Note that we still sort in-place on Compare, because its possible the
sets values have been altered in different ways (e.g. test TestSetCopy).

Analogous to #3823, where we did this with objects' keys slices.


Also noticed a problem of #3823: the ordering could me messed up in ways different from inserting key/val pairs, e.g. by changing key names in Iter(), so we'll need to care about our invariant after the Iter() call.


馃毀 Some discussion items below 馃憞

@srenatus
Copy link
Contributor Author

srenatus commented Nov 17, 2021

We cannot get rid of the other sort calls,

because we're returning the underlying keys slice in (set).Slice() []*Term. If we made that return a copy instead, we would be able to ensure that our ordering isn't messed up.

@srenatus
Copy link
Contributor Author

I've looked more into this, but returning a copy of the terms from Slice() is at odds with how the other methods work: none of the Object methods, for example Keys(), or Iter(), operate on copies (except Map). So, this would single out sets in the wrong way.

@srenatus srenatus force-pushed the sr/ast/sort-sets-on-insert branch 2 times, most recently from 05f392f to 52befca Compare November 17, 2021 13:24
@srenatus
Copy link
Contributor Author

OK, I've dialled this back a little: we now do what's advertised in the title: sort sets at insertion time. We still sort them (again) when comparing, or when printing them, because we don't know the ordering hasn't been messed with.

@srenatus srenatus marked this pull request as ready for review November 17, 2021 13:43
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just make this PR do the sort-on-insert change to sets. The reason is that the data structures do not support in-place mutation today--if something reaches into the elements and mutates them, we're going to get undefined behaviour with Iter() and other functions. Unfortunately, sort-on-compare does not solve this. Let's yank those other changes out and just sort-on-insert in sets.

@srenatus srenatus force-pushed the sr/ast/sort-sets-on-insert branch 2 times, most recently from d0e90e7 to da87d15 Compare November 17, 2021 18:35
tsandall
tsandall previously approved these changes Nov 17, 2021
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Before, we'd sort a set's `keys` slice in-place, and only when it
was actually compared to another set. This side-effect of a comparison
can be unexpected.

Now, we'll keep the `keys` slice sorted by inserting every new element
into its sorted position.

Analogous to open-policy-agent#3823, where we did this with objects' `keys` slices.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@@ -1348,15 +1348,12 @@ func (s *set) String() string {
}
var b strings.Builder
b.WriteRune('{')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsandall sorry f-pushed another one after your review -- this was the change I had not included before 馃憞

@srenatus srenatus merged commit 7b8b6d8 into open-policy-agent:main Nov 17, 2021
@srenatus srenatus deleted the sr/ast/sort-sets-on-insert branch November 17, 2021 18:54
floriangasc pushed a commit to floriangasc/opa that referenced this pull request Nov 22, 2021
Before, we'd sort a set's `keys` slice in-place, and only when it
was actually compared to another set. This side-effect of a comparison
can be unexpected.

Now, we'll keep the `keys` slice sorted by inserting every new element
into its sorted position.

Analogous to open-policy-agent#3823, where we did this with objects' `keys` slices.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@philipaconrad philipaconrad mentioned this pull request Jul 1, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants