-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Allow collection expressions with CreateCompilation
#71134
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
#if !NET8_0_OR_GREATER | ||
|
||
namespace System.Runtime.CompilerServices | ||
{ | ||
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface)] | ||
public sealed class CollectionBuilderAttribute : Attribute | ||
{ | ||
public CollectionBuilderAttribute(Type builderType, string methodName) | ||
{ | ||
BuilderType = builderType; | ||
MethodName = methodName; | ||
} | ||
|
||
public Type BuilderType { get; } | ||
public string MethodName { get; } | ||
} | ||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,10 @@ | |
#nullable disable | ||
|
||
using System; | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.Diagnostics; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Text; | ||
using Microsoft.CodeAnalysis.Text; | ||
|
@@ -19,7 +19,8 @@ namespace Microsoft.CodeAnalysis.CSharp.Test.Utilities | |
/// Represents the source code used for a C# test. Allows us to have single helpers that enable all the different ways | ||
/// we typically provide source in testing. | ||
/// </summary> | ||
public readonly struct CSharpTestSource | ||
[System.Runtime.CompilerServices.CollectionBuilder(typeof(CSharpTestSourceBuilder), nameof(CSharpTestSourceBuilder.Create))] | ||
public readonly struct CSharpTestSource : IEnumerable<CSharpTestSource> | ||
{ | ||
public static CSharpTestSource None => new CSharpTestSource(null); | ||
|
||
|
@@ -83,5 +84,17 @@ public SyntaxTree[] GetSyntaxTrees(CSharpParseOptions parseOptions, string sourc | |
public static implicit operator CSharpTestSource(List<SyntaxTree> source) => new CSharpTestSource(source.ToArray()); | ||
public static implicit operator CSharpTestSource(ImmutableArray<SyntaxTree> source) => new CSharpTestSource(source.ToArray()); | ||
public static implicit operator CSharpTestSource(CSharpTestSource[] source) => new CSharpTestSource(source); | ||
|
||
// Dummy IEnumerable support to satisfy the collection expression and CollectionBuilder requirements | ||
IEnumerator<CSharpTestSource> IEnumerable<CSharpTestSource>.GetEnumerator() => throw new NotImplementedException(); | ||
IEnumerator IEnumerable.GetEnumerator() => throw new NotImplementedException(); | ||
|
||
internal static class CSharpTestSourceBuilder | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need to be a separate type? Why not just make it a static member on this type? #WontFix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not need to be a separate type, but it is customary for the collection and the collection builder to be different types, and I think it makes sense. See all the collection tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is a brand new pattern, there's no "customary" at all 😆. I'd just as soon keep it simpler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All our collection builder tests beg to differ. The spec also calls out two different concepts: a "collection type" and a "builder type". It is clearer not to blend them and doesn't seem "simpler". |
||
{ | ||
public static CSharpTestSource Create(ReadOnlySpan<CSharpTestSource> source) | ||
{ | ||
return source.ToArray(); | ||
} | ||
} | ||
} | ||
} |
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.
Just to make sure that we don't run afoul of any weird situations (such as razor test dependencies), we should typeforward in an
#else
: https://github.com/dotnet/roslyn/blob/main/docs/Target%20Framework%20Strategy.md#pattern-for-types #ResolvedThere 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 for the pointer. That's a useful doc