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.

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

Jump to bottom

Fix for "Function pointer invocation has "None" IOperation" #57191

Merged
merged 48 commits into from
Nov 5, 2021
Merged

Fix for "Function pointer invocation has "None" IOperation" #57191

merged 48 commits into from
Nov 5, 2021

Conversation

bernd5
Copy link
Contributor

@bernd5 bernd5 commented Oct 17, 2021

fixes: #48082

@bernd5 bernd5 requested a review from a team as a code owner October 17, 2021 11:33
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 17, 2021
@bernd5 bernd5 marked this pull request as draft October 17, 2021 11:34
@bernd5 bernd5 changed the title Draft for Function pointer invocation has "None" IOperation Fix for "Function pointer invocation has "None" IOperation" Oct 17, 2021
@bernd5 bernd5 marked this pull request as ready for review October 17, 2021 16:56
@bernd5
Copy link
Contributor Author

bernd5 commented Oct 17, 2021

@333fred could you have a look?

@333fred 333fred marked this pull request as draft October 17, 2021 17:11
@333fred
Copy link
Member

333fred commented Oct 17, 2021

@bernd5 this needs an API review. Please follow the process outlined in https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md.

@bernd5
Copy link
Contributor Author

bernd5 commented Oct 17, 2021

See #57195

@bernd5 bernd5 mentioned this pull request Oct 17, 2021
Closed
@bernd5
Copy link
Contributor Author

bernd5 commented Oct 18, 2021

@333fred Do you see why the "roslyn-integration-CI" test failed?

@333fred
Copy link
Member

333fred commented Oct 18, 2021

Don't worry about it.

@bernd5
Copy link
Contributor Author

bernd5 commented Oct 29, 2021

Should we catch null-Type in GetFunctionPointerSignature - this can happen for code with errors?

@runfoapp runfoapp bot mentioned this pull request Oct 29, 2021
Open
@333fred
Copy link
Member

333fred commented Oct 29, 2021

Should we catch null-Type in GetFunctionPointerSignature - this can happen for code with errors?

Are you stating that it can happen and you have a test case, or that you're unsure if it can happen and wondering if we should handle it?

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 2, 2021

For now I extended NoneOperation with missing type data (see CSharpOperationFactory.Create).
Unfortunately this breaks a lot of tests. To workaround this I introduced a WorkaroundNoneOperation.
In another PR the tests should be updated. After that we can remove this workaround and the corresponding check in OperationTreeVerifier.LogCommonProperties.

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 2, 2021

@333fred @AlekseyTs Could you have a look again?

@333fred 333fred mentioned this pull request Nov 2, 2021
Closed
@bernd5
Copy link
Contributor Author

bernd5 commented Nov 3, 2021

Is there a failing test?

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 3, 2021

@333fred @AlekseyTs : All checks have passed now

AlekseyTs
AlekseyTs approved these changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 47)

@runfoapp runfoapp bot mentioned this pull request Nov 3, 2021
Closed
@bernd5
Copy link
Contributor Author

bernd5 commented Nov 4, 2021

@333fred is here still something I can do?

333fred
333fred approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 47), modulo one small suppression that can be removed. @bernd5, once that is fixed I'll set this to auto squash and merge.

@333fred
Copy link
Member

333fred commented Nov 4, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@333fred 333fred enabled auto-merge (squash) November 4, 2021 23:07
@333fred 333fred merged commit 308d631 into dotnet:main Nov 5, 2021
@ghost ghost added this to the Next milestone Nov 5, 2021
@bernd5 bernd5 deleted the fix_FunctionPointerOperation branch November 5, 2021 06:40
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
@333fred 333fred mentioned this pull request Mar 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@333fred 333fred 333fred approved these changes

Assignees
No one assigned
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.1.P2
Development

Successfully merging this pull request may close these issues.

Function pointer invocation has "None" IOperation
4 participants
@bernd5 @333fred @AlekseyTs @allisonchou