Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Collection expression spread optimization using List<T>.AddRange(IEnumerable<T>) skipped for ICollection<U> when T and U are distinct types at compile-time only #74894

Closed
cston opened this issue Aug 24, 2024 · 4 comments · Fixed by #74949
Assignees
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Milestone

Comments

@cston
Copy link
Member

cston commented Aug 24, 2024

The collection expression spread optimization for [.. x] where the target type is List<T> is skipped unnecessarily when:

  • x has a value type enumerator, and
  • x implements ICollection<U>, and
  • T and U are distinct types at compile-time but not at runtime (for instance, object vs. object?, or (int X, int Y) vs. (int, int)).

See #74630
See #74769 (comment)

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 24, 2024
@cston
Copy link
Member Author

cston commented Aug 24, 2024

Lowering is currently using TypeSymbolExtensions.ImplementsInterface() to check for ICollection<T> (see LocalRewriter), but we should probably use a conversion method such as ConversionsBase.ClassifyBuiltInConversion() instead.

@DoctorKrolic
Copy link
Contributor

ImplementInterface currently uses TypeCompareKind.ConsiderEverything2, which is marked as "temporary" an should be removed. Given that current behavior of ImplementInterface may lead to unexpected results like this issue and is confuisng enough, that it passed a compiler code review, maybe we should revise its implementation and use other compare kind (like CLRSignatureCompareOptions) instead? Currently this helper is only used in 1 other place, so it should be simple to track all behavior changes if we revise the helper. Also note, that in both places discardedUseSiteInfo is passed as a second argument, so if we decide to touch the method, maybe we should also get rid of that parameter entirely?

@cston
Copy link
Member Author

cston commented Aug 29, 2024

As mentioned in #74769 (comment), I don't think we should use ImplementsInterface() here.

The check we want to perform in this case is essentially expr is ICollection<T> which should be implemented by ConversionsBase.ClassifyBuiltInConversion() or a similar conversion method. It's not clear that ImplementsInterface() matches the expected conversion behavior.

@cston
Copy link
Member Author

cston commented Aug 29, 2024

ImplementsInterface(this TypeSymbol subType, ...) also does not seem to handle the case where subType is a type parameter that implements interfaces - see #74769 (comment).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants