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

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

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Dec 5, 2023

Closes #71054 /cc @CyrusNajmabadi

@jnm2 jnm2 requested a review from a team as a code owner December 5, 2023 23:28
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 5, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 5, 2023
@jnm2 jnm2 requested a review from cston December 5, 2023 23:29
@RikkiGibson RikkiGibson self-assigned this Dec 5, 2023
@jnm2 jnm2 requested a review from cston December 7, 2023 21:54
@jnm2 jnm2 requested a review from cston December 7, 2023 23:21
@CyrusNajmabadi
Copy link
Member

@RikkiGibson ptal :)

@@ -89,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);
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
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?

Copy link
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)

Copy link
Contributor

@RikkiGibson RikkiGibson Dec 8, 2023

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.

Copy link
Member

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 :)

Copy link
Member

@cston cston Dec 8, 2023

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.

@cston cston self-requested a review December 8, 2023 16:09
@RikkiGibson
Copy link
Contributor

It sounds like we are waiting on a suggested test being added and then we are ready to merge. Is that right? @cston @jnm2

@jnm2
Copy link
Contributor Author

jnm2 commented Dec 12, 2023

Yes, done now.

@cston cston merged commit b9f96c7 into dotnet:main Dec 12, 2023
@cston
Copy link
Member

cston commented Dec 12, 2023

Thanks @jnm2!

@ghost ghost added this to the Next milestone Dec 12, 2023
@jnm2 jnm2 deleted the synth_collection_spec_recommendation branch December 12, 2023 13:59
@jnm2
Copy link
Contributor Author

jnm2 commented Dec 12, 2023

Thanks for your guidance!

@CyrusNajmabadi
Copy link
Member

Thanks @jnm2 !

Comment on lines +105 to +106
!compilation.IsTypeMissing(SpecialType.System_Collections_Generic_IReadOnlyCollection_T)
&& !compilation.IsTypeMissing(SpecialType.System_Collections_Generic_IReadOnlyList_T);
Copy link
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?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @alrz. Logged #71297.

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. Feature - Collection Expressions 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.

Compiler is too strict with its requirements for collection-expression code-gen when targeting an interface.
7 participants