-
Notifications
You must be signed in to change notification settings - Fork 777
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
C# collection expression support for F# lists & sets #17359
C# collection expression support for F# lists & sets #17359
Conversation
❗ Release notes required
|
* The `EditorBrowsable` attribute on the `ScopedRefAttribute` definition only exists on the BCL implementation because the `scope` keyword is meant to be used in C# instead of this attribute. Since F# doesn't have the `scoped` keyword, this reasoning doesn't apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice, thanks for this. It's nice to be able to improve C# F# interop.
Does anyone know what needs to be done with the new C# test project I added to make the CI happy? Something about the test results needing to be in a certain place, and/or packaging/PDBs? |
…ollection-expression-support
Forgive me if I'm being obtuse, but I don't think I understand the cause of this error in the CI:
The only results on Google are dotnet/runtime#62892 and dotnet/machinelearning#4400, which don't help me much, since I don't understand the underlying problem. I can see that it comes from here, but... I do not understand why this PR is triggering that. Is there something obvious I'm missing? |
Failed to run : https://github.com/dotnet/fsharp/actions/runs/9718927879 |
67457e9
to
44e0dcd
Compare
Do they need to be part of new project? We successfully test interop in component tests. |
Hmm, I guess they could technically go there, although these are specifically meant to test the C# compiler's handling of types from FSharp.Core and have nothing to do with the F# compiler per se. |
…ollection-expression-support
If we want to put these tests in FSharp.Compiler.ComponentTests/Interop (or, perhaps better, FSharp.Core.UnitTests/Interop), we need to update Line 92 in 5c6d8e7
The version in Separately, while it would be nicer from the test writer's perspective to test the consumption of FSharp.Core constructs in C# in an actual C# project, I guess it would have the downside of making it harder to run specific tests against specific versions of C#. |
…ollection-expression-support
The Roslyn deps were updated in #17144, merged to |
Caution Repository is on lockdown for maintenance, all merges are on hold. |
…ollection-expression-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Brian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I'm happy to merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, as usual. Thanks!
Description
Examples
Checklist