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

Reapply keyword params #101499

Merged
merged 2 commits into from
May 20, 2024
Merged

Reapply keyword params #101499

merged 2 commits into from
May 20, 2024

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Apr 24, 2024

Closes #101261.
Closes #77873.

Continuation of #101308.

@jozkee jozkee added this to the 9.0.0 milestone Apr 24, 2024
@jozkee jozkee requested a review from stephentoub April 24, 2024 15:48
@jozkee jozkee self-assigned this Apr 24, 2024
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@stephentoub
Copy link
Member

We won't be able to merge this until the C# compiler is updated to guard overload resolution with params span on LangVersion.

@stephentoub stephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 14, 2024
@stephentoub
Copy link
Member

/azp run

Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@stephentoub stephentoub reopened this May 20, 2024
@stephentoub stephentoub merged commit 5f067ce into dotnet:main May 20, 2024
146 checks passed
@jozkee jozkee deleted the paramsspan-keyword branch May 22, 2024 16:53
@jozkee jozkee added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels May 22, 2024
Copy link
Contributor

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@stephentoub
Copy link
Member

@jozkee, why did you mark this as breaking? What is breaking about it?

@jozkee
Copy link
Member Author

jozkee commented May 22, 2024

source breaking?

@stephentoub
Copy link
Member

Can you elaborate?

@jozkee
Copy link
Member Author

jozkee commented May 22, 2024

There's a case where a collection expression can't resolve between ROS<string> and ROS<object>, see dotnet/sdk#41089 (comment). It may be a compiler bug, though.

Additionally, do we not want to document cases of types with conditions similar to StringValues?

@cston
Copy link
Member

cston commented May 22, 2024

There's a case where a collection expression can't resolve between ROS<string> and ROS<object>, see dotnet/sdk#41089 (comment). It may be a compiler bug, though.

Currently, the better conversion rules for collection expressions treat this case as ambiguous.

Perhaps there should be an additional rule to prefer System.ReadOnlySpan<E₁> over System.ReadOnlySpan<E₂> if an implicit conversion exists from E₁ to E₂.

@stephentoub
Copy link
Member

There's a case where a collection expression can't resolve between ROS<string> and ROS<object>, see dotnet/sdk#41089 (comment). It may be a compiler bug, though.

Currently, the better conversion rules for collection expressions treat this case as ambiguous.

Perhaps there should be an additional rule to prefer System.ReadOnlySpan<E₁> over System.ReadOnlySpan<E₂> if an implicit conversion exists from E₁ to E₂.

I thought that was part of the first-class span support. Is it not, @333fred?

@cston
Copy link
Member

cston commented May 22, 2024

Perhaps there should be an additional rule to prefer System.ReadOnlySpan<E₁> over System.ReadOnlySpan<E₂> if an implicit conversion exists from E₁ to E₂.

I thought that was part of the first-class span support. Is it not, @333fred?

Unfortunately, implicit conversions from first-class span support may not be sufficient for a collection expression argument since implicit conversions are only considered in better conversion when the types are not span types.

  • E is a collection expression and one of the following holds:
    • ...
    • T₁ is not a span_type, and T₂ is not a span_type, and an implicit conversion exists from T₁ to T₂

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@cston
Copy link
Member

cston commented Jun 7, 2024

Currently, the better conversion rules for collection expressions treat this case as ambiguous.

This is tracked by dotnet/roslyn#73857.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 8, 2024
@jozkee
Copy link
Member Author

jozkee commented Sep 27, 2024

Note for breaking change doc: If this ship as is, it should be pointed out that string.Trim*(params ROS<char>) will hide extension methods which usually match the whole sequence while the one added here matches any of the characters.

See #102822 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Memory breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
3 participants