Skip to content
Permalink
Browse files

Make a Failure throw when it is used as something Iterable

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.
  • Loading branch information...
lizmat committed Mar 15, 2019
1 parent 29878d8 commit 23fca8f6fb45d2a2d694c63862c27a26ec0df252
Showing with 3 additions and 0 deletions.
  1. +3 −0 src/core/Failure.pm6
@@ -39,6 +39,9 @@ my class Failure is Nil {
unless $!handled;
}

# allow Failures to throw when they replace an Iterable
multi method iterator(Failure:D:) { self!throw }

# Marks the Failure has handled (since we're now fatalizing it) and throws.
method !throw(Failure:D:) {
$!handled = 1;

3 comments on commit 23fca8f

@salortiz

This comment has been minimized.

Copy link
Contributor

replied Mar 16, 2019

@lizmat, This alone not even "fix" #2650, it introduces inconsistencies:

for Failure.new { say 'alive' } # Throws
my $a = Failure.new; for $a { say 'alive' }; # Succeeds
for ($ = Failure.new) { say 'alive' }; # Succeeds.
for Failure.new, Failure.new { say 'alive' }; #Succeeds.
for Failure.new.item { say 'alive' }; #Throws
sub f { Failure.new };  for f() { say 'alive' }; # Throws
sub f { Failure.new }; for @(f()) { say 'alive' }; #Succeeds

You know my stance on this matter, so, again, I'm against this change.

@jnthn

This comment has been minimized.

Copy link
Member

replied Mar 16, 2019

for Failure.new { say 'alive' } # Throws
my $a = Failure.new; for $a { say 'alive' }; # Succeeds
for ($ = Failure.new) { say 'alive' }; # Succeeds.

None of these are surprising to me; it's clearly defined that for with a scalar will form a 1-item list, and that list will be iterated. There's nothing specific to Failure going on there, that's just the general semantics of for.

for Failure.new, Failure.new { say 'alive' }; #Succeeds.

This also seems completely reasonable; for iterates lists of things. If one of the things in the list is a Failure, there's nothing wrong with that. In fact, part of the point of Failure was that you could do parallel computations where things go wrong with individual elements, and it doesn't blow up your entire computation.

for Failure.new.item { say 'alive' }; #Throws

This one bothers me; I think it should be equivalent to for ($ = Failure.new) { say 'alive' }. So that'd count as a problem. I'm not sure it's one introduced by this commit, though.

sub f { Failure.new }; for f() { say 'alive' }; # Throws

This is equivalent to for Failure.new { say 'alive' }, which we'd expect.

sub f { Failure.new }; for @(f()) { say 'alive' }; #Succeeds

Failure would also need to have a method list that throws for this to also throw, and that could be reasonable, though the question is if Failure should become Iterable also.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

replied Mar 16, 2019

Adding a method list to Failure makes tests in S03-operators/context-forcers.t die (line 146). FWIW, I think these tests are bogus and have been giving false positives in the past. So I've added that with 4ffb408

Please sign in to comment.
You can’t perform that action at this time.