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

Propagate dependencies through implicit parameters #5601

Merged
merged 1 commit into from Dec 12, 2018

Conversation

Projects
None yet
4 participants
@milessabin
Copy link
Contributor

milessabin commented Dec 12, 2018

Fixes #5427.

If an implicit parameter list is dependent, we must propagate inferred types through the remainder of the parameter list, similarly to how it's done for non-implicit parameter lists in Applications#matchArgs#addTyped.

@smarter
Copy link
Member

smarter left a comment

Thanks for taking care of this!

// done for non-implicit parameter lists in Applications#matchArgs#addTyped.
val formals2 =
if (wtp.isParamDependent && arg.tpe.exists)
formals1 mapconserve {f1 => safeSubstParam(f1, wtp.paramRefs(n), arg.tpe)}

This comment has been minimized.

@smarter

smarter Dec 12, 2018

Member

style nitpick:

Suggested change Beta
formals1 mapconserve {f1 => safeSubstParam(f1, wtp.paramRefs(n), arg.tpe)}
formals1.mapconserve(f1 => safeSubstParam(f1, wtp.paramRefs(n), arg.tpe))

This comment has been minimized.

@milessabin

milessabin Dec 12, 2018

Contributor

Done.

This comment has been minimized.

@smarter

smarter Dec 12, 2018

Member

I suggest pressing "Resolve conversation" below instead of saying "Done.", it reduces the number of emails we get :).

// types through the remainder of the parameter list similarly to how it's
// done for non-implicit parameter lists in Applications#matchArgs#addTyped.
val formals2 =
if (wtp.isParamDependent && arg.tpe.exists)

This comment has been minimized.

@smarter

smarter Dec 12, 2018

Member

I don't think arg.tpe.exists can occur here unlike addTyped, but we should check arg.tpe.isError since in case of implicit failure, the type will be an instance of SearchFailureType which is a subtype of ErrorType:

Suggested change Beta
if (wtp.isParamDependent && arg.tpe.exists)
if (wtp.isParamDependent && !arg.tpe.isError)

I also suggest adding some negative test cases that exercises this code path.

This comment has been minimized.

@milessabin

milessabin Dec 12, 2018

Contributor

We do need exists here. Switching to !arg.tpe.isError results in an assertion for a non-existent getter denotation.

I'll add some negative test cases.

if (wtp.isParamDependent && arg.tpe.exists)
formals1 mapconserve {f1 => safeSubstParam(f1, wtp.paramRefs(n), arg.tpe)}
else formals1
arg :: implicitArgs(formals2, n+1)

This comment has been minimized.

@smarter

smarter Dec 12, 2018

Member
Suggested change Beta
arg :: implicitArgs(formals2, n+1)
arg :: implicitArgs(formals2, n + 1)

This comment has been minimized.

@milessabin

milessabin Dec 12, 2018

Contributor

Done.

@@ -2348,7 +2348,7 @@ class Typer extends Namer
def dummyArg(tp: Type) = untpd.Ident(nme.???).withTypeUnchecked(tp)

def addImplicitArgs(implicit ctx: Context) = {
def implicitArgs(formals: List[Type]): List[Tree] = formals match {
def implicitArgs(formals: List[Type], n: Int): List[Tree] = formals match {

This comment has been minimized.

@smarter

smarter Dec 12, 2018

Member

either give a more explicit name for this parameter or add some documentation for this method (something that would be nice to have anyway :))

This comment has been minimized.

@milessabin

milessabin Dec 12, 2018

Contributor

Renamed to argIndex.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Dec 12, 2018

@smarter incorporated your feedback.

@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Dec 12, 2018

We prefix Dotty issues with i (e.g. i5427.scala). Issues that start with t come from scalac

@smarter

This comment has been minimized.

Copy link
Member

smarter commented Dec 12, 2018

@milessabin Now that github shows diffs when push-forcing commits, I prefer when commits are amended and push-forced to address the feedback of reviewers instead of making new commits just for that (it keeps the history tidy)

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Dec 12, 2018

Okey doke. I'll reprefix the tests to i, squash and force-push.

Propagate dependencies through implicit parameters
Fixes #5427.

If an implicit parameter list is dependent, we must propagate inferred
types through the remainder of the parameter list, similarly to how it's
done for non-implicit parameter lists in
Applications#matchArgs#addTyped.

@milessabin milessabin force-pushed the milessabin:topic/t5427 branch from c598c53 to ccba10c Dec 12, 2018

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Dec 12, 2018

@smarter do you want to do the honours?

@milessabin milessabin merged commit 7c8dee5 into lampepfl:master Dec 12, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@biboudis biboudis added this to the 0.12 Tech Preview milestone Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment