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

Compiler chooses params span overload in expression trees leading to errors #73743

Open
ericstj opened this issue May 28, 2024 · 8 comments
Open

Comments

@ericstj
Copy link
Member

ericstj commented May 28, 2024

Version Used:
4.11.0-2.24268.2+b9c35f1021d2d9d40521474c388d49ee7580ec98

Steps to Reproduce:

  1. Call an API that has both params [] and params span overloads from an expression.

Minimal repro: exprSpan.zip

(sharplab doesn't yet have params span feature)

This could be seen as a source-breaking change for folks if they hit in conjunction with seeing new params span overloads for framework APIs. That's what happened here dotnet/efcore@dae7d42

Diagnostic Id:
CS8640, CS9226

Expected Behavior:
No error

Actual Behavior:

C:\scratch\exprSpan\Program.cs(8,51): error CS8640: Expression tree cannot contain value of ref struct or restricted type 'ReadOnlySpan'.
C:\scratch\exprSpan\Program.cs(8,51): error CS9226: An expression tree may not contain an expanded form of non-array params collection parameter.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 28, 2024
@CyrusNajmabadi
Copy link
Member

Oh that's interesting. And something we didn't consider when we had the intuition of "calling the span version is always better" :) This is def worth bringing to LDM to discuss.

@jaredpar jaredpar added Bug New Feature - ParamsCollections and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 5, 2024
@jaredpar jaredpar added this to the 17.11 milestone Jun 5, 2024
@jaredpar
Copy link
Member

jaredpar commented Jun 5, 2024

@AlekseyTs this seems a potentially problematic case. Expression trees can't work with the new overloads but it's also quite logical for them to bind to them. Not sure that we have any precedence for this in the language. Definitely have cases where we say an expression tree can't use a feature but this is more we need it to change binding within expression trees. Don't have any good intuition on how to solve this.

@AndriySvyryd
Copy link
Member

If the behavior won't be changed, consider adding an analyzer that could fix all of these instances.

@333fred
Copy link
Member

333fred commented Jun 19, 2024

Discussed in LDM on the 12th and 17th.

@AlekseyTs
Copy link
Contributor

From https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-06-17.md#conclusion:

Do not change the compiler. Improve the error reporting experience.

From https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-06-17.md#params-span-breaks:

We do think we should invest a bit more in the error experience; having the compiler detect when such an overload is used in an expression tree and issue a specific diagnostic, and having the IDE offer to add an explicit array creation will go a long way to making the experience understandable.

@AlekseyTs
Copy link
Contributor

It looks to me that we already produce a unique enough error message for the scenario. Specifically, I am referring to

error CS9226: An expression tree may not contain an expanded form of non-array params collection parameter.

It is true that the error is not limited to span scenarios, but I think that the suggested IDE fix will be helpful for users in non-span scenarios as well.

@AlekseyTs
Copy link
Contributor

Moving to IDE to offer the suggested fix.

@AlekseyTs
Copy link
Contributor

@CyrusNajmabadi Could you please route this issue accordingly? Add the right labels, etc.

@AlekseyTs AlekseyTs removed their assignment Jun 19, 2024
@CyrusNajmabadi CyrusNajmabadi self-assigned this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants