[Sema] Improves diagnostic when passing an argument as property of an optional class instance (#57437)

[Rajveer100]

Resolves #57437

As described in the issue, “Type of expression is ambiguous” error is thrown when passing an argument as property of an optional class instance, which further broken down into an argument or contextual type.

[Rajveer100]

A small query, although I have understood certain pattern to calling the getConstraintLocator 's constructors, mainly being a (anchor, path) and (locator), could you differentiate among the different calls for specific cases?

[xedin]

The changes need clang-format but I'm going to run tests to see if anything is broken.

[xedin]

@swift-ci please test

[xedin]

@swift-ci please test

[Rajveer100]

I haven't updated tests for new error diagnostics, will do.

[xedin]

I think some of them are regressions because they loose optionality and there is one test-case in parameterized_existentials.swift where instead of reporting any P<Int> and any P<String> the diagnostic is only about Int and String, which could be okay but it has to be reworded to not speak about return expression directly.

[xedin]

Maybe it's just a matter of moving new code after if (flags.contains(TMF_MatchingGenericArguments)) check.

[Rajveer100]

Maybe it's just a matter of moving new code after if (flags.contains(TMF_MatchingGenericArguments)) check.

Let's try this.

[AnthonyLatsis]

@Rajveer100 Have you tried running that test locally to see if this change counters the regression?

[Rajveer100]

Just to confirm, we are interested in the following file:

• ./test/Constraints/parameterized_existentials.swift

[AnthonyLatsis]

Yes, that should be the one.

[Rajveer100]

Well, the tweak doesn't seem to work:

/test/Constraints/parameterized_existentials.swift:39:32: error: incorrect message found
  return x // expected-error {{cannot convert return expression of type '(any P)?' to return type '(any P<Int>)?'}}
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                               cannot convert return expression of type 'any P' to return type 'any P<Int>'
/test/Constraints/parameterized_existentials.swift:47:32: error: incorrect message found
  return x // expected-error {{cannot convert return expression of type '(any P<Int>)?' to return type '(any P<String>)?'}}
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                               cannot convert return expression of type 'String' to return type 'Int'

[Rajveer100]

What do you recommend, change it to the desired diagnostic message?

[xedin]

I think it should be good to strip the optionals but preserve complete types:

cannot convert return expression of type 'any P<Int>' to return type 'any P<String>'

and produce a note like other cases do:

arguments to generic parameter 'T' ('any P<Int>' and 'any P<String>') are expected to be equal

[Rajveer100]

The notes are already implemented for the generic struct, did you mean any other addition?

struct G<T> {}
// expected-note@-1 {{arguments to generic parameter 'T' ('any P<Int>' and 'any P') are expected to be equal}}
// expected-note@-2 {{arguments to generic parameter 'T' ('any P' and 'any P<Int>') are expected to be equal}}
// expected-note@-3 {{arguments to generic parameter 'T' ('any P<Int>' and 'any P<String>') are expected to be equal}}

[xedin]

We need to make sure that they are actually emitted for this error, these notes might be attached to the declaration of the struct but they should be emitted as part of the conversion diagnostic.

[Rajveer100]

What do you suggest in terms of rewording the second case in the test file as here?
Wouldn't rewording affect diagnostics and tests for other cases?

I have stripped the optionals as discussed, also did you mean to add notes just for this case or also including the reworded case to be done?

[xedin]

For this case the most important thing is to drop the optionals because first of all they match and secondly they are not the problem, the actual problem is that Int is not a String and that's why I'm emphasizing that we need notes and that diagnostics should mention a full type minus the optional bit.

[Rajveer100]

@xedin
How do we convert the basic types to full types, i.e, String to any P<String> and Int to any P<Int>. I have added notes (only in test file, yet to add in codebase) and also reworded the diagnostic, let me know the further changes to be made about the rewording of message.

[LucianoPAlmeida]

IIUC the feedback from @xedin, the discussion about rewording was not about only re-phrasing but add more info about the types involved.

[Rajveer100]

Indeed, notes are to be added, as I said above, I have only added in the test file at the moment. Will add it in the code as well to reflect the same.

[Rajveer100]

@LucianoPAlmeida
For this particular case:

struct S {
  var str: String
}

func test(s: S?) {
  let _: Int? = s?.str

  func f(_: Int?) {}
  f(s?.str)
}

I don't get the expected error, it still gives an ambiguous diagnostic message:

error: type of expression is ambiguous without a type annotation

[LucianoPAlmeida]

Seems like fixes are being correctly added in repairFailures but something is happening in the middle resulting in not a solution formed with those fixes.

[Rajveer100]

Yeah, in diagnoseFailureFor, it's falling through this call:

DE.diagnose(expr->getLoc(), diag::type_of_expression_is_ambiguous)
        .highlight(expr->getSourceRange());

[Rajveer100]

Actually I added breakpoints in repairFailures , it actually never goes through path.back().is<LocatorPathElt::ContextualType>() as a successful check. Hence, it does through the ambiguous diagnosis.