Skip to content

Conversation

@liufengyun
Copy link
Contributor

Fix #4419: liftFun should also update method parameter types

@liufengyun liufengyun changed the title [WIP] Fix #4419: liftFun should also update method parameter types Fix #4419: liftFun should also update method parameter types May 1, 2018
@liufengyun
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented May 1, 2018

performance test scheduled: 1 job(s) in queue, 1 running.

@dottybot
Copy link
Member

dottybot commented May 1, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4428/ to see the changes.

Benchmarks is based on merging with master (f54e7aa)

@odersky odersky merged commit 0e0dab3 into scala:master May 28, 2018
else {
val name = UniqueName.fresh(prefix)
var liftedType = fullyDefinedType(expr.tpe.widen, "lifted expression", expr.pos)
// don't instantiate here, as the type params could be further constrained, see tests/pos/pickleinf.scala
Copy link
Member

Choose a reason for hiding this comment

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

This is very likely to break things. Dotty does not have global type inference, once you create a val, its type cannot be further constrained. (In fact, I remember adding the fullyDefinedType call here to fix a problem, git blame should tell you more)

Copy link
Member

Choose a reason for hiding this comment

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

After looking into it this might be OK because we're more careful about how we deal with signatures containing type variables in recent versions of Dotty, I've found one related issue for which I've opened a PR: #4593

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for looking into it @smarter !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants