Skip to content

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

Merged
333fred merged 48 commits intodotnet:mainfrom
bernd5:fix_FunctionPointerOperation
Nov 5, 2021
Merged

Fix for "Function pointer invocation has "None" IOperation"#57191
333fred merged 48 commits intodotnet:mainfrom
bernd5:fix_FunctionPointerOperation

Conversation

@bernd5
Copy link
Copy Markdown
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 Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

bernd5 commented Oct 17, 2021

See #57195

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

bernd5 commented Oct 18, 2021

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

@333fred
Copy link
Copy Markdown
Member

333fred commented Oct 18, 2021

Don't worry about it.

@bernd5
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
333fred
333fred reviewed Nov 2, 2021
View reviewed changes
@bernd5
Copy link
Copy Markdown
Contributor Author

bernd5 commented Nov 3, 2021

Is there a failing test?

@bernd5
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

bernd5 commented Nov 4, 2021

@333fred is here still something I can do?

333fred
333fred reviewed Nov 4, 2021
View reviewed changes
333fred
333fred approved these changes Nov 4, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

333fred commented Nov 4, 2021

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Nov 4, 2021

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

@333fred 333fred 333fred approved these changes

@AlekseyTs AlekseyTs AlekseyTs 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