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

No ambiguity exists when a type has an implicit conversion to RequestContent #3587

Closed
pshao25 opened this issue Jul 13, 2023 · 4 comments · Fixed by #3616
Closed

No ambiguity exists when a type has an implicit conversion to RequestContent #3587

pshao25 opened this issue Jul 13, 2023 · 4 comments · Fixed by #3616

Comments

@pshao25
Copy link
Member

pshao25 commented Jul 13, 2023

Background:

We will make protocol method an overload (make the RequestContext required) if the input body is optional to avoid ambiguous call. For example:

op get(@body optional?: Model) : void;

We will generate

public virtual Response Get(Model optional = null, CancellationToken cancellationToken = default);
public virtual Response Get(RequestContent content, RequestContext context);

The RequestContext is required otherwise there will be ambiguous call Get(null).

Problem:

We are introducing such kind of implicit conversion from Model to RequestContent:

public static implicit operator RequestContent(Model model)
{
    // Some logic
}

After this change, below code has no ambiguity (attention: RequestContext is back to optional). We could call Get(null) and it goes to convenience method.

public virtual Response Get(Model optional = null, CancellationToken cancellationToken = default);
public virtual Response Get(RequestContent content, RequestContext context = null);

From this document,

Given an implicit conversion C₁ that converts from an expression E to a type T₁, and an implicit conversion C₂ that converts from an expression E to a type T₂, C₁ is a better conversion than C₂ if one of the following holds:
E exactly matches both or neither of T₁ and T₂, and T₁ is a better conversion target than T₂ (§12.6.4.7)

and this document,

Given two types T₁ and T₂, T₁ is a better conversion target than T₂ if one of the following holds:
An implicit conversion from T₁ to T₂ exists and no implicit conversion from T₂ to T₁ exists

It seems to be expected. Try code here.

Pay attention to that we already have implicit conversion from string/BinaryData/DynamicData to RequestContent. So same logic applies to these types.

Action:

We might revert the previous logic because we don't need to generate overload for an optional body after that change.

@lirenhe
Copy link
Member

lirenhe commented Jul 17, 2023

I understand the backend logic that could resolve the ambiguity call, but just wondering whether this is obvious from end users' prospective.

To avoid the misuse of Get(null), is it better to keep them separate?

cc @m-nash

@pshao25
Copy link
Member Author

pshao25 commented Jul 17, 2023

I think we should be clear about the principles of when to make a protocol method overload. Should we keep RequestContext context = null as possible as we can (in this case we should revert previous logic), or as long as there is no ambiguous call we could do either way (in this case we could keep current behavior).

whether this is obvious from end users' prospective.

Of course, we change the signature.

To avoid the misuse of Get(null), is it better to keep them separate?

Why would Get(null) a misuse? I think it totally makes sense.

@lirenhe
Copy link
Member

lirenhe commented Jul 17, 2023

I think we should be clear about the principles of when to make a protocol method overload. Should we keep RequestContext context = null as possible as we can (in this case we should revert previous logic), or as long as there is no ambiguous call we could do either way (in this case we could keep current behavior).

whether this is obvious from end users' prospective.

Of course, we change the signature.

To avoid the misuse of Get(null), is it better to keep them separate?

Why would Get(null) a misuse? I think it totally makes sense.

From our guideline:

Approachable

  • We are experts in the supported technologies so our customers, the developers, don’t have to be.

@pshao25
Copy link
Member Author

pshao25 commented Jul 18, 2023

The only scenarios we need to consider the ambiguity is: all required parameters are set and all the optional parameters are not pass in.

For each pair of protocol and convenience, we will try to call with only required parameters provided. If the compilation passes, then we keep the RequestContext optional, otherwise change it to required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants