SeqMap.keys is Seq [ci: last-only]

[som-snytt]

Fixes scala/bug#12991

Ensure that keys of a SeqMap is a Seq.

That satisfies the unwritten expectation that keys is ordered and mutually comparable.

[som-snytt]

@lrytz deprecated overriding of SeqMap#keys may have been hasty. IF it is useful (a big IF), it is usefully specialized. WDYT?

#10544

[sjrd]

FWIW the deprecation has been mildly inconvenient for us:
https://github.com/scala-js/scala-js/blob/28366eab2923a5807f02d421871eeb1dfaaf0ae3/project/Build.scala#L1789
https://github.com/scala-js/scala-js/blob/28366eab2923a5807f02d421871eeb1dfaaf0ae3/library/src/main/scala-new-collections/scala/scalajs/js/WrappedDictionary.scala#L102

[lrytz]

I think either keys is an alais of keySet, or we guarantee additional properties. I assume these would be "strict copy" and "same iteration order for ordered / sorted maps"? Maybe we can add overrides that reutrn Seq after SIP-51, but even without that the properties can be expressed in scaladoc and tests, and it's clear whether something is a bug or not.

Doing something in between can probably avoid some surprises but not really prevent bugs.

[som-snytt]

Not sure if lrytz agreed to undeprecate, so I pushed it as a separate commit, unsquashed. Sorry, Jenkins.

To be clear, this PR was motivated by Ichoran's endorsement on the ticket, that even if keys is not (yet) Seq, go ahead and change it to return one anyway, as nobody could possibly be relying on the current behavior.

with nobody being the wiser for it

I can report that I do not feel wiser.

[lrytz]

I see the point that returning a Seq doesn't hurt and can prevent surprises.

But if we don't actually guarantee anything then the value is very limited, surprises when using keys are just delayed.

I think either keys should be a final / deprecatedOverriding alias, or it should have documented properties that clients can rely on, anything in between is useful only by accident. But changing keys to always return a strict Seq copy is a (performance) breaking change, so it's not clear whether we can do that.

[som-snytt]

This is only for SeqMaps that don't yet override. It could be restricted further to SeqMap1..4, to be more conservative.

Then we can sort whether it's useful API etc under SIP-51.

Otherwise, I'll have to at-notify Ichoran.

[som-snytt]

it's not clear what the API intends:

      assertEquals(/*"Expect the same keys to appear in the join taken either way around.",*/
        ourChanges.mergeByKey(theirChangesRedux).keySet,
        theirChangesRedux.mergeByKey(ourChanges).keys,
      )