Skip to content

Support params keyword on non-array collections #3444

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ds5678
Copy link
Contributor

@ds5678 ds5678 commented Apr 1, 2025

Resolves #3431

Problem

This adds support for showing the params keyword on non-array collection parameters. It should not affect call sites.

Solution

  • Any comments on the approach taken, its consistency with surrounding code, etc.
    • I'm not sure if this should be part of TypeSystemOptions, but that seemed consistent.
  • Which part of this PR is most in need of attention/improvement?
    • I'm not certain that I did language versioning correct.
  • At least one test covering the code changed

@ds5678
Copy link
Contributor Author

ds5678 commented Apr 1, 2025

Apparently, the CI didn't like that I bumped the language version to 13. I'm not sure how to fix that.

@ds5678 ds5678 force-pushed the params-collections-annotations branch from d62ede5 to a807e1a Compare April 5, 2025 19:14
@siegfriedpammer
Copy link
Member

Don't worry about the build fail... We will take care of the infrastructure for the new language version

@christophwille
Copy link
Member

For reference - this happens regularly #2873

@ds5678 ds5678 force-pushed the params-collections-annotations branch from 54e2f51 to d6a251b Compare April 8, 2025 18:15
@christophwille
Copy link
Member

I only had a cursory look comparing #2873 to your changes - global.json and ICSharpCode.Decompiler.TestRunner.csproj actually should be set to net90. Also, Helpers/Tester.cs then needs different output paths. Maybe if you want to implement that transition for tests to net90 it would be best to compare side-by-side with that old PR.

@ds5678 ds5678 force-pushed the params-collections-annotations branch from d6a251b to 6e5c2f9 Compare April 19, 2025 05:15
@christophwille
Copy link
Member

I just saw the break for dotnet format; that didn't bomb back then with the move to net70.

Maybe this applies then here https://github.com/icsharpcode/ILSpy/pull/3238/files

https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json exists with a version of 9.0.520307

@ds5678
Copy link
Contributor Author

ds5678 commented Apr 19, 2025

Bumping to the .NET 9 SDK also caused some issues with tests now failing. It changed how some code is compiled (eg overload resolution priority). I'm not sure how to fix that, other than to implement support for the features it's using.

@christophwille
Copy link
Member

@siegfriedpammer oh, that is even more involved then. We need to have a look.

@siegfriedpammer
Copy link
Member

I'm not sure how to fix that, other than to implement support for the features it's using.

Yeah, it's similar to when we upgrade to a newer version of Roslyn. We need to update the decompiler engine in lots of different places.

I think it would be a good idea to do the dotnet upgrade in a separate PR before anything else.

Unfortunately, I am blocked by family obligations due to the holidays, so I won't have time to look at this soon.

@christophwille
Copy link
Member

Update @ds5678 - @siegfriedpammer and I had a longer deliberation on how to tackle the net90 and upgrade. We came to the conclusion it is better to right away go net10. We'll keep you posted once that has happened (shouldn't be too far out)

@ds5678
Copy link
Contributor Author

ds5678 commented Apr 22, 2025

Thanks for the status update!

@siegfriedpammer
Copy link
Member

@ds5678 Just to let you know, the decompiler is missing some smaller transforms related to InlineArray and Span, which are used extensively in net10, blocking the net10 upgrade currently.

This will take some more time to resolve. I will create a branch/PR in the next few days to make progress more visible. Also, if you want to help, we can discuss further steps then.

@siegfriedpammer
Copy link
Member

So, I have spent some time looking into the whole thing and here's what I found:

The .NET 10 upgrade fails due to missing support for

  • Some inlining/variable splitting special cases
  • InlineArrays
  • Collection Expressions (*)

(*) some of the special cases mentioned in the first point can only be generally represented using Collection Expressions...

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

Successfully merging this pull request may close these issues.

Params keyword on non-array collections
3 participants