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 null check combined with nullable bool test #32159

Merged
merged 2 commits into from Jan 10, 2019

Conversation

Projects
None yet
7 participants
@sharwell
Copy link
Member

sharwell commented Jan 4, 2019

  • Fixes #31739
  • Fixes timeout for integration tests (they took 1:30 locally so I allowed for 2:00 on CI)

Customer scenario

Visual Studio can crash with a FailFast during editing.

Bugs this fixes

#31739

Workarounds, if any

None.

Risk

Low. The change restores a null check that was unintentionally removed.

Performance impact

Negligible.

Is this a regression from a previous update?

Yes.

Root cause analysis

The issue was caught during code review but not corrected before merge.

How was the bug found?

Integration tests.

Test documentation updated?

N/A

@sharwell sharwell requested a review from dotnet/roslyn-ide as a code owner Jan 4, 2019

@sharwell sharwell force-pushed the sharwell:fix-nre branch from 654ed4c to 39ddec8 Jan 4, 2019

@mavasani
Copy link
Contributor

mavasani left a comment

Thanks!

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 4, 2019

@jaredpar for approval

⚠️ Note the timeout change during approval

return;
}

if (!documentOpt.SupportsDiagnostics())

This comment has been minimized.

@gafter

gafter Jan 4, 2019

Member

You could write

            if (documentOpt?.SupportsDiagnostics() != true)
            {
                return;
            }

This comment has been minimized.

@sharwell

sharwell Jan 4, 2019

Member

I could, but I figured if I already messed it up once it's not obvious enough to prefer.

@sharwell sharwell changed the base branch from master to dev16.0-preview2-vs-deps Jan 9, 2019

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 9, 2019

@jinujoseph I need this for integration tests to work

Show resolved Hide resolved eng/build.ps1 Outdated

@sharwell sharwell force-pushed the sharwell:fix-nre branch from 39ddec8 to c010b18 Jan 9, 2019

@mavasani

This comment has been minimized.

Copy link
Contributor

mavasani commented Jan 9, 2019

@sharwell Integration test failures in #32283 are due to this bug, confirmed the failure call stacks.

@mavasani

This comment has been minimized.

Copy link
Contributor

mavasani commented Jan 9, 2019

Another PR with integration tests failing due to the FailFast being fixed here: #32293

@Neme12

This comment has been minimized.

Copy link
Contributor

Neme12 commented Jan 9, 2019

Is this the reason that integration tests have been failing in all PRs for the past month?
https://dnceng.visualstudio.com/public/_build?definitionId=245 Will this fix that?

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 9, 2019

@Neme12 This is one of many reasons.

@jinujoseph

This comment has been minimized.

Copy link
Contributor

jinujoseph commented Jan 10, 2019

@sharwell good to merge

@sharwell sharwell changed the base branch from dev16.0-preview2-vs-deps to dev16.0-preview2 Jan 10, 2019

@sharwell sharwell merged commit 07eee6e into dotnet:dev16.0-preview2 Jan 10, 2019

2 of 3 checks passed

roslyn-integration-CI #20190110.40 failed
Details
license/cla All CLA requirements met.
Details
roslyn-CI #20190110.40 succeeded
Details

@sharwell sharwell deleted the sharwell:fix-nre branch Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment