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

Ensure that IOperations nodes within a C# local function are part of … #19906

Merged
merged 2 commits into from
Jun 1, 2017

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented May 31, 2017

…the operation tree

Customer scenario

Analysis of any code within a local function with IOperation analyzers will produce false or missing diagnostics, as the operation tree doesn't contain any operation nodes within the BoundLocalFunctionStatement. This can be a dogfood blocker for IOperation analyzers on real world code - we had few false reports due this on the Roslyn repo as we use this new language feature, and corresponding rules had to be disabled in the repo.

Bugs this fixes:

Workaround for #19902

Workarounds, if any

Disable or uninstall IOperation based analyzers

Risk

Low risk. We are following the common pattern for not yet implemented IOperation feature API for 15.3 - we don't yet provide rich IOperation API for the new language feature, but ensure that any descendant operation nodes are still included in the Operation tree.

Performance impact

None

Is this a regression from a previous update?

No, this never worked before.

Root cause analysis:

We missed this while adding unit tests because BoundLocalFunctionStatement actually returns OperationKind.LocalFunctionStatement, but there is no corresponding IOperation API for local functions.

How was the bug found?

Dogfooding.

…the operation tree

Analysis of any code within a local function with IOperation analyzers will produce false or missing diagnostics, as the operation tree doesn't contain any operation nodes within the BoundLocalFunctionStatement. This can be a dogfood blocker for IOperation analyzers on real world code - we had few false reports due this on the Roslyn repo as we use this new language feature, and corresponding rules had to be disabled in the repo.
@mavasani
Copy link
Contributor Author

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

Note the unshipped public API for VisitLocalFunctionStatement was not correct - it operation on an IOperation node, instead of a specific type of IOperation API for local functions. I have removed this for 15.3 and we can add it back in 15.later when we have designed IOperation API for local functions.

@CyrusNajmabadi
Copy link
Member

@mavasani you're referencing a PR not an Issue.

@jinujoseph
Copy link
Contributor

@CyrusNajmabadi corrected with issueid

@mavasani
Copy link
Contributor Author

Test failure due to NuGet hit a corrupted cache on this machine. Please delete the agent and then retest, noting you hit "NuGet cache corruption with null files".

@mavasani
Copy link
Contributor Author

@dotnet-bot retest windows_coreclr_test_prtest please

@mavasani
Copy link
Contributor Author

Ping..

@mavasani
Copy link
Contributor Author

mavasani commented Jun 1, 2017

Can someone from @dotnet/roslyn-compiler take a look as well?

Tagging @MattGertz for approval.

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 mavasani merged commit e758b53 into dotnet:master Jun 1, 2017
@mavasani mavasani deleted the FixLocalFunctionStatement branch June 1, 2017 20:08
@AlekseyTs
Copy link
Contributor

Please follow the policy of two sign-offs from @dotnet/roslyn-compiler .

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM

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.

9 participants