Raku / old-issue-tracker

Tickets from RT
https://github.com/Raku/old-issue-tracker/issues
2 stars 1 forks source link

Deprecate `flatmap` #5989

Open p6rt opened 7 years ago

p6rt commented 7 years ago

Migrated from rt.perl.org#130520 (status was 'open')

Searchable as RT130520$

p6rt commented 7 years ago

From @smls

So, Perl 6 has a built-in `flatmap` routine​:

  .flatmap({ ... })

Show of hands​: Who is reasonably sure, without testing it or looking it up, which of the following four expressions it corresponds to?

  .flat.map({ ... })

  .map({ ... }).flat

  .flat.map({ ... }).flat

  .map({ slip ... })

And if Perl 6 regulars and core developers can't tell, do you think a typical Perl 6 newbie will guess correctly, and that code reviewers will notice when the wrong behavior was assumed?

Note that the four behaviors are the same (or at least easy to confuse) for many simple cases - so it's quite possible to assume the wrong one even after a quick test, if one didn't test the more complicated inputs for which where they do differ.


Second strike​: Whoever wrote the p6doc entry \https://docs.perl6.org/routine/flatmap assumed the wrong behavior as well, and no one else seems to have noticed or cared. (See my initial report at \https://github.com/perl6/doc/issues/851.)

Third strike​: Its behavior isn't actually tested in Roast. The single Roast test that exists for it \https://github.com/perl6/roast/blob/master/S03-metaops/hyper.t#L410 merely tests that it is nodal, and doesn't distinguish between the four possible behaviors listed above.

Fourth strike​: Whereas `deepmap` and `duckmap` differ from plain `map` in how they iterate the input and how often they call the lambda, `flatmap` is the same as `map` in those regards and differs, rather, in how it collects the output. Users who take the naming scheme "deepmap, duckmap, flatmap" as an indication that these are three of a kind, will assume the wrong behavior for `flatmap`.


Is all that confusion (and the needless proliferation of the Perl 6 core setting) worth it, just to save typing a single character in a specific circumstance?

Unless I'm missing something vital, I'd say it's hard to argue that the answer is "yes".

I therefore suggest initiating the deprecation cycle in order to eventually drop of this routine from the Perl 6 language and Rakudo implementation.

p6rt commented 7 years ago

From @zoffixznet

On Thu, 05 Jan 2017 21​:47​:20 -0800, smls75@​gmail.com wrote​:

So, Perl 6 has a built-in `flatmap` routine​:

\.flatmap\(\{ \.\.\. \}\)

Show of hands​: Who is reasonably sure, without testing it or looking it up, which of the following four expressions it corresponds to?

And if Perl 6 regulars and core developers can't tell

What do you mean? I just raised my hand showing I was sure! And there are 57 instances of its use in core code, so seems like I'm not the only one.

Second strike​: Whoever wrote the p6doc entry \https://docs.perl6.org/routine/flatmap assumed the wrong behavior as well, and no one else seems to have noticed or cared. (See my initial report at \https://github.com/perl6/doc/issues/851.)

You seem to have a pretty good idea of what it does. Why not fix the docs and ameliorate the confusion issue instead of letting that doc Issue rot?

p6rt commented 7 years ago

The RT System itself - Status changed from 'new' to 'open'

p6rt commented 7 years ago

From @LLFourn

I took a look through my codebase to see where I have used flatmap. Every place I think I would prefer map({ }).flat on second look.

so +1

LL

On Fri, Jan 6, 2017 at 4​:47 PM Sam S. \perl6\-bugs\-followup@​perl\.org wrote​:

# New Ticket Created by Sam S. # Please include the string​: [perl #​130520] # in the subject line of all future correspondence about this issue. # \<URL​: https://rt-archive.perl.org/perl6/Ticket/Display.html?id=130520 >

So, Perl 6 has a built-in `flatmap` routine​:

\.flatmap\(\{ \.\.\. \}\)

Show of hands​: Who is reasonably sure, without testing it or looking it up, which of the following four expressions it corresponds to?

\.flat\.map\(\{ \.\.\. \}\)

\.map\(\{ \.\.\. \}\)\.flat

\.flat\.map\(\{ \.\.\. \}\)\.flat

\.map\(\{ slip \.\.\. \}\)

And if Perl 6 regulars and core developers can't tell, do you think a typical Perl 6 newbie will guess correctly, and that code reviewers will notice when the wrong behavior was assumed?

Note that the four behaviors are the same (or at least easy to confuse) for many simple cases - so it's quite possible to assume the wrong one even after a quick test, if one didn't test the more complicated inputs for which where they do differ.

---

Second strike​: Whoever wrote the p6doc entry \https://docs.perl6.org/routine/flatmap assumed the wrong behavior as well, and no one else seems to have noticed or cared. (See my initial report at \https://github.com/perl6/doc/issues/851.)

Third strike​: Its behavior isn't actually tested in Roast. The single Roast test that exists for it \https://github.com/perl6/roast/blob/master/S03-metaops/hyper.t#L410 merely tests that it is nodal, and doesn't distinguish between the four possible behaviors listed above.

Fourth strike​: Whereas `deepmap` and `duckmap` differ from plain `map` in how they iterate the input and how often they call the lambda, `flatmap` is the same as `map` in those regards and differs, rather, in how it collects the output. Users who take the naming scheme "deepmap, duckmap, flatmap" as an indication that these are three of a kind, will assume the wrong behavior for `flatmap`.

---

Is all that confusion (and the needless proliferation of the Perl 6 core setting) worth it, just to save typing a single character in a specific circumstance?

Unless I'm missing something vital, I'd say it's hard to argue that the answer is "yes".

I therefore suggest initiating the deprecation cycle in order to eventually drop of this routine from the Perl 6 language and Rakudo implementation.

p6rt commented 7 years ago

From @AlexDaniel

On 2017-01-05 21​:47​:20, smls75@​gmail.com wrote​:

So, Perl 6 has a built-in `flatmap` routine​:

.flatmap({ ... })

Show of hands​: Who is reasonably sure, without testing it or looking it up, which of the following four expressions it corresponds to?

.flat.map({ ... })

.map({ ... }).flat

.flat.map({ ... }).flat

.map({ slip ... })

And if Perl 6 regulars and core developers can't tell, do you think a typical Perl 6 newbie will guess correctly, and that code reviewers will notice when the wrong behavior was assumed?

Note that the four behaviors are the same (or at least easy to confuse) for many simple cases - so it's quite possible to assume the wrong one even after a quick test, if one didn't test the more complicated inputs for which where they do differ.

---

Second strike​: Whoever wrote the p6doc entry \https://docs.perl6.org/routine/flatmap assumed the wrong behavior as well, and no one else seems to have noticed or cared. (See my initial report at \https://github.com/perl6/doc/issues/851.)

Third strike​: Its behavior isn't actually tested in Roast. The single Roast test that exists for it \https://github.com/perl6/roast/blob/master/S03-metaops/hyper.t#L410 merely tests that it is nodal, and doesn't distinguish between the four possible behaviors listed above.

Fourth strike​: Whereas `deepmap` and `duckmap` differ from plain `map` in how they iterate the input and how often they call the lambda, `flatmap` is the same as `map` in those regards and differs, rather, in how it collects the output. Users who take the naming scheme "deepmap, duckmap, flatmap" as an indication that these are three of a kind, will assume the wrong behavior for `flatmap`.

---

Is all that confusion (and the needless proliferation of the Perl 6 core setting) worth it, just to save typing a single character in a specific circumstance?

Unless I'm missing something vital, I'd say it's hard to argue that the answer is "yes".

I therefore suggest initiating the deprecation cycle in order to eventually drop of this routine from the Perl 6 language and Rakudo implementation.

Another thing to note is that flatmap was working differently before the GLR.

\ commit​: pre-glr,HEAD say ((1, 2), \).flatmap(&uc).join('|'); \ AlexDaniel, ¦«pre-glr»​: 1|2|A|B␤¦«HEAD»​: 1 2|A B \ commit​: pre-glr,HEAD say ((1, 2), \).map(&uc).join('|'); \ AlexDaniel, ¦«pre-glr,HEAD»​: 1 2|A B

To me it seems that it slipped through the cracks and nobody thought about it after the GLR.

So yes, if somebody thinks that flatmap should stay, please justify its existence.

Until then, +1.

p6rt commented 7 years ago

From @smls

What do you mean? I just raised my hand showing I was sure! And there are 57 instances of its use in core code, so seems like I'm not the only one.

Okay, I get it, I shouldn't have tried to be cute in how I phrased that.

But do you really think I'm wrong in considering each of the four behaviors listed at the top as something that a reasonable Perl 6 user might expect a routine called `flatmap` to do, and that this presents a trap? Especially if the behavior of the similarly named `deepmap` leads one to guess wrong?

If a feature provides significant value, such a trap can be a fair price. But if `flatmap` has any value at all over chaining `map` and `flat`, it's not apparent.

Can it work marginally faster? If so, that might justify its use in core, but for a user-exposed builtin that seems like a weak rationale. I can't think of any other functionally redundant built-in in Perl 6 that exists purely for some slight performance benefit. The possibilities for a slippery slope would be endless​: Do we also need a `flatzip`, `flatrotor`, `grepmap`, etc? If the Perl 6 language was designed from the ground up for squeezing out every last drop of performance (rather than for convenience/flexibility/extensibility), it would be a very different language.

Or was it introduced to avoid copying the whole list in memory between the `map` and `flat` step, back when `map` used to return a memoizing `List/Parcel` rather than a one-off `Seq` before the GLR?

You seem to have a pretty good idea of what it does. Why not fix the docs and ameliorate the confusion issue

I think the state of its documentation is a symptom of the confusion the routine provokes, not its cause.

Also, I only know what I found out by thoroughly testing the routine with Rakudo.

In order to properly update the docs, waiting for an explanation from the language designers / core developers would be prudent, I think, seeing how it has no meaningful tests in Roast isn't mentioned in the design docs.

p6rt commented 7 years ago

From @zoffixznet

On Wed, 11 Jan 2017 03​:03​:29 -0800, smls75@​gmail.com wrote​:

Okay, I get it, I shouldn't have tried to be cute in how I phrased that.

Right, it's a bit weird to base your argument on a fictional survey with results you made up :)

But do you really think I'm wrong in considering each of the four behaviors listed at the top as something that a reasonable Perl 6 user might expect a routine called `flatmap` to do

In a way, yeah. deepmap doesn't do a combination of .deep and .map called in a particular order. I'd expect .flatmap to do something that *can't* be replicated by putting a dot in its name to make two other method calls. But it does, so IMO .flatmap is kinda useless.

p6rt commented 7 years ago

From @zoffixznet

But do you really think I'm wrong

Nope. I agree it should be tossed.

There's one test in 6.c-errata that merely tests its nodality.

I'd say deprecate it in 6.d and toss it in 6.e. I listed that on the 6.d's TODO​: https://github.com/perl6/6.d-prep/commit/2cc614bde4bc5c69884a63c3ba972dfa680b312c

p6rt commented 7 years ago

From @AlexDaniel

A lot of discussion drifted to this ticket for some reason​: https://github.com/perl6/doc/issues/1428

Let's get ourselves back here.

On 2017-07-31 14​:40​:50, cpan@​zoffix.com wrote​:

But do you really think I'm wrong

Nope. I agree it should be tossed.

There's one test in 6.c-errata that merely tests its nodality.

I'd say deprecate it in 6.d and toss it in 6.e. I listed that on the 6.d's TODO​: https://github.com/perl6/6.d- prep/commit/2cc614bde4bc5c69884a63c3ba972dfa680b312c