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

Ambiguous call under spread scenarios #3585

Closed
pshao25 opened this issue Jul 13, 2023 · 2 comments
Closed

Ambiguous call under spread scenarios #3585

pshao25 opened this issue Jul 13, 2023 · 2 comments
Assignees
Labels

Comments

@pshao25
Copy link
Member

pshao25 commented Jul 13, 2023

Typespec:

alias ThingAlias = {
  @doc("age of the Thing")
  age?: int32;
  @doc("name of the Thing")
  name?: string;
};
op spreadAlias( ...ThingAlias ): void;

Actual:

public virtual async Task<Response> SpreadAliasAsync(int? age = null, string name = null, CancellationToken cancellationToken = default);
public virtual async Task<Response> SpreadAliasAsync(RequestContent content, RequestContext context = null)
{
    Argument.AssertNotNull(content, nameof(content));
}

This call is an ambiguity: SpreadAliasAsync(null). This will not happen to string, see details in #3587.

Expected:
In the normal case like

public virtual async Task<Response> SpreadAliasAsync(Model model, CancellationToken cancellationToken = default)
{
    Argument.AssertNotNull(model, nameof(model));
}
public virtual async Task<Response> SpreadAliasAsync(RequestContent content, RequestContext context = null)
{
    Argument.AssertNotNull(content, nameof(content));
}

Even there is an ambiguous call SpreadAliasAsync(null), we still leave RequestContext optional, because SpreadAliasAsync(null) makes no sense.

What makes this scenario special from others is SpreadAliasAsync(null) makes sense to convenience method. So I think the protocol method should change when the first parameter in convenience method is not string.

Solution:

public virtual async Task<Response> SpreadAliasAsync(RequestContent content, RequestContext context)
{
    Argument.AssertNotNull(content, nameof(content));
}

Update:

According to #3587 (comment), we don't take SpreadAliasAsync(null) into consideration, so leave RequestContext context = null.

@AlexanderSher
Copy link
Contributor

This will not happen to string, see details in #3587.

RequestContent has an implicit cast from string, but no implicit cast to string, so during cast resolution string parameter wins.

@pshao25
Copy link
Member Author

pshao25 commented Jul 19, 2023

In the update section of description, this is by design.

@pshao25 pshao25 closed this as completed Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants