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

Surface IOperation info to allow analyzers to detect shadow copies for non-readonly methods over in params #58539

Open
Sergio0694 opened this issue Dec 31, 2021 · 2 comments
Labels
Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Milestone

Comments

@Sergio0694
Copy link
Contributor

Follow up from the conversation with @jaredpar and @CyrusNajmabadi in #roslyn in the C# Discord server.

Background and Motivation

A big issue with in parameters today is that they can introduce shadow copies when:

  1. Invoking non-readonly members on an in struct parameter.
  2. Invoking any member on an in T generic parameter, regardless of whether the runtime type/member is readonly.

There currently is no way to spot them, you just have to know, and even then, it's easy to accidentally introduce copies without noticing, as there is zero compiler help here. Introducing copies can at best still defeat the whole point of using in, which is to avoid copies and improve performance, and at worst just flat out break the code, in case it relied on being invoked on the original value.

Proposal

There should be enough info on IOperation to allow analyzer authors to spot all hidden copies done by Roslyn. Not familiar with the API surface there so I don't have an exact API proposal here, but I know Jared will be able to fill the blanks here 😄

Risks

None that I can see.

@Sergio0694 Sergio0694 added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Dec 31, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 31, 2021
@CyrusNajmabadi
Copy link
Member

@333fred for thoughts. Do the bound nodes know/expose if they introduced a defensive copy?

@jinujoseph jinujoseph removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jan 4, 2022
@jinujoseph jinujoseph added this to the Backlog milestone Jan 4, 2022
@sharwell
Copy link
Member

sharwell commented Jan 4, 2022

I've discussed this in the past with @mavasani. I believe it will need a new top-level analyzer callback for value copies. In addition to potentially having a copy on any IOperation node, there are situations where a copy can occur without any IOperation representation available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Projects
None yet
Development

No branches or pull requests

4 participants