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

Fix analysis of IOperation descendants for BoundDelegateCreationExpre… #19307

Merged
merged 2 commits into from
May 10, 2017

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented May 5, 2017

…ssion

Customer scenario

Customer uses IOperation analyzers in their code base and see large number of false positives in code with delegate creation expressions taking lambda arguments. For example see #8884 (comment)

Bugs this fixes:

Fixes #8884 (comment)

Workarounds, if any

Customer has to suppress all IOperation analyzers in code containing delegate creation expressions OR disable these analyzers completely.

Risk

Low. We have taken same approach for bunch of other bound nodes that still require IOperation design.

Performance impact

Low.

Is this a regression from a previous update?
No.

Root cause analysis:

The current IOperation API implementation of BoundDelegateCreationExpression has a bunch of issues and needs to be redesigned. This is tracked by #8897 and will be addressed post 15.3. Meanwhile, to unblock analyzers on code containing delegate creation expressions with lambda arguments, this bound node has been switched to OperationKind.None with override for Children property.

How was the bug found?

Dogfooding/customer reports.

…ssion

The current IOperation API implementation of BoundDelegateCreationExpression has a bunch of issues and needs to be redesigned. This is tracked by dotnet#8897 and will be addressed post 15.3. Meanwhile, to unblock analyzers on code containing delegate creation expressions with lambda arguments, this bound node has been switched to OperationKind.None with override for Children property.

Fixes the first repro case provided in dotnet#8884
@mavasani
Copy link
Contributor Author

mavasani commented May 5, 2017

Tagging @AlekseyTs @cston @dotnet/analyzer-ioperation for review

@cston
Copy link
Member

cston commented May 5, 2017

LGTM


public void Method(Delegate d)
{
var a = /*<bind>*/new Delegate(Method2)/*</bind>*/;
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2017

Choose a reason for hiding this comment

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

Method2 [](start = 39, length = 7)

Please add a test with an explicit receiver (a parameter reference, for example). We should get it in the output. #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note to #17588 about adding this unit test when implementing IOperation API for bound delegate creation expression.


In reply to: 115620033 [](ancestors = 115620033)

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

@mavasani
Copy link
Contributor Author

mavasani commented May 9, 2017

Tagging @MattGertz for ask mode approval.

@mavasani mavasani merged commit a33479e into dotnet:master May 10, 2017
@mavasani mavasani deleted the DelegateCreationExpression branch May 10, 2017 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IParameterReferenceExpression operation not generated for parameter access in a certain cases
6 participants