Fix bug where Flags.remove could set flags in addition to unsetting them

[redvers]

Note - I've written a test-suite to cover 90% of this module but it's dependent on a second PR that I'm about to open.

Test coverage is in there.

[SeanTAllen]

The title of the PR will appear in the changelog, can you change it to something more "end user friendly"?

[SeanTAllen]

FYI a "Fixes X" should appear in general at the end of the commit comment.

[ponylang-main]

Hi @redvers,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 3777.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

[SeanTAllen]

I'd rather see the test coverage as either part of this PR or for the test coverage PR to come first and this to follow. There's no test here to show this works and doesn't break other things.

I'd be in favor of adding the test coverage to this PR.

[redvers]

So, my test suite uses the new create() constructor throughout... so won't work unless that change is also merged.

eg:

    // Mutable Bitwise Operator: without
                                                           // 0b0011
                                                           // 0b0101 without
    h.assert_true(set0011.without(set0101) == _TestFlagsFlags(0b0010))
                                                           // 0b0101
                                                           // 0b0011 without
    h.assert_true(set0101.without(set0011) == _TestFlagsFlags(0b0100))


    // Mutable Operator: union
    t = set0011.clone()              //0b0011
    t.union(set0101)                 //0b0101
    h.assert_true(t == _TestFlagsFlags(0b0111))

    // a.union(b) == b.union(a)
    t = set0101.clone()              //0b0101
    t.union(set0011)                 //0b0011
    h.assert_true(t == _TestFlagsFlags(0b0111))

Should I put it all in one then?

[SeanTAllen]

Is the bug possible without the create change?

[redvers]

I'll re-write the test suite to use the other format and push.

[SeanTAllen]

@redvers can you update the commit comment to be:

Fix bug where Flags.remove could set flags in addition to unsetting them

Flags.remove when given a flag to remove that wasn't currently present in the set, would turn the flag on. 
It should only be turning flags off, not turning them on.

Closes #3776 

Release notes of

## Fix bug where Flags.remove could set flags in addition to unsetting them

Flags.remove when given a flag to remove that wasn't currently present in the set, would turn the flag on. 
It should only be turning flags off, not turning them on.

would be good as well.

[SeanTAllen]

These two

• https://playground.ponylang.io/?gist=038f92acb95f87e33a2cdbcfae335e40
• https://playground.ponylang.io/?gist=2dfbb2ea6a8fa1af192ebeba86474bd8

as tests would be good enough coverage for me to push this update through.

We don't need a full suite. Merely ones showing the two basic cases we expect for remove and them passing with this change.

[SeanTAllen]

@redvers I can do the commit comment change on squash/merge.

[redvers]

If it's okay with you I'd like to do the squash. I want to make sure I understand the full-workflow so any future work will require less work from the maintainers.

[SeanTAllen]

@redvers fine with me.

[redvers]

Okay - so I think pending CI - I think that's everything? Yay!