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

Fix unhelpful type error #55

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Fix unhelpful type error #55

merged 3 commits into from
Oct 24, 2022

Conversation

bbrk24
Copy link
Contributor

@bbrk24 bbrk24 commented Oct 13, 2022

Issue Link

Fixes #54

Jira issue: THOS-5

Overview of Changes

This PR fixes type resolution errors. Basically, what would happen in a situation like this:

protocol P {}
class C: P {
    init(a: Int, c: String) {}
}

func setup() {
    Factory.register { // [1]
        Service(.transient, P.self) { r in
            C(
                a: r.resolve(),
                b: r.resolve(), // [2]
                c: r.resolve()
            )
        }
    }
}

is that Swift would be unable to figure out the generic type argument to r.resolve() at [2], since the initializer for C doesn't take a b. Because it can't determine the return type, swift would report "type of expression is ambiguous without more context", completely ignoring the fact that it's not meant to be there. Due to the result builder transform, the error would be reported at [1] instead of [2].

Adding an overload that returns Any works around this problem: if it can't figure out how to call resolve<T>() -> T, it tries the other overload, resolve() -> Any. This allows it to complete type-checking and realize that b shouldn't even be there.

Anything you want to highlight?

I marked the overload as @available(*, unavailable). This has two consequences:

  1. In a line like var x: Any = Factory.shared.resolve(), this is inferred as resolve<Any>() instead of the new overload.
  2. A line with no types at all, such as var x = Factory.shared.resolve(), still emits an error:

Screen Shot 2022-10-13 at 9 56 12 AM

However, it has the downside that the new overload doesn't show up in the documentation. Using deprecated instead of unavailable makes it show up in the documentation, but also makes point 1 no longer true, and downgrades point 2 from an error to a warning.

Test Plan

Paste the code snippet above (or any equivalent) into a project that imports Dependiject. You should see the error at [2] instead of [1].

Comment on lines +37 to +43
/// This overload fixes compiler errors.
///
/// To explicitly specify the type, use ``resolve(_:name:)`` or ``resolve(_:)`` instead.
///
/// Without this overload, adding extraneous arguments to an `init` inside of the callback
/// passed to ``Service/init(_:_:name:_:)`` does not error at the `init` call, but at the top
/// of the ``Factory/register(builder:)`` block.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding a note mentioning that this overload doesn't appear in the documentation just incase someone is confused why it isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense to me.

@bbrk24 bbrk24 requested a review from wboyd600 October 17, 2022 20:32
@bbrk24 bbrk24 marked this pull request as ready for review October 17, 2022 20:32
Dependiject/Resolver.swift Outdated Show resolved Hide resolved
@@ -19,7 +19,7 @@ public extension Resolver {
/// Get an unnamed dependency of the specified type.
/// - Returns: The unnamed instance of the type.
/// - Important: There must be an unnamed registration of the specified type. When resolving a
/// named dependency, use ``resolve(_:name:)`` or ``resolve(name:)`` instead.
/// named dependency, use ``resolve(_:name:)`` or ``Resolver/resolve(name:)`` instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

did this link not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to, but for some reason when I added the overload it didn't anymore, and I had to specify the base type.

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.

Unhelpful error with extra arguments to an init
4 participants