Skip to content

Loosen compiler restriction which was not required by the spec #71110

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.

Sign up for GitHub

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

Merged
cston merged 5 commits into from
Dec 12, 2023
Merged

Loosen compiler restriction which was not required by the spec #71110

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5ae35bd
The compiler isn't required by the spec to implement IReadOnly interf…
jnm2 Dec 5, 2023
677aa72
Apply suggestions from review
jnm2 Dec 7, 2023
42a7098
Remove unnecessary field and finish reverting addProperty edits
jnm2 Dec 7, 2023
7dabde7
Whitespace
jnm2 Dec 7, 2023
a37167f
Add suggested test
jnm2 Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 72 additions & 25 deletions ...CSharp/Portable/Symbols/Synthesized/ReadOnlyListType/SynthesizedReadOnlyListTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,16 @@ internal sealed class SynthesizedReadOnlyListTypeSymbol : NamedTypeSymbol
{
SpecialType.System_Collections_IEnumerable,
SpecialType.System_Collections_Generic_IEnumerable_T,
SpecialType.System_Collections_Generic_IReadOnlyCollection_T,
SpecialType.System_Collections_Generic_IReadOnlyList_T,
Comment thread
jnm2 marked this conversation as resolved.
SpecialType.System_Collections_Generic_ICollection_T,
SpecialType.System_Collections_Generic_IList_T,
};

private static readonly SpecialType[] s_readOnlyInterfacesSpecialTypes = new[]
{
SpecialType.System_Collections_Generic_IReadOnlyCollection_T,
SpecialType.System_Collections_Generic_IReadOnlyList_T,
};

private static readonly WellKnownType[] s_requiredWellKnownTypes = new[]
{
WellKnownType.System_Collections_ICollection,
Expand Down Expand Up @@ -63,8 +67,6 @@ internal sealed class SynthesizedReadOnlyListTypeSymbol : NamedTypeSymbol
WellKnownMember.System_Collections_IList__Insert,
WellKnownMember.System_Collections_IList__Remove,
WellKnownMember.System_Collections_IList__RemoveAt,
WellKnownMember.System_Collections_Generic_IReadOnlyCollection_T__Count,
WellKnownMember.System_Collections_Generic_IReadOnlyList_T__get_Item,
WellKnownMember.System_Collections_Generic_ICollection_T__Count,
WellKnownMember.System_Collections_Generic_ICollection_T__IsReadOnly,
WellKnownMember.System_Collections_Generic_ICollection_T__Add,
Expand All @@ -79,6 +81,12 @@ internal sealed class SynthesizedReadOnlyListTypeSymbol : NamedTypeSymbol
WellKnownMember.System_NotSupportedException__ctor,
};

private static readonly WellKnownMember[] s_readOnlyInterfacesWellKnownMembers = new[]
{
WellKnownMember.System_Collections_Generic_IReadOnlyCollection_T__Count,
WellKnownMember.System_Collections_Generic_IReadOnlyList_T__get_Item,
};

private static readonly WellKnownMember[] s_requiredWellKnownMembersUnknownLength = new[]
{
WellKnownMember.System_Collections_Generic_List_T__Count,
Expand All @@ -93,6 +101,10 @@ internal static NamedTypeSymbol Create(SourceModuleSymbol containingModule, stri
var compilation = containingModule.DeclaringCompilation;
DiagnosticInfo? diagnosticInfo = null;

var hasReadOnlyInterfaces =
!compilation.IsTypeMissing(SpecialType.System_Collections_Generic_IReadOnlyCollection_T)
&& !compilation.IsTypeMissing(SpecialType.System_Collections_Generic_IReadOnlyList_T);

@RikkiGibson RikkiGibson Dec 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It feels like this should be initialized in terms of the contents of s_readOnlyInterfacesSpecialTypes, not repeating those types again here. We should also require the well-known Count and Length members to be present.

I think something like s_readOnlyInterfacesSpecialTypes.All(...IsPresent) && s_readOnlyInterfacesWellKnownMembers.All(...IsPresent) would be suitable.

@RikkiGibson RikkiGibson Dec 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's possible that I missed some reason that existence of the types should be checked here, but not existence of the members. @cston in case you had already given some advice here that I missed, I want to make sure we're in agreement about what to do for this.

@jnm2 jnm2 Dec 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@RikkiGibson In an earlier form, it would implement the interfaces if present, and then implement each member if the member was present. Does that sound interesting?

@jnm2 jnm2 Dec 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(Basically, the first commit in the branch)

@RikkiGibson RikkiGibson Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that would be fine, although I would suggest checking the members independently, we can simply implement the interfaces and members that happen to exist and don't implement the ones that don't exist.

(though if the well known interfaces have extra members that we didn't expect, it would also be reasonable to skip implementing the interface entirely or possibly to report an error. Is there already some precedent in this synthesized type already for this case?)

However, I don't want to lead you around in a circle if Cyrus or Chuck have already steered you away from that path. So let's make sure we have consensus on how that should work.

@CyrusNajmabadi CyrusNajmabadi Dec 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i have zero opinion here. IT's up to compiler side how you'd like this code to be structured :)

@cston cston Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These are well-known interfaces with well-known members. Given that, I think we should handle the two likely cases - where the interfaces are available, and where the interfaces are not - to limit the test matrix today and in the future.

Comment on lines +105 to +106

@alrz alrz Dec 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

compilation.IsTypeMissing

This helper doesn't seem to be actually checking if the type is missing. It's rather "IsTypeMarkedAsMissing" and therefore is not suitable for non-test code. The proper way to do this I think is to check if the return value is of type MissingMetadataTypeSymbol. Is that correct?

@cston cston Dec 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @alrz. Logged #71297.


foreach (var type in s_requiredSpecialTypes)
{
diagnosticInfo = compilation.GetSpecialType(type).GetUseSiteInfo().DiagnosticInfo;
Expand All @@ -102,6 +114,18 @@ internal static NamedTypeSymbol Create(SourceModuleSymbol containingModule, stri
}
}

if (hasReadOnlyInterfaces && diagnosticInfo is null)
{
foreach (var type in s_readOnlyInterfacesSpecialTypes)
{
diagnosticInfo = compilation.GetSpecialType(type).GetUseSiteInfo().DiagnosticInfo;
if (diagnosticInfo is { })
{
break;
}
}
}

if (diagnosticInfo is null)
{
foreach (var type in s_requiredWellKnownTypes)
Expand Down Expand Up @@ -138,6 +162,18 @@ internal static NamedTypeSymbol Create(SourceModuleSymbol containingModule, stri
}
}

if (hasReadOnlyInterfaces && diagnosticInfo is null)
{
foreach (var member in s_readOnlyInterfacesWellKnownMembers)
{
diagnosticInfo = getWellKnownTypeMemberDiagnosticInfo(compilation, member);
if (diagnosticInfo is { })
{
break;
}
}
}

if (!hasKnownLength)
{
if (diagnosticInfo is null)
Expand All @@ -163,7 +199,7 @@ internal static NamedTypeSymbol Create(SourceModuleSymbol containingModule, stri
return new ExtendedErrorTypeSymbol(compilation, name, arity: 1, diagnosticInfo, unreported: true);
}

return new SynthesizedReadOnlyListTypeSymbol(containingModule, name, hasKnownLength);
return new SynthesizedReadOnlyListTypeSymbol(containingModule, name, hasKnownLength, hasReadOnlyInterfaces);

static DiagnosticInfo? getSpecialTypeMemberDiagnosticInfo(CSharpCompilation compilation, SpecialMember member)
{
Expand Down Expand Up @@ -194,7 +230,7 @@ internal static NamedTypeSymbol Create(SourceModuleSymbol containingModule, stri
private readonly ImmutableArray<Symbol> _members;
private readonly FieldSymbol _field;

private SynthesizedReadOnlyListTypeSymbol(SourceModuleSymbol containingModule, string name, bool hasKnownLength)
private SynthesizedReadOnlyListTypeSymbol(SourceModuleSymbol containingModule, string name, bool hasKnownLength, bool hasReadOnlyInterfaces)
{
var compilation = containingModule.DeclaringCompilation;

Expand All @@ -218,15 +254,23 @@ private SynthesizedReadOnlyListTypeSymbol(SourceModuleSymbol containingModule, s
var iCollectionT = compilation.GetSpecialType(SpecialType.System_Collections_Generic_ICollection_T).Construct(typeArgs);
var iListT = compilation.GetSpecialType(SpecialType.System_Collections_Generic_IList_T).Construct(typeArgs);

_interfaces = ImmutableArray.Create(
iEnumerable,
iCollection,
iList,
iEnumerableT,
iReadOnlyCollectionT,
iReadOnlyListT,
iCollectionT,
iListT);
_interfaces = hasReadOnlyInterfaces
? ImmutableArray.Create(
iEnumerable,
iCollection,
iList,
iEnumerableT,
iReadOnlyCollectionT,
iReadOnlyListT,
iCollectionT,
iListT)
: ImmutableArray.Create(
iEnumerable,
iCollection,
iList,
iEnumerableT,
iCollectionT,
iListT);

var membersBuilder = ArrayBuilder<Symbol>.GetInstance();
membersBuilder.Add(
Expand Down Expand Up @@ -314,16 +358,19 @@ private SynthesizedReadOnlyListTypeSymbol(SourceModuleSymbol containingModule, s
this,
((MethodSymbol)compilation.GetSpecialTypeMember(SpecialMember.System_Collections_Generic_IEnumerable_T__GetEnumerator)!).AsMember(iEnumerableT),
generateGetEnumeratorT));
addProperty(membersBuilder,
new SynthesizedReadOnlyListProperty(
this,
((PropertySymbol)compilation.GetWellKnownTypeMember(WellKnownMember.System_Collections_Generic_IReadOnlyCollection_T__Count)!).AsMember(iReadOnlyCollectionT),
Comment thread
jnm2 marked this conversation as resolved.
generateCount));
addProperty(membersBuilder,
new SynthesizedReadOnlyListProperty(
this,
((PropertySymbol)((MethodSymbol)compilation.GetWellKnownTypeMember(WellKnownMember.System_Collections_Generic_IReadOnlyList_T__get_Item)!).AssociatedSymbol).AsMember(iReadOnlyListT),
generateIndexer));
if (hasReadOnlyInterfaces)
{
addProperty(membersBuilder,
new SynthesizedReadOnlyListProperty(
this,
((PropertySymbol)compilation.GetWellKnownTypeMember(WellKnownMember.System_Collections_Generic_IReadOnlyCollection_T__Count)!).AsMember(iReadOnlyCollectionT),
generateCount));
addProperty(membersBuilder,
new SynthesizedReadOnlyListProperty(
this,
((PropertySymbol)((MethodSymbol)compilation.GetWellKnownTypeMember(WellKnownMember.System_Collections_Generic_IReadOnlyList_T__get_Item)!).AssociatedSymbol).AsMember(iReadOnlyListT),
generateIndexer));
}
addProperty(membersBuilder,
new SynthesizedReadOnlyListProperty(
this,
Expand Down
23 changes: 13 additions & 10 deletions src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9468,11 +9468,11 @@ static void compareMembers(NamedTypeSymbol sourceType, NamedTypeSymbol synthesiz
[Theory]
[InlineData(SpecialType.System_Collections_IEnumerable, "System.Collections.IEnumerable")]
[InlineData(SpecialType.System_Collections_Generic_IEnumerable_T, "System.Collections.Generic.IEnumerable`1")]
[InlineData(SpecialType.System_Collections_Generic_IReadOnlyCollection_T, "System.Collections.Generic.IReadOnlyCollection`1")]
[InlineData(SpecialType.System_Collections_Generic_IReadOnlyList_T, "System.Collections.Generic.IReadOnlyList`1")]
[InlineData(SpecialType.System_Collections_Generic_IReadOnlyCollection_T, "System.Collections.Generic.IReadOnlyCollection`1", true)]
[InlineData(SpecialType.System_Collections_Generic_IReadOnlyList_T, "System.Collections.Generic.IReadOnlyList`1", true)]
[InlineData(SpecialType.System_Collections_Generic_ICollection_T, "System.Collections.Generic.ICollection`1")]
[InlineData(SpecialType.System_Collections_Generic_IList_T, "System.Collections.Generic.IList`1")]
public void SynthesizedReadOnlyList_MissingSpecialTypes(SpecialType missingType, string missingTypeName)
public void SynthesizedReadOnlyList_MissingSpecialTypes(SpecialType missingType, string missingTypeName, bool isOptional = false)
{
string source = """
using System.Collections.Generic;
Expand All @@ -9487,13 +9487,16 @@ static void Main()
""";
var comp = CreateCompilation(source);
comp.MakeTypeMissing(missingType);
comp.VerifyEmitDiagnostics(
// (6,30): error CS0518: Predefined type 'System.Collections.IEnumerable' is not defined or imported
// IEnumerable<int> x = [0];
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "[0]").WithArguments(missingTypeName).WithLocation(6, 30),
// (7,30): error CS0518: Predefined type 'System.Collections.IEnumerable' is not defined or imported
// IEnumerable<int> y = [..x];
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "[..x]").WithArguments(missingTypeName).WithLocation(7, 30));
comp.VerifyEmitDiagnostics(isOptional
? []
: [
// (6,30): error CS0518: Predefined type 'System.Collections.IEnumerable' is not defined or imported
// IEnumerable<int> x = [0];
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "[0]").WithArguments(missingTypeName).WithLocation(6, 30),
// (7,30): error CS0518: Predefined type 'System.Collections.IEnumerable' is not defined or imported
// IEnumerable<int> y = [..x];
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "[..x]").WithArguments(missingTypeName).WithLocation(7, 30),
]);
}
Comment thread
jnm2 marked this conversation as resolved.

[Theory]
Expand Down