Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Sema] Produce expected diagnostic for invalid operator usage in loop #80042

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rajveer100
Copy link
Contributor

Resolves #79999

Snippet:

func foo(_ a: Int) -> [Int] { [] }
for _ in foo(-) {}

@Rajveer100 Rajveer100 requested review from hborla and xedin as code owners March 16, 2025 12:05
@Rajveer100
Copy link
Contributor Author

This may be totally incorrect, let me know the appropriate place/direction for this.

@jameesbrown
Copy link
Contributor

While the current diagnostic is expected, I think we could improve it to be more consistent with other argument mismatches. For example, when passing an argument of the wrong type to a function, we get:

func foo(_ str: String) {}
foo(10) // Cannot convert value of type 'Int' to expected argument type 'String'

A similar diagnostic should be produced when passing an operator as an argument in a context where it is not valid, e.g.,

foo(+) // Cannot convert operator '+' to expected argument type 'String'  

Try tracing where and how AllowArgumentMismatch is created and how this results in emitting a diagnostic in ArgumentMismatchFailure::diagnoseAsError(), as that should give you a good intuition for where the missing piece might be.

Additionally, we should add a test case. Let me know if this seems like a sensible direction.

//
// func example(_ a: Int) -> [Int] { [] }
// for _ in foo(-) {}
for (auto &arg: sequence->getArgs()->getArgExprs()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but this is not the right place for this fix. When you look at the -debug-constraints output for foo(-) expression, you'd notice that we end up with multiple solutions that all have AllowArgumentMismatch fix. The ConstraintFix has a special method for cases like this diagnoseForAmbiguity, overload needs to be added to AllowArgumentMismatch and the diagnostic logic need to be added there. There is also a question of what actual diagnostic we need to provide here because each solution would have different type on left-hand side but the same on the right, so if there is a single error here it would have to be general enough to cover all of the cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my thought process:

Seeing the overload choices, is there a way to retrieve or check if the specific solution points at the argument parameter, like in the example one of the choices:

locator@0x12d1c6ca0 [[email protected]:5:14] with _Concurrency.(file).Instant extension.- as -: (SuspendingClock.Instant, SuspendingClock.Instant) -> Duration

It points at 5:14 while rest are at either the return/decl or other parts of the function expression. A little unsure about the usage of the API.

Also, just to be sure, you meant to add a method as such:

bool AllowArgumentMismatch::diagnoseForAmbiguity(CommonFixesArray commonFixes) const {
  const auto *locator = getLocator();
  for (auto &entry: commonFixes) {
    auto &solution = entry.first;
    
  }
}

And then here we go over the choices and try to diagnose if I understand correctly. I was trying to look at the other cases where obviously each have a different approach depending on the type of ambiguity for example contextual/relabel/invalid member etc.

I believe we could throw the argument mismatch diagnostic right which should cover the cases? I could be missing some though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be worth for you to take a look at diagnoseAmbiguityWithFixes, this is the method that analyzes all of the fixes across solutions and tries to group them and invoke appropriate methods. There is currently no diagnoseForAmbiguity override on AllowArgumentMismatch because it's non-trivial to implement - the fixes could point to different argument/parameter pairs or the same pair but either argument or parameter might have different type (i.e. your example with foo(-) all solutions point to the same pair and parameter types are all the same but argument types are all different).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I understand the flow now, each of the types return many different types due to the overload, like Int, Int128, Duration. Maybe we could first go over the overload choices and then filter for OverloadedDeclRef, then we check if this is some sort of built-in like +, -, etc or if it's a custom one like **.

I think we can just throw a generalised error like:

error: cannot convert overload type of type '<string_name>' to expected argument type 'Int'

What do you think/suggest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that's it's not only about applied operators or functions in general:

func test() -> Int {}
func test() -> String {}

func other(_: Int32) {}

other(test())

But we can diagnose the other way around pretty well:

func test(_: Int) {}
func test(_: String) {}

func compute() -> Double { 0 }

test(compute())

Maybe other(test()) should be diagnosed as ambiguous use of 'test()' with a note for each mismatch...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These examples don't seem to be an issue based on the last change I made:

debug-79999.swift:13:7: error: cannot convert value of type 'Int' to expected argument type 'Int32'
11 | func other(_: Int32) {}
12 | 
13 | other(test())
   |       `- error: cannot convert value of type 'Int' to expected argument type 'Int32'
14 | 
debug-79999.swift:20:1: error: no exact matches in call to global function 'test'
15 | func test(_: Int) {}
   |      `- note: candidate expects value of type 'Int' for parameter #1 (got 'Double')
16 | func test(_: String) {}
   |      `- note: candidate expects value of type 'String' for parameter #1 (got 'Double')
17 | 
18 | func compute() -> Double { 0 }
19 | 
20 | test(compute())
   | `- error: no exact matches in call to global function 'test'
21 | 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot produce cannot convert value of type 'Int' to expected argument type 'Int32' because test has 2 overloads and they are failing. The diagnostic makes it seem like there is only one viable overload test here which is incorrect.

Resolves swiftlang#79999

Snippet:

```
func foo(_ a: Int) -> [Int] { [] }
for _ in foo(-) {}
```
@Rajveer100 Rajveer100 force-pushed the invalid-operator-loop branch from 9657605 to 426965d Compare March 20, 2025 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing diagnostic for invalid operator usage in for loop
3 participants