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

Option<>.collect() not to call PartialFunction on args where it is not defined #2580

Merged
merged 1 commit into from May 13, 2020
Merged

Option<>.collect() not to call PartialFunction on args where it is not defined #2580

merged 1 commit into from May 13, 2020

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
Member

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants