scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

Improve documentation for deprecated methods that only make sense for parallel collections #12900

Closed SethTisue closed 6 months ago

SethTisue commented 7 months ago

at https://github.com/scala/toolkit/issues/31#issuecomment-1794225687 @philipschwarz noted:

Maybe instead of just saying that the aggregate function is not relevant for sequential collections. Use `foldLeft(z)(seqop) instead., "2.13.0 the [Scala Doc for aggregate]> (https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/IterableOnce.scala#L1122) could also say that aggregate is relevant for parallel collections and it could provide a link to https://github.com/scala/scala-parallel-collections.

perhaps the method should also be deprecated? and perhaps there are other such methods? @scala/collections

philipschwarz commented 7 months ago

"perhaps the method should also be deprecated?" - but it is already deprecated, right?

SethTisue commented 7 months ago

@philipschwarz You are right. In that case, perhaps all that's needed is to adjust the deprecation message and/or Scaladoc for the deprecated method(s). I've adjusted the ticket title accordingly.

I never took the parallel programming course — does aggregate come up there, is that the concern?

philipschwarz commented 7 months ago

I never took the parallel programming course — does aggregate come up there, is that the concern?

yes, it starts coming up

som-snytt commented 7 months ago

I was just visiting the ticket that scala 2.13.12> :require scala-parallel-collections_2.13-1.0.4.jar doesn't work.

It would be amazing to have targeted API, where certain symbols are "unlocked" by certain values of --release. Or perhaps unlocked if certain packages are available. aggregate would be invisible in Scala unless unlocked.

Similarly, at least the parallel classes that "infect" core packages could be preloaded in REPL but invisible. The REPL use case was ad hoc unlocking, which would require symbols exist but filtered from lookup.

ansvonwa commented 7 months ago

Actually, I do not really get why aggregate was deprecated in the first place: Wasn't the whole point of aggregate that I as a library/function author could use it to consume a collection independently of its internal structure?

Like "If someone hands me a parallel collection, it will be fast. If it's a non-parallel one (and maybe par collections aren't even on the classpath) it will still work as expected with expected performance. And if it's some very weird view of a distributed collection it will still be as fast as the collection allows." Didn't this use case became even more relevant when par collections were removed from the std lib? Before, you could (if you really cared) pattern match on the collection type. But when par collections may not even be on the classpath, this is not really an option.

Or was it just that nobody really used it so it was deemed irrelevant?

som-snytt commented 7 months ago

Not an expert, but it doesn't make sense on IterableOnce because you'd have to traverse it just to partition it.

My other mantra is that deprecation doesn't entail removal, it just means "there is a better way". In this case, use aggregate on a more suitable type, namely, a parallel thingy. (ParIterableLike)

My PR to supply some missing doc did not detect aggregate, maybe because of the deprecation noise.

I think combop, which I would have named comboop but I'm not an expert, should be marked unused here, as well. I was wondering if Scaladoc should document unusedness. "Brittle inheritance" means you never know what an override will do.

The semantics of comboop is restrictive, so for pedagogy some doc is needed. I'm willing to take a swing at capturing pithily both ansvonwa's niche use case and the deprecated sense that if you don't know what you're doing, this is probably not the operation you want. I've forgotten what "Odersky level" corresponds to ansvonwa's use case. @Odersky(L4) we need an official annotation. Then scalac --odersky=L4.

Deprecation affects autocomplete? That may justify deprecation. Experts will hit tab twice, or whatever the secret gesture.

som-snytt commented 7 months ago

I did not search for more work to do here: https://github.com/scala/scala/pull/10605/commits/1d0597c687d44e5c5c6a34ca27a419a007033d90

philipschwarz commented 7 months ago

speaking of the aggregate function, I just finished making this yesterday and can't resist the temptation to share it here in case someone looking at this ticket might be interested in it: http://fpilluminated.com/assets/scala-left-fold-parallelisation-three-approaches.html image

philipschwarz commented 7 months ago

@som-snytt FWIW, I left some comments in https://github.com/scala/scala/commit/1d0597c687d44e5c5c6a34ca27a419a007033d90, but only realised later that it was a commit rather than a PR.

som-snytt commented 7 months ago

@philipschwarz thanks, I definitely read

IMHO this is a great improvement.

which I obviously remember clearly. I still have a mark where I patted myself on the back.

I think github didn't notify me about the other comments, but

the above are just some ideas - feel free to ignore

was not the reason I did not update. I agree with your clarifications, and thanks.

Edit: github doesn't navigate from that commit to PRs to which it belongs, which is https://github.com/scala/scala/pull/10605

SethTisue commented 6 months ago

in this space: https://github.com/scala/scala/pull/10624