Skip to content

Conversation

@sleepytomcat
Copy link
Contributor

@sleepytomcat sleepytomcat commented May 12, 2020

Fixes ##2579

Expected behavior: Option<>.collect() calls PartialFunction passed as argument only for the value on which this PartialFunction is defined.

Observed behavior: PartialFunction called on values for which it is not defined, resulting in undesired behavior (i.e. unchecked exception thrown).

Example code which fails with arithmetic exception (division by zero):

@Test void collect() {	PartialFunction<Integer, Integer> undefinedFor0 = new PartialFunction<>() {	private static final long serialVersionUID = 1L;	@Override public Integer apply(Integer t) {return 1/t;}	@Override public boolean isDefinedAt(Integer value) {return value != 0;}	};	Option<Integer> zero = Option.some(0);	assertTrue(zero.collect(undefinedFor0).isEmpty()); } 

Using io.vavr:vavr:0.10.2

I've checked the code and this is what I've found:

  1. Optiona<>.collect() eventually calls PartialFunction.lift(), here

    return flatMap(partialFunction.lift());

  2. in turn, PartialFunction.lift() looks like this:

    default Function1<T, Option> lift() {
    return t -> Option.when(isDefinedAt(t), apply(t));
    }

— meaning apply(t) will be always called, no matter if isDefinedAt(t) returns true or false.

return t -> Option.when(isDefinedAt(t), apply(t));

The proposed fix: changing the code to

default Function1<T, Option<R>> lift() { return t -> Option.when(isDefinedAt(t), () -> apply(t)); } 
@sleepytomcat
Copy link
Contributor Author

I'm fairly new to open source collaboration and pull requests. @mincong-h , can you please guide what are my next steps to merge the changes to trunk?
Thank you!

@mincong-h
Copy link
Member

@sleepytomcat , you need to wait until someone who has write permission to approve and merge it for you. My approval is useless because I don't have write permission thus cannot merge your PR. I just came across your PR by curiosity... Usually @danieldietrich will review.

Copy link
Contributor

@danieldietrich danieldietrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sleepytomcat, looks like I'm the sleepy tomcat 😅 Sorry for letting you wait a bit.
Your fix looks great, thanks for reporting the bug!

@danieldietrich danieldietrich added this to the v1.0.0 milestone May 13, 2020
@danieldietrich danieldietrich merged commit 5a01397 into vavr-io:master May 13, 2020
danieldietrich pushed a commit that referenced this pull request Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants