Add Type.tambig and make ready to use. #7503
Conversation
|
Thanks for your pull request, @ibuclaw! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#7503" |
|
I don't see in this PR any examples of what an ambiguous type may be, and according to the code no type is ever set to being ambiguous. I can't evaluate what the case for this is. |
|
See the referenced PR. |
|
@ibuclaw Would you mind accommodating @WalterBright and adding one of #2130's other commits so he can evaluate this further? IMO this is a very important PR for a couple of reasons, and if this is the right approach (sorry I'm not skilled enough to weigh in on that myself) I'd very much like to move it forward. |
What's wrong with simply opening #2130 and looking at the bugs it fixes? (also I rebased this PR) |
|
ping @WalterBright |
| { | ||
| // Even if is(typeof(func)) is OK, is(typeof(func) Sym) could be NG | ||
| if (e.id) | ||
| e.error("cannot take ambiguous type"); |
There was a problem hiding this comment.
What does "cannot take" mean?
There was a problem hiding this comment.
Also, should set result to Type.terror
| @@ -1156,6 +1160,14 @@ extern (C++) abstract class Type : RootObject | |||
| return true; | |||
| } | |||
|
|
|||
| /************************** | |||
| * Returns true if T was resolved ambiguously. | |||
There was a problem hiding this comment.
- Use Ddoc style comments.
- What does "ambiguous" mean in this context?
src/dmd/mtype.d
Outdated
| if (t.isAmbiguous()) | ||
| { | ||
| //printf("typeof exp = %s -> t = %s\n", exp.toChars(), t.toChars()); | ||
| error(loc, "expression `%s` has ambiguous type", exp.toChars()); |
There was a problem hiding this comment.
Need a much more expressive error message. I doubt the user has any idea what an "ambiguous" type is, and the message doesn't say what the type is. If reasonable, it should also suggest corrective action.
| tf.next = rs.exp.type; | ||
| if (rs.exp.type.isAmbiguous()) | ||
| { | ||
| rs.error("cannot infer return type from ambiguous expression `%s`", rs.exp.toChars()); |
There was a problem hiding this comment.
need more expressive error message, see my other comment
| @@ -5334,6 +5357,12 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor | |||
| Type t1b = exp.e1.type.toBasetype(); | |||
| Type tob = exp.to.toBasetype(); | |||
|
|
|||
| if (t1b.isAmbiguous() && exp.e1.implicitConvTo(exp.to) <= MATCH.nomatch) | |||
| { | |||
| exp.error("cannot cast ambiguous expression `%s` to `%s`", exp.e1.toChars(), exp.to.toChars()); | |||
There was a problem hiding this comment.
The types of the two expressions need to be displayed, too, as well as what it means for an expression to be "ambiguous". (Especially because ambiguous expressions are used here, while elsewhere it's ambiguous types.)
| // Even if is(typeof(func)) is OK, is(typeof(func) Sym) could be NG | ||
| if (e.id) | ||
| e.error("cannot take ambiguous type"); | ||
| if (e.id || e.tspec || (e.tok2 != TOK.reserved && e.tok2 != TOK.function_)) |
There was a problem hiding this comment.
errors and no error message?
First commit of #2130. I've checked the PR, and fixing all mentioned issues (1983, 7322, 7549, 8868, 9027) depends on this first if we are to take on the original patch.
I've replaced the handling of
isAmbiguousin dmangle and hdrgen with asserts, as I do not know whether they are reachable. See that the type is only used for generating errors, I would suspect not.