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

[SR-9415] Provide more specific diagnostic when an instance method is invoked on a metatype (or vice versa) #51880

Open
jckarter opened this issue Dec 5, 2018 · 19 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation type checker Area → compiler: Semantic analysis

Comments

@jckarter
Copy link
Member

jckarter commented Dec 5, 2018

Previous ID SR-9415
Radar None
Original Reporter @jckarter
Type Bug
Status In Progress
Resolution
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, DiagnosticsQoI, TypeChecker
Assignee @xedin
Priority Medium

md5: 06d29d9914b6de56ed3956abc969ca22

Issue Description:

https://twitter.com/uliwitness/status/1070339433738747904

'When you call an instance method from a static method, Swift’s error message “‘ParamType’ is not convertible to ‘ReceiverType’” makes no sense, until you remember that instance methods are just regular functions with the ‘self’ pointer inserted as a hidden first parameter.'

We should recognize specially when an instance method is attempted to be invoked on a metatype and diagnose this specifically, instead of giving a generic type mismatch error.

@theblixguy
Copy link
Collaborator

@jckarter Is there an example which reproduces the issue(s)? Also, I was wondering if `FailureDiagnosis::diagnoseGeneralConversionFailure` is the correct place to emit the new diagnostic?

@jckarter
Copy link
Member Author

jckarter commented Dec 5, 2018

@xedin might be able to help you better with implementation specifics. It looks like we do emit a special diagnostic in simple cases:

class Foo {
  static func f() {
    g(0)
  }

  func g(_: Int) {}
}

I asked Uli on Twitter for an example of where he's seeing a less helpful error. Maybe we fail to recognize this situation in some cases.

@jckarter
Copy link
Member Author

jckarter commented Dec 5, 2018

He replied, "It was a function inside an enum. Does that help? Parameter was a class type as well." Indeed, that reproduces the issue for me:

class Bar {}

enum Foo {
  static func f() {
    g(Bar())
  }

  func g(_: Bar) {}
}

Hopefully, that means there's existing code for this diagnostic we can generalize.

@xedin
Copy link
Member

xedin commented Dec 6, 2018

@theblixguy If you want to work on this here is a tip - I think we should be able to "correct" such use and record a fix to diagnose later, that would make it much easier to deal with situations like this. Solver could just pretend that it deals with instance of Foo instead of its metatype while trying to resolve a member, similar to how fixes for invalid unwrapping from this PR #21043

@xedin
Copy link
Member

xedin commented Jan 10, 2019

@theblixguy Just checking - are you working on this one, do you need any help?

@theblixguy
Copy link
Collaborator

@xedin yup, I actually started last night! I went through your PR above to understand how the new approach works. Correct me if I’m wrong, but we need to record that we encountered this error and then remove the error by “fixing” it (i.e we pretend that the error was never there) and continue with the type checking and then in the end, we check if we recorded such an error and emit a diagnostic?

@xedin
Copy link
Member

xedin commented Jan 10, 2019

Yes, exactly, we'd need to either remove or add metatype in this case and if that produces members record a fix and diagnose it. You just need to add ConstraintFix subclass and FailureDiagnostic subclass and connect them, diagnosing itself should be already done.

@xedin
Copy link
Member

xedin commented Jan 12, 2019

@theblixguy One more tip - MemberLookupResult would have `UnviableReason` for all of the possibilities, so if all of them are `UR_TypeMemberOnInstance` or `UR_InstanceMemberOnType` you can just use resulting declarations to form disjunction constraint instead of calling into `simplifyMemberConstraint` again.

@theblixguy
Copy link
Collaborator

Thanks @xedin! At the moment, I have got it working for the above case. In `simplifyMemberConstraint()`, I basically check if the `baseTy` is a `MetatypeType` and then perform a member lookup using the instance type of the base type and if that produces any viable candidates, then I record a fix, assign `instanceTy` to the `baseTy`, and let the function continue (i.e. pretend that its dealing with an instance type instead of a metatype).

Is there another case I need to handle? Do I also need to modify `solveWithNewBaseOrName` and check for `UnviableReason`?

@xedin
Copy link
Member

xedin commented Jan 12, 2019

As I mentioned calling `simplifyMemberConstraint` recursively for that case in unnecessary. Take a look at https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L3828 - you need to add a switch statement there to check if reason of unavailability was `UR_InstanceMemberOnType` or `UR_TypeMemberOnInstance` and and if so - call addOverloadSet with declarations from `result.UnviableCandidates` + record your new fix, this way you can fix it for both - calling instance member on type and type member on instance 🙂

@theblixguy
Copy link
Collaborator

@xedin That if statement doesn't get executed at all for the example code above 🙁 This is because lookup is returning viable candidates so we're calling `addOverloadSet()` & returning `SolutionKind::Solved`.

A way around it would be to modify the check here: https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L3705 and add `&&!shouldAttemptFixes()`.

This makes the if statement execute, but the problem is that if I remove the metatype from a `baseTy` which is a `ClassType` then it causes "unhandled coercion" crash. I guess this could be fixed by a simple check before assigning `baseTy` its instance type.

@theblixguy
Copy link
Collaborator

@xedin

Also, I may have misinterpreted your suggestion - do I need to do this alongside what I mentioned before (i.e. checking if `baseTy` is a `MetatypeType, etc)?

Here's what I have done so far: https://github.com/apple/swift/compare/master...theblixguy:fix/SR-9415?expand=1

@xedin
Copy link
Member

xedin commented Jan 12, 2019

A way around it would be to modify the check here: https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L3705 and add `&&!shouldAttemptFixes()`.

I think that is the way to go! Follow-up code will fall into `if (shouldAttemptFixes())` block, so I think you don't have to modify base type, but rather just use results.

Block of code that you have added to simplifyMemberConstraint to lookup members is unnecessary - wrong base type would still return correct "unviable" results, we just need to pretend that they are "correct" and use them.

Here is what I think we need to add to https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L3828 to make `RemoveMetatype` fix work:

if (llvm::all_of(result.UnviableCandidates, 
                      [&](const std::pair<OverloadChoice, UnviableReason> &e) {
                         return e.second == UR_InstanceMemberOnType;
                      })) {
      auto *fix = RemoveMetatype::create(*this, instanceTy, member, getConstraintLocator(locator));

     if (recordFix(fix)) {
        // The fix was not successful, so bail out
        return SolutionKind::Error;
     }

    addOverloadSet(memberTy, result.UnviableCandidates, useDC, locator,
                                result.getFavoredChoice(), {});
    return SolutionKind::Solved;
}

It would be great if we could extend that check to `e.second == UR_InstanceMemberOnType || e.second == UR_TypeMemberOnInstance` and rename `RemoveMetatype` to something which handles both errors.

@theblixguy
Copy link
Collaborator

@xedin Oh, I actually tried something similar, but the issue is that that line 3828 is never executed for the example code, even after adding ` &&!shouldAttemptFixes` to line 3705. In fact, the entire `if (shouldAttemptFixes()) { ... }` part isn't executed.

Tbh, simply setting `baseTy = baseTy->getMetatypeInstanceType();` is enough to trigger the correct diagnostic, but it leads to a crash if the instance type is a `ClassType`.

@theblixguy
Copy link
Collaborator

@xedin Here's what I have got so far: theblixguy@2d0decb

This block of code is never executed for the example. If I set the `baseTy` to `baseTy->getMetatypeInstanceType()` initially, then I get a crash as I explained above.

Here's the example code:

class Bar {}

enum Foo {
  static func f() {
    g(Bar())
  }

  func g(_: Bar) {}
}

class Baz {
  static func a() {
    h(0)
}
  func h(_: Int) {}
}

class A {
  static func b() {}
  func c() {
    b()
  }
}

@xedin
Copy link
Member

xedin commented Jan 13, 2019

Thank you @theblixguy! I'm going to take a look a bit later tonight or tomorrow, AFK at the moment.

@xedin
Copy link
Member

xedin commented Jan 13, 2019

Ok so looks like `performMemberLookup` doesn't return results the way I expected for the example you've mentioned, instead of adding `g`, `h` and `b` to `UnviableCandidates` it actually does the opposite and constraint system gets built incorrectly as a result because argument conversions get recorded against `Self` portion of the overload type (that's why there is this weird diagnostic that "Bar is not convertible to Foo"). That is made possible by trusting `performMemberLookup` so much that we actually don't create any constraints to make sure that Self type of the overload and given base type do indeed match.

@slavapestov It seems like we need to extend checks in https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L3434L3440 to not only track whether base could have instance methods/members but that base type is also not a metatype if candidate declaration is an instance method/member, that would help diagnostics immensely. WDYT?

@theblixguy
Copy link
Collaborator

@xedin Oh! I changed it to:

if (decl->isInstanceMember()) {
      if (((isa<FuncDecl>(decl) && !hasInstanceMethods)
           || (!isa<FuncDecl>(decl) && !hasInstanceMembers))
           || isMetatype) {
        result.addUnviable(candidate,
                           MemberLookupResult::UR_InstanceMemberOnType);
        return;
      }

and now it seems to work fine.

However, it crashes when `addOverloadSet()` is called with overload choices from unviable candidates:

Assertion failed: (decl), function operator(), file /Users/suyashsrijan/Documents/swift-src/swift/lib/Sema/ConstraintSystem.cpp, line 1404.

Seems like it then calls `tryOptimizeGenericDisjunction()` which has an `assert(decl)` which causes the crash

EDIT: never mind, I had to pass my overload choices as an `ArrayRef`:

`ArrayRef<OverloadChoice>(overloadChoices)`

@theblixguy
Copy link
Collaborator

@xedin I have created a PR: #21830

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation type checker Area → compiler: Semantic analysis
Projects
None yet
Development

No branches or pull requests

3 participants