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 param collection expression binding in expression trees #74166

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bernd5
Copy link
Contributor

@bernd5 bernd5 commented Jun 26, 2024

fixes: #74163

@bernd5 bernd5 requested a review from a team as a code owner June 26, 2024 11:39
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 26, 2024
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 26, 2024
@bernd5
Copy link
Contributor Author

bernd5 commented Jun 26, 2024

We should may add a test for the concrete issue, too. I just created one for the root issue.
(the test-project seems to have no ref assembly to Net9.0)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

We don't want to bind differently in expression tree context. See https://github.com/dotnet/csharplang/blob/main/proposals/params-collections.md#expression-trees

@bernd5
Copy link
Contributor Author

bernd5 commented Jun 26, 2024

Oh, then it becomes a breaking change to existing code. But this is wanted???

@AlekseyTs
Copy link
Contributor

Oh, then it becomes a breaking change to existing code. But this is wanted???

Please see https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-06-17.md#params-span-breaks for more information.

@bernd5
Copy link
Contributor Author

bernd5 commented Jun 26, 2024

I see - then we would need to fix the lowering (and use an array).

@AlekseyTs
Copy link
Contributor

I see - then we would need to fix the lowering (and use an array).

Please see #74163 (comment). First, we need to understand the root cause of the problem. Where and what is going wrong. Only then we can decide what the fix should look like. I suggest you to start with that if you would like to continue working on the issue.

@AlekseyTs
Copy link
Contributor

then we would need to fix the lowering (and use an array).

We probably don't want to change the lowering in this fashion either.

@bernd5
Copy link
Contributor Author

bernd5 commented Jun 26, 2024

We probably don't want to change the lowering in this fashion either.

I wouldn't like to change the lowering code for interpolated string, too.
We could simply dissallow such code (breaking change) - but this is also not very well.

@AlekseyTs
Copy link
Contributor

I wouldn't like to change the lowering code for interpolated string, too. We could simply dissallow such code (breaking change) - but this is also not very well.

Let's discuss possible fix strategies in the issue. While proposing a strategy, please make the proposal very specific and detailed. For example, the quote above looks neither specific, nor detailed to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET 9 Preview.5, Preview.6, Preview.7 C# compiler crashes on LINQ expression with string interpolation
3 participants