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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IO::Path.dir failure listifies not-fatally on error #2650

Closed
Leont opened this issue Jan 27, 2019 · 29 comments
Closed

IO::Path.dir failure listifies not-fatally on error #2650

Leont opened this issue Jan 27, 2019 · 29 comments
Labels
tests needed Issue is generally resolved but tests were not written yet

Comments

@Leont
Copy link
Contributor

Leont commented Jan 27, 2019

The Problem

IO::Path.dir returns a Failure. This works fine when put directly in a for loop, but when assigned to an array this doesn't quite DWIM.

Expected Behavior

An exception is thrown

Actual Behavior

An array with a single Failure member is created

Steps to Reproduce

my @foo = "/var/spool/cups".IO.dir; dd @foo

Suggested solutions:

Either:

  1. Make failures throw when put in list context.
  2. Throw an exception instead of returning a failure object.

Environment

Rakudo 2018.12

@Leont Leont changed the title IO::Path failure listifies not-fatally on error IO::Path.dir failure listifies not-fatally on error Jan 27, 2019
@lizmat
Copy link
Contributor

lizmat commented Jan 27, 2019

Fixed with 38f4b7b

@lizmat lizmat added the tests needed Issue is generally resolved but tests were not written yet label Jan 27, 2019
@Leont
Copy link
Contributor Author

Leont commented Jan 27, 2019

Thanks!

@AlexDaniel
Copy link
Contributor

Some discussion here: 38f4b7b#commitcomment-32644165

@AlexDaniel AlexDaniel added the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Mar 7, 2019
@AlexDaniel
Copy link
Contributor

Marked as blocker to make sure we make a proper decision on this as soon as possible.

salortiz referenced this issue Mar 8, 2019
In general, if something is returing an Iterable, we throw rather than fail
to prevent exactly the problem that R#2650 describes.  Is spectest clean, so
we weren't too strict about this anyway.
@salortiz
Copy link
Contributor

@Leont, there are simple options if you need the potential returned Failure to thrown, all of them DWIMer IMO:

my @foo = '/var/spool/cups'.IO.dir.self; # Filter the Failure, throwing it.
my Str @foo = '/var/spool/cups'.IO.dir;  # Use a typed Array
use fatal; # Make Failures thrown at return point
my @foo =  '/var/spool/cups'.IO.dir; 

Do you have any reason not to consider acceptable all those options?

@Leont
Copy link
Contributor Author

Leont commented Mar 13, 2019

No one ever wants a list containing a single Failure as a return value. No one. Never.

Do you have any reason not to consider acceptable all those options?

This is not and edge-case. This is a perfectly normal thing to expect to work. "You can't safely store the results of the dir method into an array unless you do X" is not something one can explain to end-users. Not for any value of X, even if there are three of them.

@salortiz
Copy link
Contributor

I agree with you that this isn't an edge case.

I know that, to many ones, the concept of Failure as first-class citizen in Perl 6 represents a paradigmatic shift. Especially to one's accustomed to the try/catch paradigm.

But the fact is that in Perl6 "You can safely store the result of the foo method into any container, but be aware that if the method fails, the returned/stored value will be a Failure, unless you do X" is a perfect thing to explain to end-users, more over if I give them three Xs. (The only use of the fatal pragma in Perl6 is to opt-out the Failure paradigm)

So, why the dir method should be different? Why you expect that listifying a Failure, any Failure, be a error?

I understand that there are lot of cases when the only sensible thing to do is to thrown an Exception. At compile-time or deep inside an asynchronous call, for example. We need to explain the end-user how to CATCH them and the importance of doing so. Both paradigms compose well.

But in this case, a simple method call that can fail, with a Failure at hand the end-user doesn't even need to CATCH, there are many more fluids forms to handle it, all idiomatic Perl 6, for example

my @foo = '/nousch'.IO.dir // (); # @foo ~~ Empty

or

with '/var/spool'.IO.dir -> @foo {
    #Process @foo
} else {
   .fail; # Propagate Failure to caller
}

With all respect, your emphatic "No One, Never", sound to me as an attempt to impose to others your preferred paradigm.

@Leont
Copy link
Contributor Author

Leont commented Mar 14, 2019

So, why the dir method should be different?

The very first sentence of the documentation for dir is "Returns the contents of a directory as a lazy list of IO::Path objects…".

Why you expect that listifying a Failure, any Failure, be a error?

I don't. I expect an exception when dir returns something that isn't usable as "a lazy list of IO::Path objects". Throwing an exception directly is one way to achieve that. Returning a failure type that throws when listified (and maybe turns into a normal Failure object when itemized) is another one.

With all respect, your emphatic "No One, Never", sound to me as an attempt to impose to others your preferred paradigm.

Please do explain? I don't think I'm imposing any paradigm on anyone; I'm defining a requirement, not a solution.

@AlexDaniel
Copy link
Contributor

This is some problem-solving material right there. Putting dir aside, as a user, if I'm calling a built-in function foo, how can I guess if that particular function foo returns a failure or throws?

We need that to be written down somewhere, and we should consistify things that don't follow what we end up writing down.

@salortiz
Copy link
Contributor

Certainly, present documentation, as a work-in-progress and written by volunteers, in many cases lacks required rigour, favoure well established paradigms over new ones and omit important details misleading user expectations. Surprises me that Exceptions, a "Fundamental topic" don't even mention Failure! That is an important part of the problem that should be addressed (It took many, many years for Perl 5's documentation got it actual level).

This discussion is about expectations, so let me explain:

I expect an exception when dir returns something that isn't usable as "a lazy list of IO::Path objects".

To expect that dir or any Routine generate an Exception when can't be able to produce its expected result is perfectly sane.

To expect that an Exception be generated when a Routine returns something that isn't of it documented type, is not. In Perl 6, by design, any Routine, even when declared with a specific strict return type, is allowed to return Nil or a Failure (unless fatalized), and that is explicitly documented.

So, how the possible exception can be propagated to the caller?

Throwing an exception directly is one way to achieve that.

Yes, indeed, but that imposes the user the use of a particular handling paradigm. And, as one of your proposed solutions, to me that situated your in the "what to impose" camp.

Returning a failure type that throws when listified (and maybe turns into a normal Failure object when itemized) is another one.

I don't know what do you mean with a normal Failure object, are you proposing to create different kinds with special semantics?

Wrapping the Exception in a Failure and returning it, is another way, that allows the user to choose his preferred paradigm.

While putting a Failure in some contexts cause its Exception to thrown, to expect the same when "listifying" it, i.e. put it as an element of a generic Iterable, would be against its first-class object status (and that is your other proposal / defined requirement). To expect to thrown when putted in more strict typed containers is well established. With a generic one, the Failure final fate should be decided by the Iterable consumer.

I totally agree with @AlexDaniel than all this most be consensed, clear criteria established, and consistently implemented and documented. In my opinion, for example, specific iterators (strict-typed ones), when in the middle of the iteration, should thrown exceptions, but routines not able to create the Iterable at first instance should not. The dir case falls in the latter.

@Leont
Copy link
Contributor Author

Leont commented Mar 15, 2019

Putting dir aside, as a user, if I'm calling a built-in function foo, how can I guess if that particular function foo returns a failure or throws?

We need that to be written down somewhere, and we should consistify things that don't follow what we end up writing down.

That much is quite clear :-)

@Leont
Copy link
Contributor Author

Leont commented Mar 15, 2019

I don't know what do you mean with a normal Failure object, are you proposing to create different kinds with special semantics?

I think this would do what we both want:

class ListFailure is Failure does Iterable { method iterator { self.self }  }

Changing Failure itself in such a way would inhibit it from being used in a list; to signal that an entry has failed. Which actually points out another problem with using Failure in routines returning iterables: it doesn't allow one to cleanly differentiate between a list that failed and an element that failed.

Wrapping the Exception in a Failure and returning it, is another way, that allows the user to choose his preferred paradigm.

Choice is a good thing, but not at the cost of basic usability.

While putting a Failure in some contexts cause its Exception to thrown, to expect the same when "listifying" it, i.e. put it as an element of a generic Iterable, would be against its first-class object status

I'm not sure I understand you here.

With a generic one, the Failure final fate should be decided by the Iterable consumer.

The final error I got was «Type check failed in binding to parameter '$path'; expected IO::Path but got Failure». The old semantics fail at a distance (pun not intended), and that is a problem.

@lizmat
Copy link
Contributor

lizmat commented Mar 15, 2019

So I added an iteratormethod to Failure with 23fca8f after consultation with @jnthn . So I guess 38f4b7b should be reverted, as many other similar changes?

@lizmat
Copy link
Contributor

lizmat commented Mar 15, 2019

Nope, 38f4b7b cannot be reverted atm as adding an iterator method to something doesn't make it Iterable.

@lizmat
Copy link
Contributor

lizmat commented Mar 15, 2019

Making Failure do the Iterable role, makes one test fail: S02-types/nested_arrays.t, test 10. Will need to sleep on that one.

@Leont
Copy link
Contributor Author

Leont commented Mar 15, 2019

Making Failure do the Iterable role, makes one test fail: S02-types/nested_arrays.t, test 10. Will need to sleep on that one.

There is some spooky action at a distance going on. If the first argument of the throws-like is a Code, it doesn't throw but if it is a string/eval it does.

my @foo = Failure.new("");
throws-like { @foo[0] }, Exception, "Does throw"; #fails
throws-like q{ @foo[0] }, Exception, "Does throw"; #succeeds

Similarly bizarrely these both fail:

my @foo = Failure.new("");
lives-ok { @foo[0] }, "Doesnt throw";
throws-like { @foo[0] }, Exception, "Does throw";

This one does succeed though:

lives-ok { @foo[0]; Nil }, "Doesnt throw";

It would appear Test.pm isn't entirely Failure proof

@Leont
Copy link
Contributor Author

Leont commented Mar 15, 2019

It would appear Test.pm isn't entirely Failure proof

The second case has been clarified, it's the implicit use fatal of the try block in lives-ok. Failures do seem to suffer from strange actions at a distance.

I'm not entirely sure which behavior is desirable, but I'm pretty sure consistency is.

@salortiz
Copy link
Contributor

salortiz commented Mar 16, 2019

Again, seems to me that you are testing your expectations.

use Test;
my @foo = (Failure.new("")); throws-like { @foo[0] }, Exception, "Does throw"

vs

use Test;
my @foo = (Failure.new("")); throws-like { @foo[0].sink }, Exception, "Does throw"

Note the difference.

@Leont
Copy link
Contributor Author

Leont commented Mar 16, 2019

Again, seems to me that you are testing your expectations.

Maybe you should read the test that @lizmat mentioned before making assumptions #notamused.

Note the difference.

I understand fully the difference. I think you completely missed my point.

@Leont
Copy link
Contributor Author

Leont commented Mar 16, 2019

I think you completely missed my point.

Let me rephrase the point:

  1. Why does the quoted form die when the code form doesn't?
    Answer: because EVAL '$failure' throws (if the EVAL itself is in sink context like in throws-like)
  2. Why does EVAL stop dieing when Failure is made an Iterable?

@salortiz
Copy link
Contributor

salortiz commented Mar 16, 2019

Maybe you should read the test that @lizmat mentioned before making assumptions #notamused.

I read all tests involved, and I'm against latest @lizmat's 23fca8f6fb because: a) It doesn't solve this issue, b) favoured one specific paradigm and c) introduces another inconsistency:

for Failure.new { .perl.say } # Throws
for Failure.new, Failure.new { .perl.say } # Succeds

#notamused either.

I understand fully the difference. I think you completely missed my point.

As I think you completely missed mine: A Failure, as a delayed Exception should not throw until the user do something specific.

Why does the quoted form die when the code form doesn't?
Answer: because EVAL '$failure' throws. (if the EVAL itself is in sink context like in throws-like)

No, the quoted form die because it attempt to coerce $failure to Str, one specific and documented case for throwing. BTWthrows-like while waiting for the EVAL result, prevents sinking.

@Leont
Copy link
Contributor Author

Leont commented Mar 16, 2019

Extra data-point: this succeeds:

use Test;
class MyFailure is Failure does Iterable { method iterator { self.self } };
my $foo = MyFailure.new("XXX");
EVAL '$foo';

whereas this fails:

use Test;
class MyFailure is Failure does Iterable { method iterator { self.self } };
my @foo = MyFailure.new("XXX");
EVAL '@foo[0]';

(the difference is the last line)

@salortiz
Copy link
Contributor

salortiz commented Mar 16, 2019

Are you at the REPL, right?, please try:

sink {
class MyFailure is Failure does Iterable { method iterator { note 'called'; self.self } };
my $foo = MyFailure.new("XXX"); say 'alive';
EVAL '$foo';
}

And then

sink {
class MyFailure is Failure does Iterable { method iterator { note 'called'; self.self } };
my @foo = MyFailure.new("XXX"); say 'alive';
EVAL '@foo[0]';
}

(use Test removed b.c. not used)

Edit: Both cases thrown, the first because toplevel sink, the second inside de role because the self, the last line don't even play.

@Leont
Copy link
Contributor Author

Leont commented Mar 16, 2019

D'oh!

@salortiz
Copy link
Contributor

@Leont,
I clearly understand that you want (expect?) that Failure be thrown earlier than in the current implementation, so, let me ask you something: why?
In any case, you will end up needing to put a CATCH block somewhere the handle the Exception. (Forget for a moment the try construction, please)
Honestly, have you tried the available ways to handle Failure?

salortiz referenced this issue Mar 16, 2019
Fixes R#2650.  Makes .iterator throw immediately.  There was some discussion
on #perl6-dev on whether or not to have .iterator just return self, rather
than throwing.  But that would just delay the throwing until the FALLBACK of
Failure would catch the "pull-one", which felt as just wasting extra cycles.

In the past, code was changed for similar situations to throw the exception.
This change should allow us to actually just return the Failure.

This causes some breakage in S03-operators/minmax.t, as the test for failure
is now thrown earlier and the tests don't expect that.
@lizmat
Copy link
Contributor

lizmat commented Mar 16, 2019

Added Failure.list with 4ffb408

@kawaii kawaii removed the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Apr 23, 2019
@AlexDaniel AlexDaniel added the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Jul 16, 2019
@AlexDaniel
Copy link
Contributor

So Raku/roast@75132b8 either needs to be cherry-picked into 6.d-errata, or the rakudo commits need to be reverted.

<lizmat> not too sure about 75132b836be as that ticket is still not resolved and might actually require a problem solving ticket
<AlexDaniel> lizmat: riiight, so what do we do? Revert?
<lizmat> AlexDaniel: good question, I refer to jnthn for a decision

@AlexDaniel AlexDaniel removed the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Jul 16, 2019
@AlexDaniel
Copy link
Contributor

OK, I cherry-picked it. From my point of view, Blin was clean so not much should be affected by this change. I'm a bit surprised that these are the only tests that need tweaking, but maybe the change is not as significant as it seems. Also, it's only the 6.d-errata that needs tweaking.

@raiph
Copy link
Contributor

raiph commented Aug 4, 2021

Having just read through this, Alex's message, and the associated commits, I think this should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests needed Issue is generally resolved but tests were not written yet
Projects
None yet
Development

No branches or pull requests

7 participants