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

Add null checks for node and its parent in NewLineUserSettingFormattingRule.IsControlBlock #21833

Closed
wants to merge 1 commit into from
Closed

Add null checks for node and its parent in NewLineUserSettingFormattingRule.IsControlBlock #21833

wants to merge 1 commit into from

Conversation

tcNickolas
Copy link

Customer scenario

If syntax tree has NamespaceDeclaration as a root node instead of CompilationUnit, IsControlBlock is called for NamespaceDeclaration node which has null Parent, and this code throws NullReferenceException at line 19. This is not a common scenario, as valid trees are supposed to have CompilationUnit as a root node, but it's better to have the checks in place.

Workarounds, if any

Use CompilationUnit as root node of the tree.

Risk

Low because these checks were present in the code before.

Performance impact

Low perf impact because the only extra work done is two null checks.

Is this a regression from a previous update?

Yes, these checks were removed in 146b391.

How was the bug found?

Customer reported

@sharwell sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Aug 31, 2017
@Pilchie Pilchie requested a review from a team September 8, 2017 15:10
Copy link
Contributor

@heejaechang heejaechang left a comment

Choose a reason for hiding this comment

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

can we add a test as well?

@tcNickolas
Copy link
Author

Sure, I can try and figure out how to write a test for this. Do you know whether there is a test for a similar scenario in the codebase which I could use as an example? All existing tests seem to construct a syntax tree which has CompilationUnit as a root

@jaredpar
Copy link
Member

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!

@jaredpar jaredpar closed this Apr 24, 2018
@jaredpar jaredpar added the Resolution-Expired The request is closed due to inactivity under our contribution review policy. label Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE cla-already-signed Community The pull request was submitted by a contributor who is not a Microsoft employee. Resolution-Expired The request is closed due to inactivity under our contribution review policy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants