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.

Sign up for GitHub

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

Jump to bottom

crash on ClassifyUpdate for EventFields #35962

Merged
merged 1 commit into from
Jun 18, 2019
Merged

crash on ClassifyUpdate for EventFields #35962

merged 1 commit into from
Jun 18, 2019

Conversation

ivanbasov
Copy link
Contributor

Fixes #32889

@ivanbasov ivanbasov added Bug Interactive-EnC Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner. labels May 25, 2019
@ivanbasov ivanbasov requested review from tmat and a team May 25, 2019 00:42
@ivanbasov
Copy link
Contributor Author

@tmat please review

tmat
tmat reviewed May 30, 2019
View reviewed changes
src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs Outdated
@@ -2492,8 +2492,8 @@ private void ClassifyUpdate(VariableDeclaratorSyntax oldNode, VariableDeclarator
}

// Check if a constant field is updated:
var fieldDeclaration = (FieldDeclarationSyntax)oldNode.Parent.Parent;
if (fieldDeclaration.Modifiers.Any(SyntaxKind.ConstKeyword))
if ((oldNode.Parent.Parent is FieldDeclarationSyntax fieldDeclaration) &&
Copy link
Member

@tmat tmat May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't quite like this. We already know we are dealing with a variable declaration within a type declaration.

That means it has to be a field declaration of some kind. Therefore the check is unnecessary and just confusing since it begs question what else it could be and shoudl we handle that thing in some way as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmat , the crash (#32889) we are solving is the variable declaration with the type declaration where the variable is not a field but an event field. I think that the unit test provided covers this. Not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmat please review my previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmat please review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmat please review

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the original fix, that is cast to BaseFieldDeclarationSyntax. In this case const is not applicable to event fields, but that's fine. Then it won't be found in the modifiers anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmat I have reverted. Please sign off.
@jinujoseph @ManishJayaswal please consider for 16.2 Preview 4 / GA. It seems to be a pretty easy fix.

@jinujoseph jinujoseph added this to the 16.2.P4 milestone Jun 18, 2019
@ivanbasov ivanbasov merged commit 9c01a18 into dotnet:master Jun 18, 2019
@ivanbasov ivanbasov deleted the eventfield1 branch June 18, 2019 19:48
333fred added a commit to 333fred/roslyn that referenced this pull request Jun 18, 2019
@333fred
Merge remote-tracking branch 'dotnet/master' into null-coalesce-value…
23153de 
…-types

* dotnet/master: (63 commits)
  Fix stack overflow in requesting syntax directives (dotnet#36347)
  crash on ClassifyUpdate for EventFields (dotnet#35962)
  Disable move type when the options service isn't present (dotnet#36334)
  Fix crash where type inference doing method inference needs to drop nullability
  Fix parsing bug in invalid using statements (dotnet#36428)
  Do not suggest or diagnose use compound assignment when right hand of binary operator is a throw expression
  Add option to emit nullable metadata for public members only (dotnet#36398)
  Added null checks on F# external access services (dotnet#36469)
  Deal with discovering extra .editorconfig files
  Re-enable MSBuildWorkspaceTests.TestEditorConfigDiscovery
  Add support to VisualStudioMSBuildInstalled to support minimum versions
  Fix configuration of accessibilities in editorconfig
  Shorten a resource ID
  Revert "Extract the RDT implementation for Misc files and VS open file tracker"
  Add nullability support to use local function
  Add EditorFeatures.WPF dependency to F# ExternalAccess
  Ensure NullableWalker.AsMemberOfType locates the right new container for the member. (dotnet#36406)
  Replace `dynamic` with `object` when substituting constraints. (dotnet#36379)
  Add some string descriptions
  Adjust type of out var based on parameter state (dotnet#36284)
  ...
333fred added a commit to 333fred/roslyn that referenced this pull request Jun 20, 2019
@333fred
Merge remote-tracking branch 'dotnet/master' into wire-speculative
9de9b67 
* dotnet/master: (85 commits)
  Don't complete statement when typing semicolon inside comments in an argument list (dotnet#36521)
  Set focus to editor before finding text
  Add a bunch of nullability support to some code generation helpers
  Add 'annotations' and 'warnings' support to nullable directive (dotnet#36464)
  Fixed IDE services touching `notnull` constraint (dotnet#36508)
  Update compiler toolset to arcade version (dotnet#36549)
  Fix for 923157
  Do not include value types in NullableAttribute byte[] (dotnet#36519)
  Fix a deadlock in InvokeOnUIThread
  Apply a hang mitigating timeout to UI thread operations
  Move to a different lowering from for nullable value types to work around a bug in TransformCompoundAssignmentLHS. Addressed PR feedback.
  Fix stack overflow in requesting syntax directives (dotnet#36347)
  crash on ClassifyUpdate for EventFields (dotnet#35962)
  fixing bad merge with refactoring that was checked in later
  added basic completion statement telemetry
  Remove duplication in AbstractSymbolCompletionProvider.CreateItems
  Disable move type when the options service isn't present (dotnet#36334)
  Fix crash where type inference doing method inference needs to drop nullability
  Revert "Use IVsSolution to look up IVsHierarchy by project GUID (dotnet#35746)"
  Fix parsing bug in invalid using statements (dotnet#36428)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@tmat tmat tmat approved these changes

Assignees
No one assigned
Labels
Approved to merge Area-IDE Bug Interactive-EnC Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner.
Projects
None yet
Milestone
16.2.P4
4 participants
@ivanbasov @tmat @CyrusNajmabadi @jinujoseph