-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #1503: be more careful where to insert apply #1522
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
Conversation
felixmulder
left a comment
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.
LGTM aside from the nitpicks on formatting
|
|
||
| /** Useful for diagnsotics: The underlying type if this type is a type proxy, | ||
| /** Useful for diagnsotics: The underlying type if this type is a type proxy, | ||
| * otherwise NoType |
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.
Nitpick: weird indent doesn't match next row
| def isWeakProto: Boolean = false | ||
|
|
||
| /** Overridden in FunProto and PolyProto */ | ||
| def weakenProto: Type = this |
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.
Same on 945-946
|
This now compiles: val foo = () => println("hi")
val bar = () => println("there")
(if (1 == 1) foo else bar)()But this doesn't: def foo() = println("hi")
def bar() = println("there")
(if (1 == 1) foo else bar)()Stack trace: https://gist.github.com/smarter/91f610d4efb8cdaedff16d9b032d346c (With scalac, error: Unit does not take parameters
(if (1 == 1) foo else bar)()
^) |
`apply` nodes should not be inserted in the result parts of a block, if-then-else, match, or try. Instead they should be added to the surrounding statement.
|
@smarter Well spotted. I replaced the PR with a new version which fixes the problem and is simpler than the previous one. |
|
That's similar to a fix we tried with Felix except we gave up when we got type errors and didn't try to introduce something like |
| } | ||
|
|
||
| // ----- Normalizing typerefs over refined types ---------------------------- | ||
| /** If this is a FunProto or PolyProto, WildcardType, otherwise this */ |
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.
- Two extra spaces in the indentation
- This comment should explain the purpose of
notAppliedlike the commit message does
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.
I'm also not entirely sure why using WildcardType does not break type inference in some cases, maybe this could be explained here
| val exprCtx = index(tree.stats) | ||
| val stats1 = typedStats(tree.stats, ctx.owner) | ||
| val expr1 = typedExpr(tree.expr, pt)(exprCtx) | ||
| val ept = if (tree.isInstanceOf[untpd.InfixOpBlock]) pt else pt.notApplied |
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.
Add a comment explaining why InfixOpBlock is treated specially here, also a match would be cleaner and safer than isInstanceOf
smarter
left a comment
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.
LGTM assuming the tests pass
Fixes #1503 and related problems which are presented in i1503.scala.
This was a more fundamental problem to type inference than it appeared at first.
Review by @nicolasstucki or @felixmulder or @smarter.