Implement creator applications #6084
Conversation
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
odersky
force-pushed
the
add-creator-applications
branch
from
b3b1bd9 to
1747921
Compare
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6084/ to see the changes. Benchmarks is based on merging with master (a6bab2c) |
odersky
force-pushed
the
add-creator-applications
branch
from
1747921 to
7bd1c42
Compare
|
Blocked on the same issue as #6097. |
odersky
force-pushed
the
add-creator-applications
branch
from
7bd1c42 to
a3f1d97
Compare
|
There are several fundamental improvements to typechecking prompted by this PR. To keep focus, the PR is best reviewed commit by commit. |
odersky
force-pushed
the
add-creator-applications
branch
2 times, most recently
from
042bd3e to
c18ba91
Compare
| val conflicting = c.domainLambdas.find(tl => | ||
| this.contains(tl) && hasConflictingTypeVarsFor(tl)) | ||
| conflicting match { | ||
| case Some(tl) => ensureNotConflicting(c.rename(tl)) |
There was a problem hiding this comment.
I'm not completely sold on after-the-fact renaming being the best approach. What if instead TypeLambda had a var inUse: Boolean set to true when added to a constraint and set back to false when it's removed from the constraint ? Then we can make sure to use different TypeLambdas in different constraints live at the same time, it's a slight memory increase but should be much less expensive than doing deep replacements when merging constraints.
There was a problem hiding this comment.
Yes, possibly. I need to think this over.
There was a problem hiding this comment.
No, in fact that would not work. The problem is that we create constraints and then forget about them. So a lambda might never be removed from a constraint.
odersky
force-pushed
the
add-creator-applications
branch
from
c18ba91 to
dd6495d
Compare
The purpose of this change is that we should always report a thrown TypeError
and not hide it by backtracking into some other alternative.
Wihthout this change test i4564.scala fails after applying the creator applications
commit, since some errors about recursive methods lacking a result type are hidden
by using a constructor instead. Ehere is an example:
```
object NoClashNoSig {
private def apply(x: Boolean) = if (x) NoClashNoSig(1) else ??? // error: overloaded method apply needs result type
}
case class NoClashNoSig private(x: Int)
```
With introduction of creator applications, `NoClashSig` in `apply` would call the constructor,
effectively hiding the "overloaded method apply needs result type" error. This is not what
we want.
Without this change, tests/neg/applydynamic_sip.scala fails once creator applications are introduced.
Creator methods create new sitation where we need to integrate type arguments of the prototype in a term. Create a superclass IntegratedTypeArgs of ExtMethodApply to cover both that and extension methods.
Now includes stack of ids of all nested typer states.
When merging two constraints we can end up in a situation where a type lambda is associated with different type variables in the two constraint. This happened when compiling concat.scala after introducing an additional tryEither for typechecking the function part of an application. That caused a constraint merge of two constraints over logically separate instances of `++`, which shared the same type lambda for `++` but associated it with different type variables. This situation can arise since for efficiency we do not always clone a type lambda before adding it to a constraint. The correct way to deal with the situation is to clone the type lambda with the conflicting TypeVars in one of the constraints before proceeding with the merge.
Also, some refactorings to shorten and simplify
odersky
force-pushed
the
add-creator-applications
branch
from
1aecd2f to
52f2e63
Compare
|
@smarter Thanks for the hint about the submodules! I'm done with fixing review comments. |
ghost
removed
the
| of a class, even if there is no apply method implemented. Example: | ||
| ```scala | ||
| class StringBuilder(s: String) { | ||
| def this() = this(s) |
There was a problem hiding this comment.
Is this(s) a typo here? The compiler reasonably says that s is not accessible from the constructor.
Another attempt to implement creator applications that closely follows the preSIP.