Reword the missing nullary call error message #30705
Conversation
|
@swift-ci please smoke test |
test/Constraints/fixes.swift
Outdated
| @@ -24,27 +24,27 @@ func f7(_ a: A, _: Int) -> Int { } | |||
| func forgotCall() { | |||
| // Simple cases | |||
| var x: Int | |||
| x = f1 // expected-error{{function produces expected type 'Int'; did you mean to call it with '()'?}}{{9-9=()}} | |||
| x = f1 // expected-error{{expected an expression of type 'Int', but expression has function type returning 'Int'; did you mean to call the function with '()'?}}{{9-9=()}} | |||
There was a problem hiding this comment.
Iis this correct? expected an expression of type 'Int', but expression has function type returning 'Int' is the same type Int ...
There was a problem hiding this comment.
The idea here is that we expect an Int, but what we got was an ()->Int. I thought about phrasing it as expected an expression of type 'Int', but expression has function type '()->Int'; did you mean to call the function with '()'? instead, but this fix also applies to situations where f1 has type (String)->Int, but the String parameter has a default value. Maybe the message should be customized in that situation though.
There was a problem hiding this comment.
Ahh I see, sorry just got confused with the message ... one suggestion maybe is to use but expression is function that returns the same type. Instead of referring the type twice :)
There was a problem hiding this comment.
Maybe better if the expression is a DeclRef expression, tailor it to but expression 'f1' is function that returns the same type ... WDYT?
There was a problem hiding this comment.
I really like this idea! I made changes to customize the message if we have a specific decl we can refer to
owenv
force-pushed
the
reword_missing_nullary_call
branch
from
bff1f83 to
5256566
Compare
| (Type, Type)) | ||
| ERROR(missing_nullary_call_decl,none, | ||
| "expected an expression of type %0, but %1 is a function " | ||
| "returning %0; did you mean to call it with '()'?", |
There was a problem hiding this comment.
How about cannot convert value of type %0 (reference to %1 %2) to expected type %3; did you mean to call %2 with '()'? It seems that having diagnostic mention expression and function doesn't really add much useful information because these are both obvious from the type.
There was a problem hiding this comment.
Or if we choose to just mention the return type: cannot convert reference to %1 %2 returning %0 to expected type %0; did you mean to call %2?
There was a problem hiding this comment.
The problem is that it's not reference being converted but a type of the reference to something. I think it's okay to mention full type just to show that's it's a function with no arguments.
There was a problem hiding this comment.
I don't have a strong opinion, though a reference is still a value, or else there's the same issue with the canonical cannot convert value of type ... to type (but that's just how we phrase it outside the compiler).
I think it's okay to mention full type just to show that's it's a function with no arguments
The dilemma about mentioning the full type is because nullary call syntax can also be used with default arguments. It might be appropriate to customize the message accordingly: either mention just the return type, mention that the function has all parameters defaulted, or both.
| "function produces expected type %0; did you mean to call it with '()'?", | ||
| (Type)) | ||
| "expected an expression of type %0, but expression has function type " | ||
| "%1; did you mean to call the function with '()'?", |
There was a problem hiding this comment.
Nit: I actually like it here. It seems more vague at first blush, but I parse the current sentence has referring to the same subject (the errant expression) by two separate descriptors "the expression" and "the function".
owenv
force-pushed
the
reword_missing_nullary_call
branch
from
5256566 to
b8fdbb9
Compare
|
After thinking about this some more, I think I've found a way to incorporate most of the feedback above. The new messages are: |
|
Sounds good to me! |
|
@swift-ci smoke test |
| !srcFT->getParams().empty()); | ||
| } | ||
|
|
||
| return diags.diagnose(anchor->getLoc(), diag::missing_nullary_call, srcFT, |
There was a problem hiding this comment.
Could you please make it a method on ContextualFailure and use emitDiagnostic instead of DiagnosticEngine directly? I'm trying to change the way diagnostics are emitted in the future - #30967
There was a problem hiding this comment.
I think we'll need to duplicate it in MissingCallFailure too since this error can be emitted from both. It's fairly short though, so probably worth it to take advantage of the upcoming changes
There was a problem hiding this comment.
Could we maybe inherit MissingCallFailure from ContextualFailure?
There was a problem hiding this comment.
Maybe, I'll look into doing that
function produces expected type 'Int'; did you mean to call it with '()'?changes toexpected an expression of type 'Int', but expression has function type '()->Int'; did you mean to call the function with '()'??OR
expected an expression of type 'Int', but 'foo' is a function returning 'Int'; did you mean to call it with '()'?If we have a DeclRefExpr.
I've seen a decent amount of confusion in the past around what the 'expected type' in the current message refers to. The new one is longer, but IMO more clear.