Moved Ambiguous Overload error in Typer to case class #2354
Conversation
| s"""|There are ${alts.length} methods that could be referenced as the compiler knows too little about the expected type. | ||
| |You may specify the expected type e.g. by | ||
| |- assigning it to a value with a specified type, or | ||
| |- adding a type ascription as in (MyClass.myMethod: String => Int) |
There was a problem hiding this comment.
It looks like the type ascription example should be myObject.myMethod: String => Int
There was a problem hiding this comment.
Yes, thank you. Preferrable I'd show the method in question, but as far as I know there is no easy way right now to print out the type with =>-syntax.
There was a problem hiding this comment.
It looks like most of the functionality from ErrorReporting could be consolidated with the messages or at least moved to the reporting package. Not sure we should do this at this juncture - but if there are more tests that require Errors then perhaps it would be better to give it an apt name and put it in the reporting package.
Perhaps @odersky has some further opinions? It's his code after all 😄
Other than the comments in this review LGTM
| s"""|There are ${alts.length} methods that could be referenced as the compiler knows too little about the expected type. | ||
| |You may specify the expected type e.g. by | ||
| |- assigning it to a value with a specified type, or | ||
| |- adding a type ascription as in `(instance.myMethod: String => Int)` |
There was a problem hiding this comment.
Highlight the code example?
There was a problem hiding this comment.
Any sight of supporting a type.show-variant with notation as in source code?
There was a problem hiding this comment.
Types are Showable in Dotty as long as you have an implicit Context in scope, you can do tpe.show. If that's not what you meant, then please elaborate :)
There was a problem hiding this comment.
tpe.show gives (s: String)Int, but I'd like to get (String) => Int for better cut-and-paste fidelity.
| override def kind: String = "Reference" | ||
|
|
||
| override def explanation: String = | ||
| s"""|There are ${alts.length} methods that could be referenced as the compiler knows too little about the expected type. |
There was a problem hiding this comment.
This line is > 100 columns. Eventually, maybe we should have the hl interpolator wrap the lines.
There was a problem hiding this comment.
hl will need to start wrapping at some stage as we put in dynamic amount of text. I will wrap this one.
There was a problem hiding this comment.
Yeah, you're completely right. There are some cases where we don't want it to wrap. For instance on lines beginning with -, +, *, , and so on... maybe we could just principle it on wrapping over 80 characters until it hits two newlines or one of the disallowed symbols. It would also need to take into account ANSI escape codes for the colors.
|
|
||
| private def all = if (alts.length == 2) "both" else "all" | ||
|
|
||
| override def msg: String = |
There was a problem hiding this comment.
These shouldn't be override, I'm guessing you looked at the one above and just assumed that we write it like this now. Could you change both? 😄
There was a problem hiding this comment.
Yes. Started off from the wrong example. Will fix.
|
PS - commit messages should be in the imperative form. E.g. "Add X" over "Added X" |
|
LGTM, but needs a rebase |
ennru
force-pushed
the
ennru_AmbiguousOverload
branch
from
1a6cf58 to
458ee55
Compare
|
@felixmulder I just noticed dottybot gives the opposite advice you do over added vs add here https://github.com/lampepfl/dotty/blob/e4e481034a309d7e23c0b6b2abb87795b2d987bc/bot/src/dotty/tools/bot/PullRequestService.scala#L270 |
|
@ShaneDelmore - it should be the imperative mood, e.g "add" over "added" :) So the examples are swapped - would you mind unswapping? :) |
@felixmulder Couldn't decide on best solution how
typer.ErrorReporting.Errorsmethods should be used from error case class. There are more errors to move that will need it.