-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
More precise inference for overloaded methods when arguments types align #7631
Conversation
problematic case, not sure why yet:
|
ff18722
to
9094297
Compare
community build run queued: |
I was hoping this case would be fixed by this PR:
But it still fails with this PR:
|
I think that would need a change to specificity rules, off the top of my head. |
Community build fail to investigate:
|
Normally, an argument supplied to an application of an overloaded method is typed without an expected type. We can constrain type inference more when we have only one candidate expected type between all alternatives. So, this adds an exception when all alternatives explicitly specify the same parameter type for an argument (a missing parameter type, due to e.g. arity differences, is taken as `NoType`, thus resorting to no expected type). Also improve a few corner cases with call-by-name arguments, named arguments.
Avoid having to deal with funky prototypes during implicit search.
Addressed the failure in spray-json with 68f3c9f |
This comment has been minimized.
This comment has been minimized.
Why would the |
Good point. I'll investigate why that even compiles. I wonder if we can incorporate repeated params into the notion of being as-specific. You could think of being applicable to a repeated param involving an implicit conversion from case mt @ MethodType(_, _) if bothAreVarargs => checkIsApplicable(mt.paramTypes mapConserve repeatedToSingle) this seems to explain the behavior, but i haven't verified yet |
The original case for both methods being varargs was added in 1b80725 without much explanation. |
Looks like I found the spot. I'll open a separate PR: #7680 |
val result = inferImplicitView(fromNoAnnot, to, tree, context, reportAmbiguous, saveErrors) match { | ||
case fail if fail.isFailure => inferImplicitView(byNameType(fromNoAnnot), to, tree, context, reportAmbiguous, saveErrors) | ||
val toNorm = normalizeProtoForView(to) | ||
val result = inferImplicitView(fromNoAnnot, toNorm, tree, context, reportAmbiguous, saveErrors) match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a sensible / fast path through inferImplicitView
when pt = WildCardType
, i.e. the overloaded signatures don't agree on an effective single formal param type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't follow. If the overloaded signatures don't agree, proto.underlying
will be WildcardType
, so that will be normalizeProtoForView
's result, but a wildcard for the expected type of a view makes little sense. Are you saying we should add another check to avoid calling inferImplicitView
for such a pt
? I suggest that would be a separate issue, since this PR doesn't really add new ways we could get here with a wildcard expected type (the path from adapt
/isCoercible
would not get to their callsite of inferImplicitView
with such a pt.
When an val ptPlugins = pluginsPt(pt, this, tree, mode) // !!! Analyzer plugins (continuations? any others we know of?) will be unready for `OverloadedArgProto`
def retypingOk = (
context.retyping
&& (tree.tpe ne null)
&& (tree.tpe.isErroneous || !(tree.tpe <:< ptPlugins))
)
if (retypingOk) {
tree.setType(null)
if (tree.hasSymbolField) tree.symbol = NoSymbol
}
val alreadyTyped = tree.tpe ne null
val shouldPrint = !alreadyTyped && !phase.erasedTypes
val ptWild = if (mode.inPatternMode)
ptPlugins // scala/bug#5022 don't widen pt for patterns as types flow from it to the case body.
else
dropExistential(ptPlugins) // FIXME: document why this is done. // !!! Do we need to make `dropExistential` deal with `OverloadedArgProto`? From over in your other PR, we also check repeatedness of |
yes, they would have to be updated
I can't think of what this does -- my best guess is that it's for performance / workaround for issues doing subtyping/lub of existentials?
indeed |
I commented out the
Extending the test to inject an
Playing around a bit more reveals a latent bug (I think)
This code doesn't account for the fact that scala/src/compiler/scala/tools/nsc/typechecker/Typers.scala Lines 2939 to 2945 in f72fa7f
|
If we fix that code (naively, for now, by just calling
Then, we can remove the
|
Here's where my experiments led: https://github.com/retronym/scala/tree/topic/existential-pt We really need a canonical way to extract the param/result types from a PF or F type. There is some duplication and divergence between, e.g., This isn't strictly related to this PR, so we can address separately. Let's discuss today. |
Ok, as discussed, let's merge this and leave the existential worries for later :-) |
Followup: #7726 |
Normally, an argument supplied to an application of an overloaded method
is typed without an expected type. We can constrain type inference more
when we have only one candidate expected type between all alternatives.
So, this adds an exception when all alternatives explicitly specify the
same parameter type for an argument (a missing parameter type, due to e.g.
arity differences, is taken as
NoType
, thus resorting to no expected type).Also improve a few corner cases with implicits (views), call-by-name arguments, named arguments.
Note that this also aligns us a bit better with dotty, where constraints are solved lazily,
while being pushed through overload resolution, so that this more precise kind of type inference
falls out "naturally" :-)
Fix scala/bug#11015
@SethTisue let's see how much of the community build I broke :-)